Fix a number of Intl-related GitHub and test262 issues, add support for ICU 62.1 #5612

jackhorton · 2018-08-20T19:00:58Z

Fixes #637
Fixes #5603
Fixes #3427
Fixes #3513
Fixes #3692

Adds build support for ICU 62.1 by enabling /utf-8 in all ICU files rather than just i18n

@rhuanjl's function.length PR took our pass rate from 486 to 530; this further increases it to 580 (compared to V8's 582 and SpiderMonkey's 597). The remaining failures are all known, but slightly more complicated issues, where we shell out to ICU when the spec wants us to do something particularly snowflake-y -- V8 fails most of these tests as well. The remaining ~350 tests are for features that we don't implement and V8/SM either haven't implemented or have turned off by default.

For what its worth, some of the features we haven't implemented are extremely straightforward -- the Intl version of String.prototype.toLocale{Lower|Upper}Case and {Typed}Array.prototype.toLocaleString are tiny. Fixing those tests (by implementing those features) would bump us up to 596.

jackhorton · 2018-08-20T19:02:33Z

lib/Runtime/Library/EngineInterfaceObject.cpp

+            info->SetAttributes((FunctionInfo::Attributes) (info->GetAttributes() | FunctionInfo::Attributes::ErrorOnNew));
+
+            func->SetAttributes(PropertyIds::prototype, PropertyConfigurable);
+            func->DeleteProperty(PropertyIds::prototype, PropertyOperationFlags::PropertyOperation_Force);


I noticed that DeleteProperty doesn't respect PropertyOperation_Force, at least not the way I thought it should. It makes it so the operation doesn't throw, but it doesn't bypass configurability. I can change that if we all agree that force should bypass any other restrictions.

Since this is used by library code only, are there any cases of non-configurable prototype properties? Do you construct lambdas which go through this path?

Just out of caution I would be inclined to leave Force as it is, but it does sound like the intention is that it skips those kinds of restrictions, at least for adding properties.

We don't currently put any lambdas through this codepath, but theres nothing stopping us from doing so in the future. Additionally, we don't install anything on the prototypes of functions that go through this particular code path, where the prototype gets deleted. Constructor's prototypes get modified, but they don't hit this.

jackhorton · 2018-08-20T19:03:57Z

/cc @zenparsing

jefgen · 2018-08-20T20:25:53Z

deps/Chakra.ICU/Chakra.ICU.Build.props

@@ -36,6 +36,9 @@
        _CRT_SECURE_NO_DEPRECATE;
        %(PreprocessorDefinitions)
      </PreprocessorDefinitions>
+
+      <!-- Some ICU files use embedded UTF-8 -->


FWIW, as of ICU v59, all of the source files are supposed to be encoded as UTF-8.

Yeah, it makes more sense in Chakra.ICU.Build.props anyways.

jefgen · 2018-08-20T20:27:06Z

lib/Runtime/Library/IntlEngineInterfaceExtensionObject.cpp

-
-            // If we passed the previous check, we should reset the status to U_ZERO_ERROR (in case it was U_UNSUPPORTED_ERROR)
-            status = U_ZERO_ERROR;
+            if (strcmp(ucal_getType(cal, &status), "gregorian") == 0)


Thanks Jack! 👍

I can't see much documentation for this; are we expected to free/release the string returned by ucal_getType? Or is it static?

Any idea what the error scenarios are too? If status ends up not being success, is the returned string guaranteed to be nullptr or otherwise safe? Should we be checking the status code prior to using the returned value?

I can assert right after getType, though I believe anything that returns a const char * returns a static string that shouldn't be freed.

Yeah, I would agree that the documentation is rather lacking for this ICU API.

The API will return a static string that does not need to be free'ed. The only way status would be set to a failure state is if the incoming parameter is already a failure. The API itself doesn't modify the status variable.

I've filed a ticket on upstream ICU to improve the docs for this function:
https://unicode-org.atlassian.net/browse/ICU-20082
(Thanks for the feedback! :) )

jefgen

FWIW, LGTM.
Thanks for fixing #5603 👍
(cc: @Microsoft/target-dev as FYI.)

MSLaguana · 2018-08-20T20:30:22Z

lib/Runtime/Library/EngineInterfaceObject.cpp

+
+        func->GetFunctionProxy()->SetIsPublicLibraryCode();
+
+        const char16 *methodNameBuf = methodName->GetSz();


I don't expect the intl js code to do anything too zany here, but is it worth using GetString instead of GetSz? Since we're passing the length around anyway either should work, and I know that @sethbrenith has made some good points that it is typically better hygiene to use the GetString version

We use wcsrchr below, which doesn't take a length parameter. It used to be GetString and we always call it on literal strings, but in case we ever call it on a non-literal-string in the future, the wcsrchr would start searching past the end of the string we actually wanted.

Makes sense.

Also, this runs on Intl initialization, so its not particularly hot. I can add a comment for it.

MSLaguana · 2018-08-20T20:45:49Z

Looks like some TTD tests are complaining about this change.

jackhorton · 2018-08-21T02:12:49Z

Yeah, which is odd. I will try to run the TTD tests locally with ICU enabled -- i hope its not actually an xplat-only failure.

jackhorton · 2018-08-21T05:06:41Z

Last of the low-hanging fruit is done 🔥

zenparsing · 2018-08-21T16:28:33Z

@jackhorton 🔥, indeed

jackhorton · 2018-08-23T23:51:05Z

lib/Runtime/Library/EngineInterfaceObject.cpp

+            info->SetAttributes((FunctionInfo::Attributes) (info->GetAttributes() | FunctionInfo::Attributes::ErrorOnNew));
+
+            AssertOrFailFast(func->GetDynamicType()->GetTypeHandler()->IsDeferredTypeHandler());
+            DynamicTypeHandler::SetInstanceTypeHandler(func, scriptContext->GetLibrary()->GetDeferredFunctionTypeHandlerNoPrototype());


@pleath @boingoing Thoughts on this way of making sure these builtins don't have prototypes, rather than forcefully deleting the prototype property?

@jackhorton I added that type in #5405 and was now planning on renaming it in #5610 (as other function types say "prototype" in their name if they have one and don't mention it if they don't whereas they explicitly mention if they have a length - which this does)

Just an FYI that if 5610 is merged before this and this used here will need to rename it here (unless I drop the name change from 5610)

Yeah I was going to wait to merge this until we got 5610 and fromEntries merged so that you dont have to keep rebasing/re-bytecode-genning on top of all of our changes. I wanted to wait for some more opinion on 5610 but I may just merge it today.

dilijev · 2018-08-24T21:31:45Z

Coming back to this now; sorry for the delay.

dilijev · 2018-08-25T02:15:12Z

lib/Runtime/Base/JnDirectFields.h

+ENTRY(FallbackSymbol)
+
+// This symbol is not part of the regular Symbol API and is only used in rare circumstances in Intl.js for backwards compatibility
+// with the Intl v1 spec. It is visible to the user only using Object.getOwnPropertySymbols(Intl.NumberFormat.call(new Intl.NumberFormat())).


It is visible to the user only using Object.getOwnPropertySymbols(Intl.NumberFormat.call(new Intl.NumberFormat())). [](start = 26, length = 115)

is it supposed to be visible to the user this way?

is it supposed to be visible to the user this way?

Nope

Oh. We have never had the correct behavior here so I just tried to emulate what the other engines do. I believe both V8 and SM showed the symbol through GetOwnPropertySymbols (since the symbol is defined on this, without being an internal property, I am not sure how it could have ever not been user-visible? Perhaps I am missing something.)

Ah, you're correct. I was under the incorrect impression that this was using a private symbol that Dan was alluding to here. It's weird that the spec chose to expose this publicly.

I am not sure how it could have ever not been user-visible?

One way would be for the spec to define the lookup using a WeakMap.

The rest of the spec works off of internal slots; if FallbackSymbol were an internal slot, then that should serve the same purpose, no? Regardless, my understanding was that the fallback symbol was put in place because intl v2 broke a specific library that was using Intl in a relatively odd way, but since at least Edge has never gotten this behavior correct, I assume that the fallback behavior is too rare to be worth trying to update at this point.

dilijev · 2018-08-25T02:15:33Z

lib/Runtime/ByteCode/ByteCodeCacheReleaseFileVersion.h

@@ -4,6 +4,6 @@
 //-------------------------------------------------------------------------------------------------------
 // NOTE: If there is a merge conflict the correct fix is to make a new GUID.

-// {39DEDDF4-EEE4-43E8-A2C8-2550A26007D6}
+// d338af4b-9ef1-48f9-b103-7b2ea58a9478


nit: format of this comment

nit: you shouldn't need a bytecode GUID update because master is in development mode. (Doesn't hurt though.)

In reply to: 212784641 [](ancestors = 212784641)

Meh. Thats the least painful part of the bytecode song and dance.

Also that format is whats generated by the powershell updater script i put in core\tools

Note that the default is still ICU 61.1

@rhuanjl

…t262 issues, add support for ICU 62.1 Merge pull request #5612 from jackhorton:intl/lots-of-fixes Fixes #637 Fixes #5603 Fixes #3427 Fixes #3513 Fixes #3692 Adds build support for ICU 62.1 by enabling /utf-8 in all ICU files rather than just i18n @rhuanjl's function.length PR took our pass rate from 486 to 530; this further increases it to 580 (compared to V8's 582 and SpiderMonkey's 597). The remaining failures are all known, but slightly more complicated issues, where we shell out to ICU when the spec wants us to do something particularly snowflake-y -- V8 fails most of these tests as well. The remaining ~350 tests are for features that we don't implement and V8/SM either haven't implemented or have turned off by default. For what its worth, some of the features we haven't implemented are extremely straightforward -- the Intl version of String.prototype.toLocale{Lower|Upper}Case and {Typed}Array.prototype.toLocaleString are tiny. Fixing those tests (by implementing those features) would bump us up to 596.

jackhorton added the Intl-ICU label Aug 20, 2018

jackhorton self-assigned this Aug 20, 2018

jackhorton requested review from dilijev and jefgen August 20, 2018 19:00

jackhorton commented Aug 20, 2018

View reviewed changes

jackhorton force-pushed the intl/lots-of-fixes branch from 8a397c3 to 0a76dfd Compare August 20, 2018 20:24

jefgen reviewed Aug 20, 2018

View reviewed changes

jefgen approved these changes Aug 20, 2018

View reviewed changes

MSLaguana reviewed Aug 20, 2018

View reviewed changes

MSLaguana approved these changes Aug 20, 2018

View reviewed changes

jackhorton force-pushed the intl/lots-of-fixes branch from 0a76dfd to ba21e9a Compare August 21, 2018 05:05

jackhorton force-pushed the intl/lots-of-fixes branch from ba21e9a to 4c4bbfd Compare August 23, 2018 23:49

jackhorton commented Aug 23, 2018

View reviewed changes

jackhorton force-pushed the intl/lots-of-fixes branch from 4c4bbfd to 9515e87 Compare August 24, 2018 19:50

dilijev added the Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label label Aug 25, 2018

dilijev reviewed Aug 25, 2018

View reviewed changes

jackhorton force-pushed the intl/lots-of-fixes branch from 9515e87 to 748ef18 Compare August 27, 2018 23:26

jackhorton added 4 commits August 27, 2018 22:08

Update Chakra.ICU to support ICU 62.1

2e9a8a8

Note that the default is still ICU 61.1

Fix a number of test262 Intl issues

ecbc1f2

Update bytecode

38c0f59

Update bytecode version

3a6ae50

jackhorton force-pushed the intl/lots-of-fixes branch from 748ef18 to 3a6ae50 Compare August 28, 2018 06:29

chakrabot merged commit 3a6ae50 into chakra-core:master Aug 28, 2018

jackhorton deleted the intl/lots-of-fixes branch August 28, 2018 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a number of Intl-related GitHub and test262 issues, add support for ICU 62.1 #5612

Fix a number of Intl-related GitHub and test262 issues, add support for ICU 62.1 #5612

jackhorton commented Aug 20, 2018 •

edited

Loading

jackhorton Aug 20, 2018

MSLaguana Aug 20, 2018

jackhorton Aug 21, 2018

jackhorton commented Aug 20, 2018

jefgen Aug 20, 2018

jackhorton Aug 20, 2018

jefgen Aug 20, 2018

MSLaguana Aug 20, 2018

MSLaguana Aug 20, 2018

jackhorton Aug 20, 2018

jefgen Aug 20, 2018

jefgen left a comment

MSLaguana Aug 20, 2018

jackhorton Aug 20, 2018

MSLaguana Aug 20, 2018

jackhorton Aug 20, 2018

MSLaguana commented Aug 20, 2018

jackhorton commented Aug 21, 2018

jackhorton commented Aug 21, 2018

zenparsing commented Aug 21, 2018

jackhorton Aug 23, 2018

rhuanjl Aug 25, 2018

jackhorton Aug 25, 2018

dilijev commented Aug 24, 2018 •

edited

Loading

dilijev Aug 25, 2018

gsathya Aug 25, 2018

jackhorton Aug 25, 2018

gsathya Aug 25, 2018

jackhorton Aug 25, 2018

dilijev Aug 25, 2018

dilijev Aug 25, 2018 •

edited

Loading

jackhorton Aug 25, 2018

jackhorton Aug 25, 2018


		func->GetFunctionProxy()->SetIsPublicLibraryCode();

		const char16 *methodNameBuf = methodName->GetSz();

Fix a number of Intl-related GitHub and test262 issues, add support for ICU 62.1 #5612

Fix a number of Intl-related GitHub and test262 issues, add support for ICU 62.1 #5612

Conversation

jackhorton commented Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackhorton commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Aug 20, 2018

jackhorton commented Aug 21, 2018

jackhorton commented Aug 21, 2018

zenparsing commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev commented Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackhorton commented Aug 20, 2018 •

edited

Loading

dilijev commented Aug 24, 2018 •

edited

Loading

dilijev Aug 25, 2018 •

edited

Loading