Might be a stupid idea, but could one maybe transform the string to match in such a way that it can be more simply matched by a JS regex?

If this has a non-negligible impact on resulting JS sizes, can we please also have a setting to opt-out of this new behaviour?

Might be a stupid idea, but could one maybe transform the string to match in such a way that it can be more simply matched by a JS regex?

I had thought about that to implement support for CANON_EQ, but it doesn't work, because it also changes the captured substrings, and that is not correct behavior. So no, we cannot do that. Anyway, the only other feature for which it might help would be ASCII case insensitive, but that's not really hard to implement by transforming the pattern instead, so we don't gain anything.

If this has a non-negligible impact on resulting JS sizes, can we please also have a setting to opt-out of this new behaviour?

I haven't looked at the code size impact yet. Given the size of the implementation in Scala (1.5 kLoC), I guess it will be 3-4 kB of fastOpt code. But no, we won't be able to have a switch to disable the behavior. This is no different from any other method of the JDK. If you don't want its code size, don't use it and use js.RegExp instead. Just like if you don't want the Unicode database embedded in your program, don't use c.isDigit but c >= '0' && c <= '9'.

With the current state of this PR, where I have made some effort not to produce unreasonable code by accident, the size increases are of

• about 100 kB for fastOpt
• about 17 kB for fullOpt

Looks like I was quite off the mark for my wild estimation above. That said, I don't think it fundamentally changes the problem. As I've been working on this, I noticed how broken blindly using js.RegExp was; virtually nothing behaves the same except the most simple combinators. Even simple things like \s or . are incorrect.

I don't think we have precedent for JDK features that are conditional on ES features. How should we address that? Possible ideas are:

I think we should chose safety before generating stuff that doesn't work. Basically:

1. Introduce ESFeatures.useECMAScript20{16-21} and LinkingInfo.assumingES20{16-21} (maybe parametrize?)
2. Use these to link-time DCE the JDK autodetect.
3. Throw when we cannot translate a construct, have a nice error message if an ES feature upgrade could allow the translation.

(our usual approach here is auto-detect and polyfill; that is OK IMO as long as we actually polyfill, which here is probably not up for discussion).

So if somebody ends up using a feature, they simply need to acknowledge in their build once that they are OK with that ES version.

Looks like I was quite off the mark for my wild estimation above. That said, I don't think it fundamentally changes the problem.

While I agree with this reasoning, I think there is another point we need to consider: For all practical intents and purposes, bumping the codesize of a thing by that much is a backwards incompatible change (this is also where the argument is imprecise IMO: for most javalib stuff, we just add new things, so use it or leave it, existing things remain unchanged or get bugfixes that don't affect actual usage for the worse).

With such a big bump, maintainers of codebases that heavily relied on regexes (and have worked "just fine" for ~7 years), would now be faced with the choice of either:

• Stalling Scala.js upgrades (nobody wants that, not us, not our users).
• Investing a considerable amount of effort to migrate off Java regexes (possible, but might involve a lot of new wrapper classes etc. if code is cross compiling).

I do not think we should leave our users in this situation.

I do not have a solution for this right now (that doesn't involve dependency hellscape for at least one side of the equation), so I'll keep thinking.

With such a big bump, maintainers of codebases that heavily relied on regexes (and have worked "just fine" for ~7 years), would now be faced with the choice of either:

Supposedly, a codebase such that adjusting to js.RegExp would be a significant cost is also a large enough codebase that 17 kB won't make a real difference, would it?

I'm more worried about the actual correctness issues, in terms of breaking changes. It's not impossible that there are codebases that actually rely on Pattern behaving like JavaScript, and not as documented in the JavaDoc. Unlike our other fixes to JDK stuff, this one has been documented to behave as it does for 7 years. 😕

Supposedly, a codebase such that adjusting to js.RegExp would be a significant cost is also a large enough codebase that 17 kB won't make a real difference, would it?

Hmmm... That's a good point. Although I'm not sure that's the case. So for example, a case I was concerned about is usage of Pattern in some larger data structures / across library interfaces / underpinning of a library. For example, the scalalib itself is actually an example here:

// from the doc of scala.util.matching.Regex
val date = raw"(\d{4})-(\d{2})-(\d{2})".r

"2004-01-20" match {
  case date(year, month, day) => s"$year was a good year for PLs."
}

Will have to be transformed into:

val date = new JSExtractor(new js.RegExp("(\d{4})-(\d{2})-(\d{2})"))

"2004-01-20" match {
  case date(year, month, day) => s"$year was a good year for PLs."
}

class JSExtractor(...) {
  def unapply(...) = ...
}

But I have no clue how to quantify cost of transformation with library size :-/

Unlike our other fixes to JDK stuff, this one has been documented to behave as it does for 7 years

That is true. OTOH though, as the referenced bugs on this PR prove, it has been considered a bug by many and so far, no fix towards the Java behavior has been filed as a bug / regression.

I think we should chose safety before generating stuff that doesn't work. Basically:

1. Introduce ESFeatures.useECMAScript20{16-21} and LinkingInfo.assumingES20{16-21} (maybe parametrize?)
2. Use these to link-time DCE the JDK autodetect.
3. Throw when we cannot translate a construct, have a nice error message if an ES feature upgrade could allow the translation.

(our usual approach here is auto-detect and polyfill; that is OK IMO as long as we actually polyfill, which here is probably not up for discussion).

So if somebody ends up using a feature, they simply need to acknowledge in their build once that they are OK with that ES version.

I pushed new commits that implement that strategy.

As a bonus, this brings the fullOpt increase down from 17 KB to 15 kB in the default configuration (producing ES 2015 code). That's because the code dealing with \p{} character classes, including the predefinedCharacterClass map, can be dead-code-eliminated. (which is actually quite remarkable)

Anything that has new configuration will require a minor version bump. Since we don't have anything else in flight that can go in a patch release, I suggest we release 1.5.1 now.

@gzm0 If you agree, I prepared a version commit here: sjrd@3746189

@sjrd if you actually changed something in these, please LMK.

I'm pretty sure that, besides rebases, the only thing that has changed over the last month or so is the following diff:
https://github.com/scala-js/scala-js/compare/24ce7cbb67416f1a04f0d39622002b8b9ea23272..b1aa90d795231f4caa446666947884c4b90cd189

@gzm0 I see that you've marked a lot of comments as Resolved, but haven't submitted a new review. Is it just that you accidentally left a Pending Review without submitting it, or you truly haven't finished it yet?

I'm not done yet. Unfortunately resolved state is not part of the review batch (but I absolutely need it, otherwise I have no clue what I still need to look at :P).

Ah ah, yes, that makes sense :)

That LibrarySizeTest seems to be very sensitive to the environment. The results on Windows are significantly different than on Linux. I'm not sure why yet. :-s

It turns out the issues were related to the version of Scala being used.
Note the first two commits in the PR, which are new (and if you want I can submit them as a separate, preliminary PR).

Thanks! 🎉
Rebased and squashed.