fix for toLocaleLowercase parameter handling

[rbri]

No description provided.

[p-bakker]

Should this behavior not be behind a ECMA-402 flag? See https://tc39.es/ecma262/multipage/text-processing.html#sec-string.prototype.tolocalelowercase

[p-bakker]

And if we're going to support the Locale param, we should support it according to the spec, which means also allowing to supply an array with multiple locales and using the best matching one (https://tc39.es/ecma402/#sup-string.prototype.tolocalelowercase)

[rbri]

@p-bakker supporting an array with multiple locales is a bigger story, i only like to catch the low hanging fruits with this

[p-bakker]

@rbri I see you changed things a bit to make it only enabled when language version >= ES6.

I had suggested to put it behind a (new) ECMA_402 flag, as this feature isn't part of of the core EcmaScript standard (ECMA-262), but only part of ECMA-402, which engines can opt to implement or not.

So I think we ought to have a flag to enable/disbale ECMA-402 extensions in Rhino, especially given that we're nowhere close to fully implementing ECMA-402.

Maybe this is something to discuss

supporting an array with multiple locales is a bigger story, i only like to catch the low hanging fruits with this

I understand it's a bigger story, but I wonder if we should do partial-compliant implementations. Again, maybe something to discuss

[gbrail]

I'm happy to merge this, but it sounds like we need to agree on whether we need a new language version for this.

(IMO, we don't -- all these language version flags are very painful and not in line with what other JS engines do. Unless a new JS standard actually breaks something from an older one -- and I sure hope that they don't do that any more -- then "ES6" can be the latest, and eventually the default, option IMO.

[p-bakker]

I think a few things are being mixed up here

First we have the question of whether ECMA-402 extensions like this one (adding a Locale param to toLocaleLowerCase/toLocaleUpperCase) should be behind a feature flag or not

As for language versions: I think that in the process of getting to a v2.0.0 (in which we'll disable/remove all non-ECMA-262- compliant stuff/behavior) we'll have to introduce a Context.VERSION_ECMASCRIPT, which becomes the default in v2.0.0 and from thereonwards we're done with language versions.

The need for Context.VERSION_ECMASCRIPT is twofold:

1. Context.VERISON_ES6 is confusing as ES6 is very old and that language version in Rhino also does all the stuff of subsequent EcmaScript versions
2. Getting to EcmaScript compliance will most likely means introducing some (potentially) breaking changes, so, at least for the time being, we'll need to be able to distinquish between the old, non-compliant behavior/features and the new, EcmaScript-compliant implementations

So with regards to this case: my suggestion is not about putting these changes behind a new language version, but I'm questioning whether they ought to be behind a feature flag

[p-bakker]

Some more thought on this:

• GraalJS has a flag to enable ECMA-402 support as it requires adding ICU data (required for some of the newer features in ECMA-402)
• Rhino already supports just a tiny subset of ECMA-402 (based on the Kangax Compat table), this case would add just a tiny extra piece
• Test262 has tests for ECMA-402
• In our test262 test runner we're NOT including the ECMA-402 tests (presumably because Rhino only supports a tiny subset)

[p-bakker]

BTW: looks like Graal also has language version flags, see https://github.com/oracle/graaljs/blob/master/docs/user/JavaScriptCompatibility.md#ecmascript-language-compliance

[rbri]

Seems like @gbrail can read my mind (hopefully from the code and not from my google profile), i think we are already more or less lost in this version-switch-game.
On the other hand if GraalJS handles this as an independent switch i think we can add such a switch also.

What do the others think and how to come to an decision (i have a HtmlUnit issue for this since some weeks and this is the fix and i don't like to wait soooo much longer....

[p-bakker]

Just to be clear: this is about putting stuff that is not part of the core JavaScript specification behind a feature flag (so not a new language version).

And I'm strongly in favor of (eventually, in v2.0.0) putting all features/behaviors that are not part of the core Ecma262 specification (so also Annex B stuff) behind feature flags, so it's crystal clear what can be expected when executing Rhino.

And given that the full Ecma402 implementation most likely requires additional resources (ICU data), enabling support for Ecma402 should be an explicit thing, so that also means a feature flag imho

[rbri]

@p-bakker @gbrail can you please have a look, i like to make a new HtmlUnit release this weekend and i like to include this

[p-bakker]

LGTM

[gbrail]

This looks good to me and I'm fine with having the feature flag. Thanks!