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

[[SourceText]] type problem #1458

Closed
jmdyck opened this issue Feb 27, 2019 · 16 comments · Fixed by #1547
Closed

[[SourceText]] type problem #1458

jmdyck opened this issue Feb 27, 2019 · 16 comments · Fixed by #1547

Comments

@jmdyck
Copy link
Collaborator

jmdyck commented Feb 27, 2019

From PR #697, each ECMAScript function object now has a [[SourceText]] internal slot. Table 27 says that its type is String. However, most of the steps where it is set are of the form:

Set F.[[SourceText]] to the source text matched by |Nonterminal|.

and source text is "a sequence of [Unicode] code points", which is not a String.

Presumably, [[SourceText]] should get the result of UTF-16 encoding the source text.

(Alternatively, you could defer the UTF-16 encoding to the point where [[SourceText]] is used, in Function.prototype.toString. So the type of [[SourceText]] would be something like "Unicode code points". But then you'd have to UTF-16 decode _sourceText_ in CreateDynamicFunction, which seems silly.)

@devsnek
Copy link
Member

devsnek commented Feb 27, 2019

what if we changed the definition of "source text matched by" to include utf-16 encoding

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 27, 2019

That would pretty much conflict with other uses of the term "source text".

@ljharb
Copy link
Member

ljharb commented Feb 27, 2019

cc @michaelficarra

@michaelficarra
Copy link
Member

I would continue to keep the span of code points in [[SourceText]], UTF-16 encode [[SourceText]] in Function.prototype.toString, and WTF-16 decode the sourceText string built in CreateDynamicFunction.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 27, 2019

@michaelficarra: Interesting. Why do you prefer that alternative?

@gibson042
Copy link
Contributor

Actual UTF-16 encoding won't work because source text can contain unpaired surrogates, but using UTF16Encoding would be fine.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Mar 2, 2019

Right, but note that you can't just pass the whole source text to UTF16Encoding, because it only takes a single code point.

If we take @michaelficarra's preferred approach ([[SourceText]] is code points), encoding only happens in one spot, so it would be enough to use the phrasing that occurs in a couple other places -- the String whose code units are the UTF16Encoding of each code point of [some source text]

But if we take the other approach ([[SourceText]] is a String), then encoding happens in 26 spots, so it might be worth defining an operation for that phrasing. But then you end up saying:

Set F.[[SourceText]] to ThatOperation(the source text matched by |Nonterminal|).

which is a bit clunky.

Instead, it might be better to define an operation that takes the Parse Node as the argument, so you get:

Set F.[[SourceText]] to Whatever(|Nonterminal|).

(I have no good suggestion for the name of either operation.)

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 23, 2019

If I can get an editorial decision on which way this should go, I'll prepare a PR.

@ljharb
Copy link
Member

ljharb commented May 23, 2019

#1458 (comment) seems simplest to me, and I’d generally prefer to defer to @michaelficarra for toString questions anyways.

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 23, 2019

@michaelficarra: I'm curious about Type(func.[[SourceText]]) is String in F.p.toString. For what cases were you expecting the test to fail?

jmdyck added a commit to jmdyck/ecma262 that referenced this issue May 24, 2019
The spec was inconsistent on whether _F_.[[SourceText]] was
a String or a source text (sequence of Unicode code points).

This commit picks the latter.

Resolves issue tc39#1458.
@michaelficarra
Copy link
Member

@jmdyck It's there to prevent Function.prototype.toString from returning non-string values in the event that the host decides to store a non-string in the [[SourceText]] slot.

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 25, 2019

store a non-string in the [[SourceText]] slot.

That would be non-compliant behavior for an ordinary function, so you're talking about an exotic function that elects to have a [[SourceText]] slot, right?

@michaelficarra
Copy link
Member

Any exotic object, yes.

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 25, 2019

Also, while we're in the neighborhood, I noticed that async arrow functions don't get their [[SourceText]] set. Is there a reason that wasn't added in #697?

@ljharb
Copy link
Member

ljharb commented May 25, 2019

That’s likely an oversight. A PR to fix that would be great.

jmdyck added a commit to jmdyck/ecma262 that referenced this issue May 25, 2019
These lines didn't appear in tc39#697.
@ljharb says that was likely an oversight:
tc39#1458 (comment)
@jmdyck
Copy link
Collaborator Author

jmdyck commented May 25, 2019

Done.

ljharb pushed a commit to jmdyck/ecma262 that referenced this issue Jun 2, 2019
These lines didn't appear in tc39#697.
`@ljharb` says that was likely an oversight:
tc39#1458 (comment)
jmdyck added a commit to jmdyck/ecma262 that referenced this issue Oct 24, 2019
The spec was inconsistent on whether _F_.[[SourceText]] was
a String or a source text (sequence of Unicode code points).

This commit picks the latter.

Resolves issue tc39#1458.
jmdyck added a commit to jmdyck/ecma262 that referenced this issue Nov 16, 2019
The spec was inconsistent on whether _F_.[[SourceText]] was
a String or a source text (sequence of Unicode code points).

This commit picks the latter.

Resolves issue tc39#1458.
jmdyck added a commit to jmdyck/ecma262 that referenced this issue Feb 5, 2020
The spec was inconsistent on whether _F_.[[SourceText]] was
a String or a source text (sequence of Unicode code points).

This commit picks the latter.

Resolves issue tc39#1458.
@ljharb ljharb closed this as completed in 2669d45 Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants