Update CaseInsensitive table from hybrid of (Unicode 6.3 and later, up to 8.0) to full Unicode 8.0 support. Fixes #517

[dilijev]

Update CaseInsensitive table from hybrid of (Unicode 6.3 and later, up to 8.0) to full Unicode 8.0 support.

Fixes #517

Note: The current standard wants Unicode 9.0 but it might be too risky to update that far in a stabilization branch. Opened #2367 to track this work item.

The table was generated in the past but then was (mostly) manually edited to include various optimizations and to fix bugs over the years. To make sure we got a complete update, I wrote a tool to generate the table.

CaseInsensitive mapping generator tool

PR: dilijev#3
Source: https://github.com/dilijev/ChakraCore/tree/CaseInsensitive/tools/Unicode/CaseInsensitive

From this tool I was able to see and apply the differences from the current implementation to the correct implementation. In order to keep the change as small as possible, I used the diff as a reference for what needed changing and left out non-essential diffs. Additionally, the tool generates a suite of tests to track regressions against the update and ensure that the implementation does what is expected. I took some key tests from that suite and created the test file contained in this PR.

Overview of Changes

I have staged the changes to hopefully make this easier to review. Here's an overview.

NOTE: The individual commits list or summarize the relevant lines of UnicodeData.txt where applicable.

First, I normalized the existing table to a reasonable format (same as the output of the tool) to make the later commits more clear. This involves fixing the casing and sorting deltas on each line in ascending order.

3d0f37f

Next, I fixed a few bugs with the current table that were preventing some cases from being matched correctly.

abb5d91
4894d24
25049de

Added new codepoints:

f197902 - GREEK LETTER YOT
af2d083 - Cyrillic
cba5439 - Cherokee
6c25a51 - Latin extensions

Other tests and fixes:

cb736ab - Add test cases from #517 to ensure those issues are fixed.
fbfb953 - 0x0345 and case-insensitive equivalent characters with or without /u flag.
dc3e750 - Case-insensitive matching for Cherokee only with /u. [1]
d96eed5 - All other Unicode 8.0 cases of case-insensitive matching only with /u. [1] Added generated tests.

[1] These were with a focus on compat with v8 as determined by running the full regression test suite I generated against node-6.9.4-LTS and node-7.4.0 (latest), and double-checking a handful of tests against the latest stable Chrome (v 55).

Test Coverage

• Regex test run successful! Summary: E:\d\RegexTestCollateral had 151147 tests; 0 failures
• Internal and slow tests pass.
• Note that PRs are merged with the target branch before running Jenkins checks so attempting to run slow tests on this PR would result in failures as per [Slow Tests, Legacy] UnifiedRegex failures in {x64,x86}_{debug,test} (branches: release/1.4, master) #2316 -- but running them locally on this branch, the tests pass.

Reviewers

@tcare @bterlson @boingoing @Cellule - Thank you for your assistance with this change and for your code reviews.

[kunalspathak]

What was the scenario that you are trying to fix? Is there need to add some test cases?

[dilijev]

@kunalspathak - The initial commit I pushed was just a table normalization change that I just wanted to put up for preview so I could show to others. Coming soon (before merging this) will be additional commits which fix #517 and add tests for the same. (I'll push the additional commits and amend the title and message of this PR at that time.)

Edit: the full PR is now up.

[dilijev]

/cc @bterlson @mathiasbynens - Care to take a look?

Also, any chance you could point me to where in the spec it mentions which case-insensitive matches should not be done without /u flag? For this PR I focused on compat with v8 for those codepoints.

[mathiasbynens]

Also, any chance you could point me to where in the spec it mentions which case-insensitive matches should not be done without /u flag?

See step 2 of https://tc39.github.io/ecma262/#sec-runtime-semantics-canonicalize-ch (“If Unicode is true, then … else …”).

I’ll take a closer look at this patch as soon as possible!

[tcare]

For me it seems like reviewing the tool is just as important as the change in this PR, so I would encourage the other reviewers to do the same. Perhaps we need a separate PR for that?

[dilijev]

@tcare I can submit a PR for that but since I don't plan on merging that in, at least not to release/1.4, maybe a PR against my fork would be better -- just to keep things organized. Also I'd say it's not high-enough quality to merge at this time.

PR: dilijev#3
Source: https://github.com/dilijev/ChakraCore/tree/CaseInsensitive/tools/Unicode/CaseInsensitive

[dilijev]

@mathiasbynens Thanks! Basically it seems the difference is without /u mapping under toUpperCase, and with /u, relying on CaseFolding.txt. In a lot of cases, the CaseFolding.txt file provides exactly the same mapping as UnicodeData.txt, but I guess there might be some differences versus what is implemented in toUpperCase?

In the case of our table, we use the MappingSource::UnicodeData line to mark when we do the case mapping irrespective of /u flag (the simple toUpperCase case mappings), and MappingSource::CaseFolding to handle the cases where the CaseFolding mapping provided another alternative that should be considered equivalent.

Basically under /iu we use CaseFolding entries, then fall back on UnicodeData entries if an entry was not found for the code point in question marked as MappingSource::CaseFolding. Under /i we only use the MappingSource::UnicodeData entries.

I'm still surprised about the Cherokee letters, because the Cherokee letters are now split into uppercase and lowercase ranges since Unicode 8.0. It seems like the lowercase/uppercase pairs should still match considering there are a lot of multilingual plane code points with upper/lower pairs for which there is a match without /u.

[dilijev]

I actually generated much larger test files which test all of the equivalences captured in this table. I'll add those tests to this PR.

[dilijev]

I might look into reducing the size of these test files to save space. Minification!

@curtisman Thinks that the tool which generates the table, once reviewed, could probably be checked in to ChakraCore in master (under a brand-new tools directory) after it is cleaned up a bit.

[dilijev]

Didn't save many lines by minifying the tests, but saved nearly 100 KB on final code size. The extra diff in the original test file will still bloat the repo somewhat. Oh well.

[tcare]

Looks good, I also had a quick look at the tool and a discussion with Doug offline. We have v8 compat achieved with the tests on this change so I am happy.

[dilijev]

Thanks @tcare and @bterlson for the offline discussion and for your help as I worked on this fix!

[dilijev]

@mathiasbynens I'm planning to merge this today since I'm going on vacation for 2 weeks and don't want to let this sit here without additional internal coverage. However, I would still greatly appreciate your feedback!

[boingoing]

LGTM

[dilijev]

Thanks @Cellule and @boingoing for your help reviewing!

[mathiasbynens]

Belated LGTM, although I skipped the generated files in favor of the code that generates them, i.e. dilijev#3. 👍 Glad to see this merged!