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

The [[ExportName]] field of ExportEntry Record can also be null #1290

Open
seminaoki opened this Issue Aug 18, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@seminaoki

seminaoki commented Aug 18, 2018

In Table 42: ExportEntry Record Fields, it states that [[ExportName]] filed's value type is String.

But look at the example, [[ExportName]] can also be null, when Export Statement Form is the following:

export * from "mod";

The [[ExportName]] field of ExportEntry Record should be "String | null",
or the [[ExportName]] of export * from "mod"; should not be null.

@jmdyck

This comment has been minimized.

Show comment
Hide comment
@jmdyck

jmdyck Aug 18, 2018

Collaborator

It appears that when an ExportEntry Record has [[ImportName]] = "*", its [[ExportName]] and [[LocalName]] are completely ignored. (ParseModule puts the record in _starExportEntries_, which goes to [[StarExportEntries]], which is then processed in GetExportedNames and ResolveExport.)

So on the one hand, it doesn't matter what [[ExportName]] is set to there. But on the other hand, I agree that the use of the field should conform to its 'type declaration'. The easiest thing would be to change the latter to "String | null". (Personally, I think replacing null with (say) "*ignored*" would be somewhat more informative. Another alternative would be to have 3 'sub-types' of ExportEntry Record, none of which have any null fields.)

Collaborator

jmdyck commented Aug 18, 2018

It appears that when an ExportEntry Record has [[ImportName]] = "*", its [[ExportName]] and [[LocalName]] are completely ignored. (ParseModule puts the record in _starExportEntries_, which goes to [[StarExportEntries]], which is then processed in GetExportedNames and ResolveExport.)

So on the one hand, it doesn't matter what [[ExportName]] is set to there. But on the other hand, I agree that the use of the field should conform to its 'type declaration'. The easiest thing would be to change the latter to "String | null". (Personally, I think replacing null with (say) "*ignored*" would be somewhat more informative. Another alternative would be to have 3 'sub-types' of ExportEntry Record, none of which have any null fields.)

@seminaoki

This comment has been minimized.

Show comment
Hide comment
@seminaoki

seminaoki Aug 18, 2018

I agree that

The easiest thing would be to change the latter to "String | null"

Since other fields of ExportEntry also use "String | null" value type.

seminaoki commented Aug 18, 2018

I agree that

The easiest thing would be to change the latter to "String | null"

Since other fields of ExportEntry also use "String | null" value type.

@devsnek

This comment has been minimized.

Show comment
Hide comment
@devsnek

devsnek Aug 18, 2018

Contributor

wouldn't we say not present or unset here?

Contributor

devsnek commented Aug 18, 2018

wouldn't we say not present or unset here?

@jmdyck

This comment has been minimized.

Show comment
Hide comment
@jmdyck

jmdyck Aug 18, 2018

Collaborator

wouldn't we say not present or unset here?

The spec doesn't use the word "unset" at all, so that seems unlikely. As for "not present", did you mean like this?

ExportEntry Record { ..., [[ExportName]]: not present, ... }

That would amount to treating not present as a (spec) value, which we don't really do.

Collaborator

jmdyck commented Aug 18, 2018

wouldn't we say not present or unset here?

The spec doesn't use the word "unset" at all, so that seems unlikely. As for "not present", did you mean like this?

ExportEntry Record { ..., [[ExportName]]: not present, ... }

That would amount to treating not present as a (spec) value, which we don't really do.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 18, 2018

Member

It could be ~empty~

Member

ljharb commented Aug 18, 2018

It could be ~empty~

@jmdyck

This comment has been minimized.

Show comment
Hide comment
@jmdyck

jmdyck Aug 18, 2018

Collaborator

It could, but:

  • null is clearly the convention for ExportEntry records, and
  • you'd still have to change [[ExportEntry]]'s field type.
Collaborator

jmdyck commented Aug 18, 2018

It could, but:

  • null is clearly the convention for ExportEntry records, and
  • you'd still have to change [[ExportEntry]]'s field type.
@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 18, 2018

Member

Would you? Empty means there’s nothing there, so the type is still string, it’s just optional.

Member

ljharb commented Aug 18, 2018

Would you? Empty means there’s nothing there, so the type is still string, it’s just optional.

@jmdyck

This comment has been minimized.

Show comment
Hide comment
@jmdyck

jmdyck Aug 18, 2018

Collaborator

I don't think there's a precedent in the spec where something with a declared type of T actually allows T or ~empty~. E.g., the [[Target]] field of completion records is explicitly declared as any ECMAScript string or ~empty~.

(In several programming languages, "optional string" is quite distinct from "string". E.g. Rust, Swift, Kotlin, Scala, C#, F#.)

Collaborator

jmdyck commented Aug 18, 2018

I don't think there's a precedent in the spec where something with a declared type of T actually allows T or ~empty~. E.g., the [[Target]] field of completion records is explicitly declared as any ECMAScript string or ~empty~.

(In several programming languages, "optional string" is quite distinct from "string". E.g. Rust, Swift, Kotlin, Scala, C#, F#.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment