Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intl functions shouldn't be constructible #637

Closed
goyakin opened this issue Mar 23, 2016 · 9 comments
Closed

Intl functions shouldn't be constructible #637

goyakin opened this issue Mar 23, 2016 · 9 comments

Comments

@goyakin
Copy link

goyakin commented Mar 23, 2016

According to the spec:

Built-in function objects that are not identified as constructors do not implement the [[Construct]] internal method unless otherwise specified in the description of a particular function

Since our Intl functions are script functions (implemented in Intl.js), they are constructible by default. For example,

new Intl.Collator.prototype.compare

causes the error:

TypeError: Collator.prototype.compare: 'this' is not a Collator object

instead of:

TypeError: Function is not a constructor

@goyakin goyakin added the Bug label Mar 23, 2016
@dilijev dilijev added this to the Backlog milestone Dec 17, 2016
@dilijev dilijev self-assigned this Dec 17, 2016
@dilijev dilijev modified the milestones: Backlog, vNext Dec 20, 2016
@dilijev
Copy link
Contributor

dilijev commented Feb 4, 2017

@bterlson pointed me toward this discussion tc39/ecma402#57 as a potential concern, but tc39/ecma402#57 (comment) seems to suggest the Collator methods are not a problem.

@ljharb
Copy link
Collaborator

ljharb commented Feb 4, 2017

Could they be implemented as arrow functions, or do they need access to this?

@dilijev
Copy link
Contributor

dilijev commented Feb 4, 2017

Unsure. I'll need to understand the spec and the linked discussion a bit better first.

@dilijev
Copy link
Contributor

dilijev commented Feb 4, 2017

For myself for future reference: Intl documentation on MDN

@dilijev dilijev modified the milestones: 1.6, vNext, 1.6 candidates, Backlog May 17, 2017
@dilijev
Copy link
Contributor

dilijev commented Sep 5, 2017

Not a functionality bug per se because we emit the correct kind of error, but the message is perhaps misleading. Adding Dev Experience and unassigning myself for now.

@dilijev dilijev closed this as completed Sep 5, 2017
@dilijev dilijev reopened this Sep 5, 2017
@dilijev dilijev removed their assignment Sep 5, 2017
@dilijev dilijev added this to Backlog in Intl Sep 6, 2017
@dilijev dilijev moved this from Backlog to TODO (General) in Intl Sep 7, 2017
@dilijev dilijev modified the milestones: vNext, Backlog Sep 7, 2017
@dilijev dilijev modified the milestones: Backlog, vNext Sep 7, 2017
@dilijev dilijev moved this from TODO (General) to Backlog in Intl Sep 7, 2017
@jackhorton
Copy link
Contributor

Was playing around with this a bit -- in chakra, anonymous function expressions bound to variables don't take the name of their variables (unlike at least in SpiderMonkey), so my initial belief on turning this into arrow functions is that the best error message we could get is "Function is not a constructor", rather than "getCanonicalLocales is not a constructor". @bterlson thoughts on if thats good enough?

@jackhorton jackhorton self-assigned this Dec 22, 2017
@bterlson
Copy link
Contributor

in chakra, anonymous function expressions bound to variables don't take the name of their variables (unlike at least in SpiderMonkey),

It seems like we do the right thing (let thing = () => {}; thing.name; => "thing"). If we're not doing this it's likely a bug... where are you seeing this behavior?

@bterlson thoughts on if thats good enough?

not good enough, but our errors messages generally need help. "Function is not a constructor" is in the family of "object is null or undefined" though, in that it isn't particularly helpful.

@jackhorton
Copy link
Contributor

image

Seems like this should be fixable. Also interestingly, we definitely don't set the name property of normal function expressions assigned to variables, even though we do set the name of lambdas assigned to variables. I just confirmed this again locally but ran into it in Intl.js before.

@bterlson
Copy link
Contributor

See also: #3407.

@dilijev dilijev added the Intl label Mar 27, 2018
@dilijev dilijev modified the milestones: Backlog, 1.10 May 31, 2018
@digitalinfinity digitalinfinity modified the milestones: 1.10, vNext Aug 1, 2018
chakrabot pushed a commit that referenced this issue Aug 28, 2018
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Intl
Backlog
Development

No branches or pull requests

6 participants