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

Fix comment emit inside for arrow functions. #804

Merged
merged 1 commit into from
May 14, 2018
Merged

Conversation

mprobst
Copy link
Contributor

@mprobst mprobst commented May 14, 2018

Previously, tsickle would synthesize a comment for expression bodies of
arrow functions:

var x = () =>
    // comment
    1;

This would cause the comment to be emitted in between the return
statement and the expression after TS transform:

var x = function() {
    // comment
    return
    // comment
    1;
}

The first comment is TS correctly transforming the arrow function, the
second comment is tsickle re-emitting the synthesized comment, causing
Automatic Semicolon Insertion and thus breaking the code.

The fix is to just not do that - TypeScript emits comments in the
expression correctly, and tsickle already avoids modifying the comment
exactly because emitting comments in arrow functions is dangerous.

Fixes #802.

@mprobst mprobst requested a review from rkirov May 14, 2018 13:32
@mprobst
Copy link
Contributor Author

mprobst commented May 14, 2018

This is a workaround for microsoft/TypeScript#24096.

Copy link
Contributor

@rkirov rkirov left a comment

Choose a reason for hiding this comment

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

Reading the function documentation "prevent the comment text ranges to be emitted, to allow changing these comments.", means that because that trick doesnt work for arrow functions we just reuse the TS mechanism at the cost of not being able to change the comment if we needed to. Is this correct?

// we create here. That can cause a line comment to be emitted after the return, which causes
// Automatic Semicolon Insertion, which then breaks the code. See arrow_fn_es5.ts for an
// example.
if (node.parent && (node as ts.Node).kind !== ts.SyntaxKind.Block &&
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC, why is the 'as ts.Node' cast needed. Looking at <T extends ts.Node> and node: T that should be superfluous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, it's odd indeed. Without it, TS complains that .kind can only be [long list of SyntaxKind union type], but never ts.SyntaxKind.Block. It is though, and it can – TS must be incorrectly inferring a partial subtype here.

@mprobst
Copy link
Contributor Author

mprobst commented May 14, 2018

Reading the function documentation "prevent the comment text ranges to be emitted, to allow changing these comments.", means that because that trick doesnt work for arrow functions we just reuse the TS mechanism at the cost of not being able to change the comment if we needed to. Is this correct?

Yes, that's correct. We don't emit comments for arrow functions like this anyway, because they don't quite work and can cause issues with semicolon insertion (e.g. when returning an arrow function we'd also introduce an incorrect wrap). So this doesn't regress anything that worked previously, but might be something we should revisit once the issue on the TS side is fixed (TS 2.9.1).

Previously, tsickle would synthesize a comment for expression bodies of
arrow functions:

    var x = () =>
        // comment
        1;

This would cause the comment to be emitted in between the return
statement and the expression after TS transform:

    var x = function() {
        // comment
        return
        // comment
        1;
    }

The first comment is TS correctly transforming the arrow function, the
second comment is tsickle re-emitting the synthesized comment, causing
Automatic Semicolon Insertion and thus breaking the code.

The fix is to just not do that - TypeScript emits comments in the
expression correctly, and tsickle already avoids modifying the comment
exactly because emitting comments in arrow functions is dangerous.

Fixes #802.
@mprobst mprobst merged commit db8f394 into master May 14, 2018
@mprobst mprobst deleted the arrow-type-ret branch May 14, 2018 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Placing a comment in a single statement arrow function triggers ASI
3 participants