Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add String.search(regexp) #887

Closed
wants to merge 3 commits into from
Closed

Conversation

klolev
Copy link

@klolev klolev commented Oct 17, 2020

This Pull Request fixes/closes #117 .

It changes the following:

  • Adds String.prototype.search
  • Adds RegExp.prototype[@@search]

This is my first PR here, comments are welcome!

@codecov
Copy link

codecov bot commented Oct 17, 2020

Codecov Report

Merging #887 into master will decrease coverage by 0.09%.
The diff coverage is 58.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
- Coverage   59.28%   59.19%   -0.10%     
==========================================
  Files         155      165      +10     
  Lines        9924    10397     +473     
==========================================
+ Hits         5883     6154     +271     
- Misses       4041     4243     +202     
Impacted Files Coverage Δ
boa/src/builtins/console/mod.rs 22.47% <ø> (+3.37%) ⬆️
boa/src/builtins/error/mod.rs 66.66% <ø> (ø)
boa/src/builtins/string/string_iterator.rs 77.50% <0.00%> (ø)
boa/src/class.rs 0.00% <ø> (ø)
boa/src/environment/global_environment_record.rs 27.95% <0.00%> (ø)
boa/src/lib.rs 86.36% <ø> (ø)
boa/src/object/iter.rs 21.21% <ø> (ø)
boa/src/profiler.rs 0.00% <0.00%> (ø)
boa/src/property/attribute/mod.rs 86.95% <ø> (-3.67%) ⬇️
boa/src/syntax/ast/node/await_expr/mod.rs 0.00% <0.00%> (ø)
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc82aa2...91fb6af. Read the comment docs.

@HalidOdat HalidOdat added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Oct 18, 2020
@RageKnify
Copy link
Member

@flix477 you made this branch over an old commit of the master branch, could you please merge master into this branch so that we get accurate values for the coverage?
I don't think these 2 functions are covered and we'd probably want at least 1 test for String.search, that should be enough to cover most of both functions.

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, but needs some fixes. I also don't think your implementation of String.prototype.search is spec-compliant, rather than building a RegExp object regardless you should check for the @@search property. I don't think GetMethod is currently defined, but getting the property and checking whether it is a function is enough for now, probably leave a TODO in a comment mentioning it.

Comment on lines +517 to +519
if !this.is_object() {
panic!("Not an object");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to point 2. of the spec this should throw a TypeError.
As an example to test this: RegExp.prototype[Symbol.search].call(0, 'test') in Chrome throws: TypeError: Method RegExp.prototype.@@search called on incompatible receiver 0.
I'm not sure .prototype is accessible in boa so I don't know if we can test this.

///
/// [spec]: https://tc39.es/ecma262/#sec-regexp.prototype-@@search
/// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@search
pub(crate) fn search(this: &Value, arg_str: String, ctx: &mut Context) -> Result<Value> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone else can confirm, but I'm pretty sure the signature needs to be (this: &Value, args: &[Value], ctx: &mut Context) -> Result<Value>, all builin functions have the same signature.
This also needs to be declared as a method of the RegExp global object, you can see some examples here:

.method(Self::test, "test", 1)
.method(Self::exec, "exec", 1)
.method(Self::to_string, "toString", 0)

Note that after the algorithm definition the spec mentions the "name" of the function should is "[Symbol.search]".

/// [regex]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions
pub(crate) fn search(this: &Value, args: &[Value], ctx: &mut Context) -> Result<Value> {
let re = match args.get(0) {
Some(arg) if !arg.is_null() && !arg.is_undefined() => RegExp::constructor(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use just 1 function to check for both cases of null and undefined:

boa/boa/src/value/mod.rs

Lines 311 to 313 in 2cb2442

pub fn is_null_or_undefined(&self) -> bool {
matches!(self, Self::Null | Self::Undefined)
}

@Razican
Copy link
Member

Razican commented Dec 11, 2020

Hi @flix477, please, let us know the state of the changes proposed in this PR :) Check the comments by @HalidOdat and let us know if you have any doubts or want any help!

@Razican Razican modified the milestones: v0.11.0, v0.12.0 Jan 11, 2021
@jasonwilliams
Copy link
Member

Hey @flix477 how is this going?

@Razican Razican modified the milestones: v0.12.0, v0.13.0 May 22, 2021
@jedel1043
Copy link
Member

We should close this PR. Both String.prototype.search and RegExp.prototype[@@search] are already implemented.

@HalidOdat HalidOdat closed this Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String.search(regexp)
6 participants