Codefix: add quick fix for missing 'new' operator #27019

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

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

All CLA requirements met.

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

src/compiler/diagnosticMessages.json

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


caller -> call

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

src/compiler/diagnosticMessages.json

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


"to all calls"

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

src/services/codefixes/fixAddMissingNewOperator.ts

+        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

src/services/codefixes/fixAddMissingNewOperator.ts

+    }
+
+    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

src/services/codefixes/fixAddMissingNewOperator.ts

+
+    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

src/services/codefixes/fixAddMissingNewOperator.ts

+    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

src/services/codefixes/fixAddMissingNewOperator.ts

+
+    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

src/services/codefixes/fixAddMissingNewOperator.ts

+        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

src/services/codefixes/fixAddMissingNewOperator.ts

+    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codefix: add quick fix for missing 'new' operator #27019

Codefix: add quick fix for missing 'new' operator #27019

iliashkolyar commented Sep 11, 2018

msftclas commented Sep 11, 2018 •

edited

Loading

DanielRosenwasser Sep 11, 2018

DanielRosenwasser Sep 11, 2018

DanielRosenwasser Sep 11, 2018

iliashkolyar Sep 11, 2018

DanielRosenwasser Sep 11, 2018

iliashkolyar Sep 11, 2018 •

edited

Loading

DanielRosenwasser Sep 11, 2018

iliashkolyar Sep 11, 2018

DanielRosenwasser commented Sep 11, 2018

DanielRosenwasser Sep 12, 2018

DanielRosenwasser Sep 12, 2018

DanielRosenwasser Sep 12, 2018

ghost commented Sep 12, 2018

iliashkolyar commented Sep 12, 2018

ghost commented Sep 12, 2018

iliashkolyar commented Sep 13, 2018

iliashkolyar commented Sep 14, 2018

iliashkolyar commented Sep 30, 2018

DanielRosenwasser Oct 1, 2018

ghost Oct 1, 2018

DanielRosenwasser Oct 1, 2018 •

edited

Loading

DanielRosenwasser Oct 1, 2018

iliashkolyar Oct 2, 2018

DanielRosenwasser commented Oct 1, 2018 •

edited

Loading

iliashkolyar commented Oct 7, 2018

iliashkolyar commented Oct 24, 2018

ghost commented Nov 7, 2018

DanielRosenwasser commented Nov 9, 2018

iliashkolyar commented Nov 10, 2018

iliashkolyar commented Nov 13, 2018

Codefix: add quick fix for missing 'new' operator #27019

Codefix: add quick fix for missing 'new' operator #27019

Conversation

iliashkolyar commented Sep 11, 2018

msftclas commented Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iliashkolyar Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 12, 2018

iliashkolyar commented Sep 12, 2018

ghost commented Sep 12, 2018

iliashkolyar commented Sep 13, 2018

iliashkolyar commented Sep 14, 2018

iliashkolyar commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Oct 1, 2018 • edited Loading

iliashkolyar commented Oct 7, 2018

iliashkolyar commented Oct 24, 2018

ghost commented Nov 7, 2018

DanielRosenwasser commented Nov 9, 2018

iliashkolyar commented Nov 10, 2018

iliashkolyar commented Nov 13, 2018

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