Codefix: add quick fix for missing 'new' operator by iliashkolyar · Pull Request #27019 · microsoft/TypeScript

iliashkolyar · 2018-09-11T06:16:52Z

msftclas · 2018-09-11T06:17:03Z

All CLA requirements met.

DanielRosenwasser · 2018-09-11T06:19:08Z

+        "category": "Message",
+        "code": 95066
+    },
+    "Add missing 'new' operator to caller": {


caller -> call

DanielRosenwasser · 2018-09-11T06:19:19Z

+        "category": "Message",
+        "code": 95067
+    },
+    "Add missing 'new' operator to all callers": {


"to all calls"

DanielRosenwasser · 2018-09-11T06:23:50Z

+        errorCodes,
+        getCodeActions(context) {
+            const { sourceFile, span } = context;
+            const identifierWithoutNew = getIdentifier(sourceFile, span.start);


This is not guaranteed to be an identifier - you really have an arbitrary expression. For example:

class C {} let x = (() => C)()();

Can you add a respective test that ensures it gets corrected to

let x = new ((() => C)())();

This indeed doesn't work at the moment.
I tried working with an expression here but it seems that the token isn't one:
Verbose Debug Information: Node OpenParenToken did not pass test 'isExpression'

When I tried working with its parent element, I received the following:
new (() => C)()() (Missing parentheses after the new addition).
Some help would be very appreciated.

DanielRosenwasser · 2018-09-11T06:29:44Z

+    }
+
+    function addMissingNewOperator(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifierWithoutNew: Identifier): void {
+        const newTypeNode = createNew(identifierWithoutNew, /*typeArguments*/ undefined, /*argumentsArray*/ undefined);


You should carry along the original type arguments and arguments. Can you add a test for these?

class C<T = number> { x?: T; constructor(x: T) { this.x = x; } } C(1, 2, 3); C<string>("hello"); C<boolean>();

Tests Added (without passing the arguments - see comment below).

DanielRosenwasser · 2018-09-11T06:33:52Z

+
+    function addMissingNewOperator(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifierWithoutNew: Identifier): void {
+        const newTypeNode = createNew(identifierWithoutNew, /*typeArguments*/ undefined, /*argumentsArray*/ undefined);
+        changes.replaceNode(sourceFile, identifierWithoutNew, newTypeNode);


I am so surprised that this is working. You should be replacing the CallExpression, not the identifier. The fact that this works is likely a bug.

I think it works because the identifier doesn't have the typeArguments and the arguments themselves here and therefore the new can replace it properly.

I guess it only works in the "simple" scenarios that I added, and indeed in the test without the identifier it doesn't.
Could you please advise on a better approach in which I use the CallExpression?
BTW, the CallExpression doesn't have typeArguments nor arguments, so how can I pass the original ones as you mentioned?

Thanks

DanielRosenwasser · 2018-09-11T06:37:44Z

@Andy-MS can you also take a look?

DanielRosenwasser · 2018-09-12T16:58:55Z

+    function getMissingNewExpression(sourceFile: SourceFile, pos: number): Expression {
+        const token = getTokenAtPosition(sourceFile, pos);
+        Debug.assert(isCallExpression(token.parent));
+        return <Expression>token;


The token isn't necessarily an expression (for example, opening parentheses). I think you should be returning the call expression itself.

DanielRosenwasser · 2018-09-12T16:59:05Z

+
+    function getMissingNewExpression(sourceFile: SourceFile, pos: number): Expression {
+        const token = getTokenAtPosition(sourceFile, pos);
+        Debug.assert(isCallExpression(token.parent));


Debug.assertNode

DanielRosenwasser · 2018-09-12T17:00:53Z

+        return <Expression>token;
+    }
+
+    function addMissingNewOperator(changes: textChanges.ChangeTracker, sourceFile: SourceFile, missingNewExpression: Expression): void {


If you return a call expression above, you now have a CallExpression to work with. From there, you can pass along the (optionaly present) typeArguments and arguments.

ghost · 2018-09-12T17:33:26Z

textChanges doesn't actually need to generate a full source file to do its work -- it just makes local changes to the text. So instead of trying to convert a CallExpression to a NewExpression it's much simpler to just do this:

function addMissingNewOperator(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number): void {
    changes.insertNodeAt(sourceFile, pos, createToken(SyntaxKind.NewKeyword), { suffix: " " });
}

iliashkolyar · 2018-09-12T19:50:12Z

@Andy-MS
Thanks a lot, that is indeed a lot simpler.

I'm still stuck with the (() => C)()() expression which needs a new set of parentheses to wrap the execution of the anonymous function. (Returning new (() => C)()() instead of new ((() => C)())()).

I can detect that the token I'm currently at is not an expression, but I can't seem to find the logic that will give me the (() => C)() expression, which I assume I will then need to replace with a new new node.

Could you please point me as to the best approach for this?
Thanks

ghost · 2018-09-12T23:20:18Z

function addMissingNewOperator(changes: textChanges.ChangeTracker, sourceFile: SourceFile, span: TextSpan): void {
    const call = cast(findAncestorMatchingSpan(sourceFile, span), isCallExpression);
    changes.insertNodeAt(sourceFile, span.start, createToken(SyntaxKind.NewKeyword), { suffix: " " });
    if (isCallExpression(call.expression)) {
        changes.insertNodeAt(sourceFile, call.expression.getStart(sourceFile), createToken(SyntaxKind.OpenParenToken));
        changes.insertNodeAt(sourceFile, call.expression.end, createToken(SyntaxKind.CloseParenToken));
    }
}

function findAncestorMatchingSpan(sourceFile: SourceFile, span: TextSpan): Node {
    let token = getTokenAtPosition(sourceFile, span.start);
    const end = textSpanEnd(span);
    while (token.end < end) {
        token = token.parent;
    }
    return token;
}

iliashkolyar · 2018-09-13T06:40:16Z

@Andy-MS
Thanks. Seems to work properly now.

iliashkolyar · 2018-09-14T05:00:19Z

@DanielRosenwasser can you please take another look?

iliashkolyar · 2018-09-30T11:38:16Z

@DanielRosenwasser - ping.

DanielRosenwasser · 2018-10-01T17:15:46Z

+    function addMissingNewOperator(changes: textChanges.ChangeTracker, sourceFile: SourceFile, span: TextSpan): void {
+        const call = cast(findAncestorMatchingSpan(sourceFile, span), isCallExpression);
+        changes.insertNodeAt(sourceFile, span.start, createToken(SyntaxKind.NewKeyword), { suffix: " " });
+        if (isCallExpression(call.expression)) {


I really feel this is masking a problem with the change tracker API. We shouldn't have to have special checks, the change tracker should do it automatically when replacing nodes, much like how our factories/printers automatically do this.

The problem is that if we replace a node with another it triggers re-formatting of the whole body (and can mess with comments), which could be annoying to users who just expected the codefix to make one local change.

Great example of where this won't work:

var foo; foo()!()

Here, you'll end up with

new foo()!()

instead of

new (foo()!)()

We already have a parenthesizeForNew function in factory.ts: https://github.com/Microsoft/TypeScript/blob/7c875465b5565e9d92a046ec24939c115c8e34cb/src/compiler/factory.ts#L4171-L4184

So the change tracker should be doing something similar here.

Ah, I see, that's a good point. My worry is that this won't be the last time we do something like this though. Maybe for now the right fix is to expose parenthesizeForNew in compiler/utilities.ts and use it here.

Thanks for the feedback!

I'm trying to implement your suggestion but I don't think I fully understand the internals needed for it to work.

According to @Andy-MS, we should not use replaceNode and stick with the insertNodeAt so we won't generate the source file again.
Using the insertNodeAt doesn't really create a new node, but rather adds a new token to the start of the text span (without any change to the existing nodes or the text span).

On the other-hand, you are suggesting to use parenthesizeForNew, which receives an expression and returns it after wrapping it in parentheses if needed.

As I understand it, to work with the parenthesizeForNew API I'll need to create a NewExpression (similarly to what occurs in createNew), which brings us back to the replaceNode logic that we decided to avoid.

The only solution that aligns with both requirements is having a similar logic to the switch case in parenthesizeForNew in my code, that adds the parentheses (using changes.insertNodeAt) in these cases only (replacing the current isCallExpression check).
But again, i'm not sure that this is what you guys are targeting for.

What do you think?

DanielRosenwasser · 2018-10-01T17:32:06Z

P.S. thanks so much for being patient. I want us to get the right fix in and I'm sure we can do that.

iliashkolyar · 2018-10-07T19:25:40Z

@DanielRosenwasser - I decided to use the createNew API as it already contains the parenthesizeForNew logic.
I saw that most of the code fixes currently use replaceNode in their logic, so i'm not sure why this code fix is any different and should avoid using it (as @Andy-MS indicated).

iliashkolyar · 2018-10-24T08:18:33Z

@DanielRosenwasser can you please take another look?

ghost · 2018-11-07T18:09:36Z

@DanielRosenwasser Please review

DanielRosenwasser · 2018-11-09T23:30:46Z

Looks good! If you can address the merge conflicts and update the tests if necessary, we can pull this in for 3.2.

iliashkolyar · 2018-11-10T00:18:25Z

@DanielRosenwasser Ready.

iliashkolyar · 2018-11-13T20:34:03Z

@DanielRosenwasser Is there anything missing for the PR to be merged?

Codefix: add quick fix for missing 'new' operator

76c6ee6

DanielRosenwasser requested changes Sep 11, 2018

View reviewed changes

Initial review

8b14fb2

DanielRosenwasser requested changes Sep 12, 2018

View reviewed changes

InsertNode instead of replace + handling call expressions

7c15378

ghost approved these changes Sep 13, 2018

View reviewed changes

Merge branch 'master' into codefix_add_missing_new_operator

304c6e9

RyanCavanaugh assigned DanielRosenwasser Sep 17, 2018

RyanCavanaugh added this to the TypeScript 3.2 milestone Sep 17, 2018

iliashkolyar added 2 commits September 19, 2018 00:06

Merge branch 'master' into codefix_add_missing_new_operator

35665d1

Merge branch 'master' into codefix_add_missing_new_operator

58b262e

DanielRosenwasser reviewed Oct 1, 2018

View reviewed changes

Code review - remove 'isCallExpression' check

6bd9b76

iliashkolyar closed this Oct 28, 2018

iliashkolyar deleted the codefix_add_missing_new_operator branch October 28, 2018 21:15

iliashkolyar restored the codefix_add_missing_new_operator branch October 28, 2018 21:16

iliashkolyar reopened this Oct 28, 2018

Merge branch 'master' into codefix_add_missing_new_operator

21feb20

DanielRosenwasser approved these changes Nov 9, 2018

View reviewed changes

Merge branch 'master' into codefix_add_missing_new_operator

047b76f

DanielRosenwasser modified the milestones: TypeScript 3.2, TypeScript 3.2.1 Nov 13, 2018

DanielRosenwasser merged commit fe26370 into microsoft:master Nov 15, 2018

iliashkolyar deleted the codefix_add_missing_new_operator branch November 15, 2018 17:42

microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025

Conversation

iliashkolyar commented Sep 11, 2018

Uh oh!

msftclas commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliashkolyar Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 12, 2018

Uh oh!

iliashkolyar commented Sep 12, 2018

Uh oh!

ghost commented Sep 12, 2018

Uh oh!

iliashkolyar commented Sep 13, 2018

Uh oh!

iliashkolyar commented Sep 14, 2018

Uh oh!

iliashkolyar commented Sep 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iliashkolyar commented Oct 7, 2018

Uh oh!

iliashkolyar commented Oct 24, 2018

Uh oh!

ghost commented Nov 7, 2018

Uh oh!

DanielRosenwasser commented Nov 9, 2018

Uh oh!

iliashkolyar commented Nov 10, 2018

Uh oh!

iliashkolyar commented Nov 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

msftclas commented Sep 11, 2018 •

edited

Loading

iliashkolyar Sep 11, 2018 •

edited

Loading

DanielRosenwasser Oct 1, 2018 •

edited

Loading

DanielRosenwasser commented Oct 1, 2018 •

edited

Loading