feat: add full Unicode support for Javascript identifiers

[ctjlewis]

Further resolves #3639, aims to prevent 99.999% of future issues that might occur from using non-English characters (except the few Greek and Latin chars manually included) in an identifier. In the current build, the following errors will occur (including a known issue with U+FF3F):

ERROR - [JSC_PARSE_ERROR] Parse error. Character '〩' (U+3029) is not a valid identifier start char
  1| var a〩 = 10;
          ^

ERROR - [JSC_PARSE_ERROR] Parse error. Character '〺' (U+303A) is not a valid identifier start char
  1| var 〺b = 10;
         ^

The caret is marked in the wrong spot for the last example, but the msg makes it clear that it's U+FF3F causing problems:

ERROR - [JSC_PARSE_ERROR] Parse error. Character '＿' (U+FF3F) is not a valid identifier start char
  1| var ａｅｓｔｈｅｔｉｃ＿;
                  ^

After:

var a〩 = 10;
var a\u3029=10;

var 〺b = 10;
var \u303ab=10;

var ａｅｓｔｈｅｔｉｃ＿;
var \uff41\uff45\uff53\uff54\uff48\uff45\uff54\uff49\uff43\uff3f;

Existing logic is retained for isIdentifierStart and isIdentifierPart, but if the character exists outside of the hardcoded ranges it is checked against regexpu-compiled patterns provided by TC39.

I would not expect that regexpu approach is very performant, but all previous optimizations were migrated into the UnicodeMatch class, so the only impact is if you use a previously unsupported character (which would have thrown an error and exited altogether prior to this patch). I will likely transform the matches into large (a <= ch & ch <= b) blocks to further optimize, but I wanted to put this up for review and feedback (especially regarding short-circuiting and branching conditions, which I am not super familiar with).

Apologies if this is an unwanted PR, I just figured I'd take a crack at it.

Update

I figured manually dumping a chain of bitwise operators would be faster than regexpu, but the compiled regex is consistently faster than the raw bitwise chain unless short-circuiting is used: https://repl.it/@christiantjl/UnicodeMatch

Jupyter notebook for generating the bitwise chain at: https://github.com/christiantjl/unicode-category-ranges/blob/master/Unicode.ipynb

[lauraharker]

Some visibility comments:

This class should be final. I think it shouldn't be public, unless we later find a compelling use case outside this package.

The Google Java best practices also recommends adding an empty private constructor for utility classes to make it clear they should not be instantiated, i.e.

  private UnicodeMatch() {}

[ctjlewis]

Thank you very much Laura, I will make the relevant changes.

[ctjlewis]

If the class is made private, I can't import it inside Scanner.java - extremely sorry if this is basic syntax, I tried to research it, but is there any way to import just public isJavascriptIdentifierStart and public isJavascriptIdentifierPart from private UnicodeMatch without importing the whole class? I tried:

import com.google.javascript.jscomp.parsing.parser.util.UnicodeMatch.isJavascriptIdentifierStart;
import com.google.javascript.jscomp.parsing.parser.util.UnicodeMatch.isJavascriptIdentifierPart;

But it will not build without importing the whole class (while public) via

import com.google.javascript.jscomp.parsing.parser.util.UnicodeMatch;

And referencing with UnicodeMatch.isJavascriptIdentifierStart and so on.

[lauraharker]

Could you also copy over the comment about minimizing branch prediction misses? It's not really clear to me otherwise why we are trying to minimize branches.

About adding the Latin and Greek letters to this fast path: I'm concerned that by adding those letters to this function, we're making the more common case of ASCII characters slower due to the lack of short-circuiting.

It seems potentially useful to have a fast path for these character, if they're common in Angular, but not at the expense of ASCII characters. @concavelenz was the one who initially added this fast path - John, do you have any thoughts?

[concavelenz]

We don't need to include non-ascii (>127) in the fast path.

[ctjlewis]

So, to be clear, I should remove the non-ASCII part of the fast path (fast path effectively becomes just 0 < ch < 127)?

The Latin and Greek optimizations were there before I touched the code - will adjust, but just looking for acknowledgement that I'm not misunderstanding the direction, which may impact Angular build times. I did not insert those optimizations.

[ctjlewis]

Edit: I misunderstood what the note was here, I believe I corrected the logic as needed.

[concavelenz]

How did you generate these values?

[ctjlewis]

These are the regexpu-transpiled patterns provided by TC39: https://github.com/tc39/proposal-regexp-unicode-property-escapes#other-examples

const regexIdentifierStart = /[$_\p{ID_Start}]/u;
const regexIdentifierPart = /[$_\u200C\u200D\p{ID_Continue}]/u;

Online regexpu portal at: https://mothereff.in/regexpu

See also the Updates section of the OP and the JSDOC comments in this class.

[concavelenz]

Do you feel this is insufficient for the native Java builds? Where do you believe you need the JavaScript version?

[ctjlewis]

@concavelenz I believe the reason I removed Character.isLetter was because the GWT build would not work for non-ASCII unicode, due to limitations of J2CL / Guava implementations of those methods. See: google/j2cl#103

Apologies for the lack of specificity, this issue is about a month old and is not super fresh in my mind. Thank you for your reviews, John.

[ctjlewis]

ASCII fast path will work in every call to fastPathIdentifierStart and fastPathIdentifierPart (because the former is evaluated in the latter).

[rishipal]

This isn't merged yet. I'm testing this internally and will update soon.

[rishipal]

I'm seeing multiple internal targets failing to build with this error message - 'The import java.util.regex cannot be resolved'.

[rishipal]

I'll take another stab at this today and repro+debug the failing tests.

[ctjlewis]

Hey @rishipal, I saw your comment about errors, I did not see any when I submitted the PR. Apologies for the delay, I have not had much free time to review this.

[rishipal]

The 'The import java.util.regex cannot be resolved' failures get fixed after switching to re2j regex library. However, I see multiple Scanner tests failing with that. I will update shortly.

[concavelenz]

We have dropped support for the JS (GWT version) of the Closure Compiler in favor of the Graal native image versions. Do you feel that this is still useful?

[ctjlewis]

@concavelenz As I understand it, this was an issue regardless of build - it was just an issue of extended Unicode characters not accurately being counted as valid identifier parts, IIRC (this work is not super fresh in my mind, apologies).

[rishipal]

Update: I tested this again over the weekend. The internal tests are passing now. I will commit this internally.

[ctjlewis]

@rishipal Thank you for your help on this PR. Was this merged internally?

Sorry to bother!

[rishipal]

Hi, this is not merged yet. A comment from John on the pending CL suggests that this may not be needed after we stopped support for GWT/J2CL version of the compiler. I will confirm the priority of this and respond by EOD tomorrow.

[rishipal]

The Scanner test failures did not get fully resolved contrary to my #3647 (comment). This PR is decided to be deprioritized and shelved.