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

[generation] Support comments directly following regular expression litterals #3808

Open
guijemont opened this issue Mar 31, 2023 · 5 comments · May be fixed by #3817
Open

[generation] Support comments directly following regular expression litterals #3808

guijemont opened this issue Mar 31, 2023 · 5 comments · May be fixed by #3817

Comments

@guijemont
Copy link
Contributor

The generation tool does not properly substitute in cases such as:

/foo//*{ bar }*/

Though something like this works:

/foo/ /*{ bar }*/

It seems that find_comments() needs to learn about regexp literals and handle them properly.

See #3807 (comment)

@guijemont
Copy link
Contributor Author

FWIW this branch contains my (broken) attempt at fixing this, unfortunately I don't have more time to spend on this right now.

@gibson042
Copy link
Contributor

I'm fine with fixing /foo//*{ bar }*/, but don't want to support syntactically invalid constructs like /(?/*{ foo }*/-/*{ bar }*/:a)/. I'd rather introduce a new style of procedural test generation with corresponding frontmatter key, even if that style remains inverted (in which templates are referenced by cases) like e.g.

/*---
esid: …

placeholder-prefix: ' %%'
placeholder-suffix: '%% '
---*/

/(? %%subpattern-add-modifiers%% - %%subpattern-remove-modifiers%% :a)/ %%global-modifiers%% ;

for accepting existing .case files like

//- subpattern-add-modifiers
i
//- subpattern-remove-modifiers
//- global-modifiers
gu

to produce content like /(?i-:a)/gu;.

@ptomato
Copy link
Contributor

ptomato commented Apr 4, 2023

I'm not sure I understand the difference - isn't /(? %%foo%% - %%bar%% :a)/ just as syntactically invalid as /(?/*{ foo }*/-/*{ bar }*/:a)/?

@gibson042
Copy link
Contributor

It's invalid at a different layer of abstraction... /(?/*{ foo }*/-/*{ bar }*/:a)/ interferes with parsing of |RegularExpressionLiteral| in the lexical grammar because the first / in /*{ foo }*/ closes the literal, while /(? %%foo%% - %%bar%% :a)/ would be recognized as a single regular expression literal and wouldn't be rejected until application of the RegExp grammar (see Regular Expression Literals: Static Semantics: Early Errors). And the distinction is relevant because the test262 procedural test generation uses a degenerate tokenizer that is ignorant of the ECMAScript syntactic grammar.

@ptomato
Copy link
Contributor

ptomato commented Apr 14, 2023

After poking at this bug, I think I understand better why you are asking for this. It's simple enough to support /foo//*{ bar }*/ but without extra work to support placeholder comments inside RegExp literals, we actually lose the existing support for /(?/*{ foo }*/-/*{ bar }*/:a)/.

Doing this extra work to support placeholder comments inside RegExp literals, however, means that we get all sorts of other arbitrary cases where the /*{ }*/ syntax is never going to work. For example, /foo/*{ bar }*/baz/g would perform a substitution, but //*{ bar }*//g would not, and I don't see a foolproof way to distinguish //*{ bar }*//g from a regular if weird-looking single-line comment without knowing the template writer's intention.

So, in the end I agree that we should use some other tokens to delimit placeholders inside RegExp literals.

ptomato added a commit to ptomato/test262 that referenced this issue Apr 14, 2023
This fixes cases such as `/foo//*{ subst }*/` where previously the `//`
would be interpreted as a single-line comment. We now keep track in
find_comments() of when we are inside a RegExp literal.

However, this breaks cases that used to work (but not tested; maybe this
worked unintentionally?) of `/regexp /*{ subst }*//`.

See: tc39#3808
ptomato added a commit to ptomato/test262 that referenced this issue Apr 14, 2023
This fixes cases such as `/foo//*{ subst }*/` where previously the `//`
would be interpreted as a single-line comment. We now keep track in
find_comments() of when we are inside a RegExp literal.

However, this breaks cases that used to work (but not tested; maybe this
worked unintentionally?) of `/regexp /*{ subst }*//`.

See: tc39#3808

Co-authored-by: Guillaume Emont <[email protected]>
ptomato added a commit to ptomato/test262 that referenced this issue Apr 14, 2023
This fixes cases such as `/foo//*{ subst }*/` where previously the `//`
would be interpreted as a single-line comment. We now keep track in
find_comments() of when we are inside a RegExp literal.

However, this breaks cases that used to work (but not tested; maybe this
worked unintentionally?) of `/regexp /*{ subst }*//`.

See: tc39#3808

Co-authored-by: Guillaume Emont <[email protected]>
ioannad added a commit to ioannad/test262 that referenced this issue Feb 29, 2024
Based on PR tc39#3807 which had generated these tests from templates,
but was stuck due to issue tc39#3808.

Since these tests are a shipping blocker for v8,
and tc39#3808 seems not straight-forward to fix,
I modified the generated tests and removed the templates.

One test is missing, as noted by @rbuckton in tc39#3960 (comment)
to be added in a followup commit.
ptomato pushed a commit to ioannad/test262 that referenced this issue Mar 5, 2024
Based on PR tc39#3807 which had generated these tests from templates,
but was stuck due to issue tc39#3808.

Co-Authored-By: Guillaume Emont <[email protected]>
Co-Authored-By: Ioanna M. Dimitriou H <[email protected]>
ptomato pushed a commit to ioannad/test262 that referenced this issue Mar 5, 2024
Based on PR tc39#3807 which had generated these tests from templates,
but was stuck due to issue tc39#3808.

Co-Authored-By: Guillaume Emont <[email protected]>
Co-Authored-By: Ioanna M. Dimitriou H <[email protected]>
ptomato pushed a commit that referenced this issue Mar 5, 2024
Based on PR #3807 which had generated these tests from templates,
but was stuck due to issue #3808.

Co-Authored-By: Guillaume Emont <[email protected]>
Co-Authored-By: Ioanna M. Dimitriou H <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants