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

[names] Add a section that clarifies how to emit names for JS #122

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

szuend
Copy link
Collaborator

@szuend szuend commented Aug 26, 2024

This is a first draft that clarifies on which JS tokens we expect mappings with a name.

I tried to stick to the ECMA-262 AST on a level where it makes sense, but let me know what you think. I also used lang

A nice tie-in with the "Scopes" proposal is, that the ( token that shows up in the definition is also what we'll probably recommend as the "generated range" start, so that would fit rather well together.

Thinks that still need clarification:

  • Phrasing around the conditions "when" to emit a mapping with a name. Since we don't know the source language we need to keep it generic but we should make it clear that it should also make sense semantically.
  • The current phrasing uses "should" and "may". I'm not sure if we should phrase it as "must"?
  • Should we spell out all the AST constructs for variables where we expect mappings with a name?
  • Figure out if we want to allow more complex expressions for variable names, e.g. if a minifier turned obj.x into f.
  • Any other open question you might come up with :)

@szuend szuend marked this pull request as draft August 26, 2024 11:43
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
@nicolo-ribaudo nicolo-ribaudo added this to the October 2024 milestone Sep 11, 2024
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
source-map.bs Outdated Show resolved Hide resolved
In particular:

  * For functions/methods/generators emit the named mapping on the
    binding identifier by default (or opening parenthesis).

  * Special case single argument arrow functions that have neither
    a binding identifier or an opening parenthesis.

  * Introduce a definition for "named mapping". I.e. a decoded
    mapping entry where the name field is not null.

  * Call out that generators may emit additional "named mappings"
    as they require. Either to support existing tools or to fit
    their requirements.
@szuend szuend marked this pull request as ready for review September 27, 2024 09:31
@szuend
Copy link
Collaborator Author

szuend commented Sep 27, 2024

Hey all, I incorporated the feedback form yesterday's meeting. Thanks a ton for the fruitful discussion! In particular:

  • For functions/methods/generators emit the named mapping on the
    binding identifier by default (or opening parenthesis if no identifier is present).

  • Special case single argument arrow functions that have neither
    a binding identifier or an opening parenthesis.

  • Introduce a definition for "named mapping". I.e. a decoded
    mapping entry where the name field is not null.

  • Call out that generators may emit additional "named mappings"
    as they require. Either to support existing tools or to fit
    their requirements.

English is not my native language and writing spec text is not my strong suite, so I'd welcome some word smithing/rephrasing to clear up the meaning. I also spelled out some more things that require review. Thanks!

source-map.bs Outdated
the name of the original source language construct. A [=decoded mapping|mapping=] with
a non-null [=decoded mapping/name=] is called a <dfn>named mapping</dfn>.

Example: A TypeScript function compiled to a JavaScript arrow function, or a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, TypeScript does not transpile a function to an arrow function in any case. Instead, we might transpile an arrow function with type information to an arrow function without, or an arrow function to a function as part of down-leveling to ES5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with "A minifier renaming functions and variables or removing function names
from immediately invoked function expressions. "

source-map.bs Outdated
Source map generators may chose to emit a [=named mapping=] on the opening parenthesis regardless of
the presence of the [=BindingIdentifier=].

1. The [=BindingIdentifier=] for [=ArrowFunction=], and [=AsyncArrowFunction=] if it
Copy link
Collaborator

@rbuckton rbuckton Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph is confusing and doesn't properly address the concern I raised. It now says to emit a name mapping on (a) either the BindingIdentifier or ( of an arrow function, or (b) the => if it is a single argument arrow function with neither a BindingIdentifier nor Arguments.

(b) is never true as a single argument arrow function has only a BindingIdentifier, and thus (a) is only the ever case that is true.

Correct me if I'm wrong, but I believe we discussed in the meeting that there are two possible places to emit a name mapping for an arrow function or async arrow function:

  1. For an ArrowFunction or AsyncArrowFunction with Arguments, the name mapping is attached to the opening (. A generator may opt to also emit a name mapping attached to the => for consistency with (2).
  2. For an ArrowFunction or AsyncArrowFunction with a SingleNameBinding, the name mapping is attached to the =>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm embarrassed to say that I don't know what I was thinking when I wrote this.

I fixed it up as proposed but with concrete references to the productions.

takikawa added a commit to takikawa/source-map-spec that referenced this pull request Oct 1, 2024
This matches the spec with PR tc39#122
@jkup jkup merged commit 4abf9df into tc39:main Oct 30, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Oct 30, 2024
SHA: 4abf9df
Reason: push, by jkup

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@szuend szuend deleted the clarify-names branch November 5, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants