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

Address feedback from plenary #77

Merged
merged 5 commits into from
May 13, 2024
Merged

Address feedback from plenary #77

merged 5 commits into from
May 13, 2024

Conversation

bakkot
Copy link
Collaborator

@bakkot bakkot commented Apr 9, 2024

Commits should be reviewed individually. Summary:

  • use \$ etc where possibly; this is not possible for all punctuators, but it is for some
  • also escape line terminators; this was just an oversight on my part
  • use \n etc where possible
  • if given a lone surrogate, escape it, so that if there is someday a non-BMP whitespace character, and we get x-mode RegExps, and someone writes new RegExp(RegExp.escape('\uHEAD') + RegExp.escape('\uTAIL'), 'x') where \uHEAD\uTAIL encodes that non-BMP whitespace character, the resulting RegExp will match the character instead of being interpreted as whitespace and therefore (in x-mode) ignored
  • hex-escape ASCII letters at the start of the string so that new RegExp('\c' + RegExp.escape('Z')) will either be an error (in u- or v-mode RegExps) or match the string '\\cZ' (in other RegExps); this also has the effect that the result cannot combine with a preceding \x or \u when the first character is A-F or a-f. Fixes Which leading characters should be escaped? #66.

@bakkot
Copy link
Collaborator Author

bakkot commented Apr 9, 2024

cc @erights @waldemarhorwat @gibson042 Please take a look.

@bakkot bakkot force-pushed the address-feedback branch 2 times, most recently from 7843108 to a839978 Compare April 9, 2024 05:13
spec.emu Outdated
@@ -62,7 +62,7 @@ contributors:
1. Return the string-concatenation of 0x005C (REVERSE SOLIDUS) and the string in the "ControlEscape" column of the row whose "Code Point" column contains _c_.
1. Let _otherPunctuators_ be the string-concatenation of *",-=<>#&!%:;@~'`"* and the code unit 0x0022 (QUOTATION MARK).
1. Let _toEscape_ be StringToCodePoints(_otherPunctuators_).
1. If _toEscape_ contains _c_ or _c_ is matched by |WhiteSpace| or |LineTerminator|, then
1. If _toEscape_ contains _c_, _c_ is matched by |WhiteSpace| or |LineTerminator|, or _c_ is in the inclusive interval from U+D800 to U+DFFF (i.e. _c_ is a leading surrogate or trailing surrogate), then
Copy link
Member

Choose a reason for hiding this comment

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

          1. If _toEscape_ contains _c_, _c_ is matched by |WhiteSpace| or |LineTerminator|, or _c_ is a leading surrogate or a trailing surrogate, then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strictly speaking, leading and trailing surrogates are defined to be code units, not code points. I figure it's close enough for the parenthetical, but not the actual algorithm step. I could change the parenthetical to be "c is the code point corresponding to a leading surrogate or trailing surrogate" but that's getting pretty wordy.

Copy link
Member

Choose a reason for hiding this comment

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

other than the oxford comma, this suggestion is a nit, so i'm ambivalent

spec.emu Show resolved Hide resolved
@erights
Copy link

erights commented Apr 9, 2024

How do I see a rendered form of this?

@bakkot
Copy link
Collaborator Author

bakkot commented Apr 9, 2024

I've put up a rendering here.

@erights
Copy link

erights commented Apr 9, 2024

Thanks. FWIW LGTM, but I delegate an approval decision to @gibson042 who understands this better than I do.

@erights
Copy link

erights commented Apr 9, 2024

Thanks for the rendering. Cut down the review effort for me by a huge factor.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Nice catch on LineTerminator! I still prefer the universally applicable \x…/\u… approach, but this does work (even if it could get stale).

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
Comment on lines +61 to +62
1. If _c_ is matched by |SyntaxCharacter| or _c_ is U+002F (SOLIDUS), then
1. Return the string-concatenation of 0x005C (REVERSE SOLIDUS) and UTF16EncodeCodePoint(_c_).
Copy link
Contributor

Choose a reason for hiding this comment

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

This strikes me as a point-in-time snapshot, and I'm not a fan because it will be weird if e.g. \@ becomes a valid escape in the future. My preference remains the universally applicable \x…/\u… approach. But that said, there is no technical issue here; merely an aesthetic one.

1. NOTE: Escaping a leading digit ensures that output corresponds with pattern text which may be used after a `\0` character escape or a |DecimalEscape| such as `\1` and still match _S_ rather than be interpreted as an extension of the preceding escape sequence.
1. Set _escaped_ to the string-concatenation of _escaped_, the code unit 0x005C (REVERSE SOLIDUS), *"x3"*, and the code unit whose numeric value is the numeric value of _c_.
1. If _escaped_ is the empty String, and _c_ is matched by |DecimalDigit| or |AsciiLetter|, then
1. NOTE: Escaping a leading digit ensures that output corresponds with pattern text which may be used after a `\0` character escape or a |DecimalEscape| such as `\1` and still match _S_ rather than be interpreted as an extension of the preceding escape sequence. Escaping a leading ASCII letter does the same for the context after `\c`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this sentence is too long to not use a parenthetical.

Suggested change
1. NOTE: Escaping a leading digit ensures that output corresponds with pattern text which may be used after a `\0` character escape or a |DecimalEscape| such as `\1` and still match _S_ rather than be interpreted as an extension of the preceding escape sequence. Escaping a leading ASCII letter does the same for the context after `\c`.
1. NOTE: Escaping a leading digit ensures that output corresponds with pattern text, which may be used after a `\0` character escape or a |DecimalEscape| such as `\1`, and still match _S_ rather than be interpreted as an extension of the preceding escape sequence. Escaping a leading ASCII letter does the same for the context after `\c`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those commas read as ungrammatical to me. In particular the second comma cuts a clause in the middle - the pattern text "maybe used after \0 [...] and still match S". Open to other rephrasing here but I don't like this particular suggestion.

1. NOTE: Escaping a leading digit ensures that output corresponds with pattern text which may be used after a `\0` character escape or a |DecimalEscape| such as `\1` and still match _S_ rather than be interpreted as an extension of the preceding escape sequence.
1. Set _escaped_ to the string-concatenation of _escaped_, the code unit 0x005C (REVERSE SOLIDUS), *"x3"*, and the code unit whose numeric value is the numeric value of _c_.
1. If _escaped_ is the empty String, and _c_ is matched by |DecimalDigit| or |AsciiLetter|, then
1. NOTE: Escaping a leading digit ensures that output corresponds with pattern text which may be used after a `\0` character escape or a |DecimalEscape| such as `\1` and still match _S_ rather than be interpreted as an extension of the preceding escape sequence. Escaping a leading ASCII letter does the same for the context after `\c`.
Copy link
Member

Choose a reason for hiding this comment

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

Also, TIL \c0 is a valid escape. This also prevents new RegExp("\\" + escape('n')) from combining.

spec.emu Show resolved Hide resolved
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 this pull request may close these issues.

Which leading characters should be escaped?
5 participants