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.

Sign up for GitHub

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

Jump to bottom

Various fixes and implementations to NativeRegExp #1434

Merged
merged 5 commits into from
Jan 24, 2024
Merged

Various fixes and implementations to NativeRegExp #1434

merged 5 commits into from
Jan 24, 2024

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Jan 5, 2024

Slightly modified (improved) PR from @duonglaiquang. See HtmlUnit#12

This PR does the following:

  1. Handle the case where RegExp.prototype.toString is called on NativeObject instead of NativeRegExp object (currently throwing TypeError: Method "toString" called on incompatible object)
  • These tests showcase the expected behavior of this method 
    <!DOCTYPE html>
<html>
<head>
<script>
console.log(RegExp.prototype.toString.call({})) // "/undefined/undefined"
console.log(RegExp.prototype.toString.call({source: "Foo"})) // "/Foo/undefined"
console.log(RegExp.prototype.toString.call({flags: "gy"})) // "/undefined/gy"
console.log(RegExp.prototype.toString.call({source: "Foo", flags: "g"})) // "/Foo/g"
console.log(RegExp.prototype.toString.call({source: "Foo", flags: "g", sticky: true})) // "/Foo/g"
</script>
</head>
<body>
</body>
</html>

  1. Implement RegExp.dotAll flag

  2. Implement String.prototype.replaceAll

  3. Fix an issue when String.includes/startsWith/endsWith throw TypeError when the first argument is a regex, even if Symbol.match of that regex has been set to false.

    This function is also used to identify if objects have the behavior of regular expressions. For example, the methods String.prototype.startsWith(), String.prototype.endsWith() and String.prototype.includes(), check if their first argument is a regular expression and will throw a TypeError if they are. Now, if the match symbol is set to false (or a Falsy value except undefined), it indicates that the object is not intended to be used as a regular expression object.

  • These tests showcase the buggy behavior of the current implementation. 
    <!DOCTYPE html>
<html>
<head>
<script>
var regExp = /./;
try {
  console.log("/./".includes(regExp))
} catch (e) {
  console.log(e); // TypeError: First argument to String.prototype.includes must not be a regular expression
  regExp[Symbol.match] = false;
  console.log("/./".includes(regExp)) // expected: true, got TypeError
}
</script>
</head>
<body>
</body>
</html>

rbri added a commit to HtmlUnit/htmlunit-rhino-fork that referenced this pull request Jan 5, 2024
@rbri
Pull Request mozilla#1434: Various fixes and implementations to Nativ…
f73614e 
…eRegExp
@rbri rbri mentioned this pull request Jan 5, 2024
Closed
@gbrail
Copy link
Collaborator

gbrail commented Jan 15, 2024

Thanks for contributing this -- it's always great to make more tests pass.

Before we finish and merge this, is there a way to set up this PR so that the original author gets some credit? For example, could we prevail on him to push his PR to this repo in addition to HtmlUnit?

@rbri
Copy link
Collaborator Author

rbri commented Jan 15, 2024

Before we finish and merge this, is there a way to set up this PR so that the original author gets some credit? For example, could we prevail on him to push his PR to this repo in addition to HtmlUnit?

@duonglaiquang any idea about this?

@atnak
Copy link
Contributor

atnak commented Jan 16, 2024

@gbrail Thank you for your concern about the credit thing. I work with @duonglaiquang and I will respond.

We use HtmlUnit (and indirectly rhino) in our business operations and we periodically push fixes upstream because it also benefits us. We're not in it for credit and these changes are not always created solely by the person pushing. @rbri doing these PRs for us is cool because it saves us a lot of time and dedication fixing code and test cases to meet rhino's / htmlunit's styles.

If credit is the only concern, we're cool without 👍

@gbrail
Copy link
Collaborator

gbrail commented Jan 24, 2024

I get it about the credit -- thank you for your contribution!

@gbrail gbrail merged commit 3f5f6cc into mozilla:master Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rbri @gbrail @atnak