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

15 changes: 11 additions & 4 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4608,7 +4608,6 @@
"category": "Message",
"code": 95062
},

"Add missing enum member '{0}'": {
"category": "Message",
"code": 95063
Expand All @@ -4619,10 +4618,18 @@
},
"Convert to async function":{
"category": "Message",
"code": 95065
"code": 95065
},
"Convert all to async functions": {
"category": "Message",
"code": 95066
"category": "Message",
"code": 95066
},
"Add missing 'new' operator to caller": {
Copy link
Member

Choose a reason for hiding this comment

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

caller -> call

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

Choose a reason for hiding this comment

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

"to all calls"

"category": "Message",
"code": 95068
}
}
29 changes: 29 additions & 0 deletions src/services/codefixes/fixAddMissingNewOperator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/* @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 identifierWithoutNew = getIdentifier(sourceFile, span.start);
Copy link
Member

Choose a reason for hiding this comment

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

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)())();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const changes = textChanges.ChangeTracker.with(context, t => addMissingNewOperator(t, sourceFile, identifierWithoutNew));
return [createCodeFixAction(fixId, changes, Diagnostics.Add_missing_new_operator_to_caller, fixId, Diagnostics.Add_missing_new_operator_to_all_callers)];
},
fixIds: [fixId],
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) =>
addMissingNewOperator(changes, context.sourceFile, getIdentifier(diag.file, diag.start))),
});

function getIdentifier(sourceFile: SourceFile, pos: number): Identifier {
const token = getTokenAtPosition(sourceFile, pos);
Debug.assert(token.kind === SyntaxKind.Identifier);
Debug.assert(isCallExpression(token.parent));
Copy link
Member

Choose a reason for hiding this comment

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

Debug.assertNode

return <Identifier>token;
}

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

Choose a reason for hiding this comment

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

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>();

Copy link
Contributor Author

@iliashkolyar iliashkolyar Sep 11, 2018

Choose a reason for hiding this comment

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

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

changes.replaceNode(sourceFile, identifierWithoutNew, newTypeNode);
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

}
}
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
13 changes: 13 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingNew.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/// <reference path='fourslash.ts' />

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

verify.codeFix({
description: "Add missing 'new' operator to caller",
index: 0,
newFileContent: `class C {
}
var c = new C();`
});
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 callers",
newFileContent:
`class C {
constructor(num?: number) {}
}
var a = new C();
var b = new C(3);`
});