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

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

9 changes: 8 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4659,7 +4659,6 @@
"category": "Message",
"code": 95062
},

"Add missing enum member '{0}'": {
"category": "Message",
"code": 95063
Expand All @@ -4683,5 +4682,13 @@
"Generate types for all packages without types": {
"category": "Message",
"code": 95068
},
"Add missing 'new' operator to call": {
"category": "Message",
"code": 95069
},
"Add missing 'new' operator to all calls": {
"category": "Message",
"code": 95070
}
}
34 changes: 34 additions & 0 deletions src/services/codefixes/fixAddMissingNewOperator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/* @internal */
namespace ts.codefix {
const fixId = "addMissingNewOperator";
const errorCodes = [Diagnostics.Value_of_type_0_is_not_callable_Did_you_mean_to_include_new.code];
registerCodeFix({
errorCodes,
getCodeActions(context) {
const { sourceFile, span } = context;
const changes = textChanges.ChangeTracker.with(context, t => addMissingNewOperator(t, sourceFile, span));
return [createCodeFixAction(fixId, changes, Diagnostics.Add_missing_new_operator_to_call, fixId, Diagnostics.Add_missing_new_operator_to_all_calls)];
},
fixIds: [fixId],
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) =>
addMissingNewOperator(changes, context.sourceFile, diag)),
});

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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Oct 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

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;
}
}
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"codefixes/importFixes.ts",
"codefixes/fixSpelling.ts",
"codefixes/fixAddMissingMember.ts",
"codefixes/fixAddMissingNewOperator.ts",
"codefixes/fixCannotFindModule.ts",
"codefixes/fixClassDoesntImplementInheritedAbstractMember.ts",
"codefixes/fixClassSuperMustPrecedeThisAccess.ts",
Expand Down
14 changes: 14 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingNew.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />

////class C {
////}
////var c = C();

verify.codeFix({
description: "Add missing 'new' operator to call",
index: 0,
newFileContent:
`class C {
}
var c = new C();`
});
14 changes: 14 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingNew2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />

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

verify.codeFix({
description: "Add missing 'new' operator to call",
index: 0,
newFileContent:
`class C {
}
let x = new ((() => C)())();`
});
16 changes: 16 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingNew3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/// <reference path='fourslash.ts' />

////class C {
////}
////let x = [C];
////let a = x[0]();

verify.codeFix({
description: "Add missing 'new' operator to call",
index: 0,
newFileContent:
`class C {
}
let x = [C];
let a = new x[0]();`
});
18 changes: 18 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingNew4.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path='fourslash.ts' />

////class C {
////}
////class D {
////}
////let x = (true ? C : D)();

verify.codeFix({
description: "Add missing 'new' operator to call",
index: 0,
newFileContent:
`class C {
}
class D {
}
let x = new (true ? C : D)();`
});
18 changes: 18 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingNew_all.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path='fourslash.ts' />

////class C {
//// constructor(num?: number) {}
////}
////var a = C();
////var b = C(3);

verify.codeFixAll({
fixId: "addMissingNewOperator",
fixAllDescription: "Add missing 'new' operator to all calls",
newFileContent:
`class C {
constructor(num?: number) {}
}
var a = new C();
var b = new C(3);`
});
22 changes: 22 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingNew_all_arguments.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/// <reference path='fourslash.ts' />

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

verify.codeFixAll({
fixId: "addMissingNewOperator",
fixAllDescription: "Add missing 'new' operator to all calls",
newFileContent:
`class C<T = number> {
x?: T;
constructor(x: T) { this.x = x; }
}
let a = new C(1, 2, 3);
let b = new C<string>("hello");
let c = new C<boolean>();`
});