-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Quick fix for functions lacking return expressions #26434
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
Changes from 17 commits
3f59aa9
7671221
0b1e6cc
1f9b9c0
aca1722
7cfd0fb
cbe59bb
48f1cca
98d7eba
537e806
505e299
6fecf32
4c0af72
98377f3
49d21bf
a2b15ed
eabc5a4
944ddf4
6c88a01
94f845a
a7f37a3
7fe9a82
af9552e
601fc5e
175cf4e
8b332fe
5d8355c
522cac8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,218 @@ | ||
/* @internal */ | ||
namespace ts.codefix { | ||
const fixId = "returnValueCorrect"; | ||
const fixIdAddReturnStatement = "fixAddReturnStatement"; | ||
const fixIdRemoveBlockBodyBrace = "fixRemoveBlockBodyBrace"; | ||
const fixIdReplaceBraceWithParen = "fixReplaceBraceWithParen"; | ||
const fixIdWrapTheBlockWithParen = "fixWrapTheBlockWithParen"; | ||
const errorCodes = [ | ||
Diagnostics.A_function_whose_declared_type_is_neither_void_nor_any_must_return_a_value.code, | ||
Diagnostics.Type_0_is_not_assignable_to_type_1.code, | ||
Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1.code | ||
]; | ||
|
||
enum FixKind { | ||
MissingReturnStatement, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arguably, these are problem kinds. The corresponding fix kinds would be "AddReturnStatement" and "AddParentheses". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are there only two kinds of fixes? Shouldn't there also be one for removing the braces? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And why not just use the fix IDs from above? |
||
MissingParentheses | ||
} | ||
|
||
interface MissingReturnInfo { | ||
kind: FixKind.MissingReturnStatement; | ||
declaration: FunctionLikeDeclaration; | ||
expression: Expression; | ||
} | ||
|
||
interface MissingParenInfo { | ||
kind: FixKind.MissingParentheses; | ||
declaration: ArrowFunction; | ||
expression: Expression; | ||
} | ||
|
||
type Info = MissingReturnInfo | MissingParenInfo; | ||
|
||
registerCodeFix({ | ||
errorCodes, | ||
fixIds: [fixIdAddReturnStatement, fixIdRemoveBlockBodyBrace, fixIdReplaceBraceWithParen, fixIdWrapTheBlockWithParen], | ||
getCodeActions: context => { | ||
const { program, sourceFile, span: { start }, errorCode } = context; | ||
const info = getInfo(program.getTypeChecker(), sourceFile, start, errorCode); | ||
if (!info) return undefined; | ||
|
||
if (info.kind === FixKind.MissingReturnStatement) { | ||
return concatenate( | ||
[getActionForfixAddReturnStatement(context, info.declaration, info.expression)], | ||
isArrowFunction(info.declaration) ? [ | ||
getActionForfixRemoveBlockBodyBrace(context, info.declaration, info.expression), | ||
getActionForfixReplaceBraceWithParen(context, info.declaration, info.expression) | ||
] : undefined); | ||
} | ||
else { | ||
return [getActionForfixWrapTheBlockWithParen(context, info.declaration, info.expression)]; | ||
} | ||
}, | ||
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { | ||
const info = getInfo(context.program.getTypeChecker(), diag.file, diag.start, diag.code); | ||
if (!info) return undefined; | ||
|
||
switch (context.fixId) { | ||
case fixIdAddReturnStatement: | ||
addReturnStatement(changes, diag.file, info.declaration, info.expression); | ||
break; | ||
case fixIdRemoveBlockBodyBrace: | ||
if (!isArrowFunction(info.declaration)) return undefined; | ||
removeBlockBodyBrace(changes, diag.file, info.declaration, info.expression, /* withParen */ false); | ||
break; | ||
case fixIdReplaceBraceWithParen: | ||
if (!isArrowFunction(info.declaration)) return undefined; | ||
removeBlockBodyBrace(changes, diag.file, info.declaration, info.expression, /* withParen */ true); | ||
break; | ||
case fixIdWrapTheBlockWithParen: | ||
if (!isArrowFunction(info.declaration)) return undefined; | ||
wrapBlockWithParen(changes, diag.file, info.declaration, info.expression); | ||
break; | ||
default: | ||
Debug.fail(JSON.stringify(context.fixId)); | ||
} | ||
}), | ||
}); | ||
|
||
function updateFunctionLikeBody(declaration: FunctionLikeDeclaration, body: Block): FunctionLikeDeclaration { | ||
Kingwl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
switch (declaration.kind) { | ||
case SyntaxKind.FunctionDeclaration: | ||
return createFunctionDeclaration(declaration.decorators, declaration.modifiers, declaration.asteriskToken, declaration.name, declaration.typeParameters, declaration.parameters, declaration.type, body); | ||
case SyntaxKind.MethodDeclaration: | ||
return createMethod(declaration.decorators, declaration.modifiers, declaration.asteriskToken, declaration.name, declaration.questionToken, declaration.typeParameters, declaration.parameters, declaration.type, body); | ||
case SyntaxKind.GetAccessor: | ||
return createGetAccessor(declaration.decorators, declaration.modifiers, declaration.name, declaration.parameters, declaration.type, body); | ||
case SyntaxKind.SetAccessor: | ||
return createSetAccessor(declaration.decorators, declaration.modifiers, declaration.name, declaration.parameters, body); | ||
case SyntaxKind.Constructor: | ||
return createConstructor(declaration.decorators, declaration.modifiers, declaration.parameters, body); | ||
case SyntaxKind.FunctionExpression: | ||
return createFunctionExpression(declaration.modifiers, declaration.asteriskToken, declaration.name, declaration.typeParameters, declaration.parameters, declaration.type, body); | ||
case SyntaxKind.ArrowFunction: | ||
return createArrowFunction(declaration.modifiers, declaration.typeParameters, declaration.parameters, declaration.type, declaration.equalsGreaterThanToken, body); | ||
} | ||
} | ||
|
||
function getFixInfo(checker: TypeChecker, declaration: FunctionLikeDeclaration, expectType: Type, isFunctionType: boolean): Info | undefined { | ||
if (!declaration.body || !isBlock(declaration.body) || length(declaration.body.statements) !== 1) return undefined; | ||
|
||
const firstStatement = first(declaration.body.statements); | ||
if (isExpressionStatement(firstStatement) && checkFixedAssignableTo(checker, declaration, firstStatement.expression, expectType, isFunctionType)) { | ||
return { | ||
declaration, | ||
kind: FixKind.MissingReturnStatement, | ||
expression: firstStatement.expression | ||
}; | ||
} | ||
else if (isLabeledStatement(firstStatement) && isExpressionStatement(firstStatement.statement)) { | ||
const node = createObjectLiteral([createPropertyAssignment(firstStatement.label, firstStatement.statement.expression)]); | ||
if (checkFixedAssignableTo(checker, declaration, node, expectType, isFunctionType)) { | ||
return isArrowFunction(declaration) ? { | ||
declaration, | ||
kind: FixKind.MissingParentheses, | ||
expression: node | ||
} : { | ||
declaration, | ||
kind: FixKind.MissingReturnStatement, | ||
expression: node | ||
}; | ||
} | ||
} | ||
else if (isBlock(firstStatement) && length(firstStatement.statements) === 1) { | ||
const firstBlockStatement = first(firstStatement.statements); | ||
if (isLabeledStatement(firstBlockStatement) && isExpressionStatement(firstBlockStatement.statement)) { | ||
const node = createObjectLiteral([createPropertyAssignment(firstBlockStatement.label, firstBlockStatement.statement.expression)]); | ||
if (checkFixedAssignableTo(checker, declaration, node, expectType, isFunctionType)) { | ||
return { | ||
declaration, | ||
kind: FixKind.MissingReturnStatement, | ||
expression: node | ||
}; | ||
} | ||
} | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
function checkFixedAssignableTo(checker: TypeChecker, declaration: FunctionLikeDeclaration, expr: Expression, type: Type, isFunctionType: boolean) { | ||
return checker.isTypeAssignableTo(checker.getTypeAtLocation(isFunctionType ? updateFunctionLikeBody(declaration, createBlock([createReturn(expr)])) : expr), type); | ||
} | ||
|
||
function getInfo(checker: TypeChecker, sourceFile: SourceFile, position: number, errorCode: number): Info | undefined { | ||
const node = getTokenAtPosition(sourceFile, position); | ||
if (!node.parent) return undefined; | ||
|
||
const declaration = findAncestor(node.parent, isFunctionLikeDeclaration); | ||
switch (errorCode) { | ||
case Diagnostics.A_function_whose_declared_type_is_neither_void_nor_any_must_return_a_value.code: | ||
if (!declaration || !declaration.body || !declaration.type || !rangeContainsRange(declaration.type, node)) return undefined; | ||
return getFixInfo(checker, declaration, checker.getTypeFromTypeNode(declaration.type), /* isFunctionType */ false); | ||
case Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1.code: | ||
if (!declaration || !isCallExpression(declaration.parent) || !declaration.body) return undefined; | ||
const pos = declaration.parent.arguments.indexOf(<Expression>declaration); | ||
const type = checker.getContextualTypeForArgumentAtIndex(declaration.parent, pos); | ||
if (!type) return undefined; | ||
return getFixInfo(checker, declaration, type, /* isFunctionType */ true); | ||
case Diagnostics.Type_0_is_not_assignable_to_type_1.code: | ||
if (!isDeclarationName(node) || !isVariableLike(node.parent) && !isJsxAttribute(node.parent)) return undefined; | ||
const initializer = getVariableLikeInitializer(node.parent); | ||
if (!initializer || !isFunctionLikeDeclaration(initializer) || !initializer.body) return undefined; | ||
return getFixInfo(checker, initializer, checker.getTypeAtLocation(node.parent), /* isFunctionType */ true); | ||
} | ||
return undefined; | ||
} | ||
|
||
function getVariableLikeInitializer(declaration: VariableLikeDeclaration): Expression | undefined { | ||
switch (declaration.kind) { | ||
case SyntaxKind.VariableDeclaration: | ||
case SyntaxKind.Parameter: | ||
case SyntaxKind.BindingElement: | ||
case SyntaxKind.PropertyDeclaration: | ||
case SyntaxKind.PropertyAssignment: | ||
return declaration.initializer; | ||
case SyntaxKind.JsxAttribute: | ||
return declaration.initializer && (isJsxExpression(declaration.initializer) ? declaration.initializer.expression : undefined); | ||
case SyntaxKind.ShorthandPropertyAssignment: | ||
case SyntaxKind.PropertySignature: | ||
case SyntaxKind.EnumMember: | ||
case SyntaxKind.JSDocPropertyTag: | ||
case SyntaxKind.JSDocParameterTag: | ||
return undefined; | ||
} | ||
} | ||
|
||
function addReturnStatement(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: FunctionLikeDeclaration, expression: Expression) { | ||
changes.replaceNode(sourceFile, declaration.body!, createBlock([createReturn(expression)])); | ||
} | ||
|
||
function removeBlockBodyBrace(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: ArrowFunction, expression: Expression, withParen: boolean) { | ||
changes.replaceNode(sourceFile, declaration.body, (withParen || needsParentheses(expression)) ? createParen(expression) : expression); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this going to delete comments? I'd be worried about anything between the end of the expression and the closing brace (and maybe things before the opening brace). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it might delete |
||
} | ||
|
||
function wrapBlockWithParen(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: ArrowFunction, expression: Expression) { | ||
changes.replaceNode(sourceFile, declaration.body, createParen(expression)); | ||
} | ||
|
||
function getActionForfixAddReturnStatement(context: CodeFixContext, declaration: FunctionLikeDeclaration, expression: Expression) { | ||
const changes = textChanges.ChangeTracker.with(context, t => addReturnStatement(t, context.sourceFile, declaration, expression)); | ||
return createCodeFixAction(fixId, changes, Diagnostics.Add_a_return_statement, fixIdAddReturnStatement, Diagnostics.Correct_all_return_expressions); | ||
} | ||
|
||
function getActionForfixRemoveBlockBodyBrace(context: CodeFixContext, declaration: ArrowFunction, expression: Expression) { | ||
const changes = textChanges.ChangeTracker.with(context, t => removeBlockBodyBrace(t, context.sourceFile, declaration, expression, /* withParen */ false)); | ||
return createCodeFixAction(fixId, changes, Diagnostics.Remove_block_body_braces, fixIdRemoveBlockBodyBrace, Diagnostics.Correct_all_return_expressions); | ||
} | ||
|
||
function getActionForfixReplaceBraceWithParen(context: CodeFixContext, declaration: ArrowFunction, expression: Expression) { | ||
const changes = textChanges.ChangeTracker.with(context, t => removeBlockBodyBrace(t, context.sourceFile, declaration, expression, /* withParen */ true)); | ||
return createCodeFixAction(fixId, changes, Diagnostics.Replace_braces_with_parentheses, fixIdReplaceBraceWithParen, Diagnostics.Correct_all_return_expressions); | ||
} | ||
|
||
function getActionForfixWrapTheBlockWithParen(context: CodeFixContext, declaration: ArrowFunction, expression: Expression) { | ||
const changes = textChanges.ChangeTracker.with(context, t => wrapBlockWithParen(t, context.sourceFile, declaration, expression)); | ||
return createCodeFixAction(fixId, changes, Diagnostics.Wrap_this_block_with_parentheses, fixIdWrapTheBlockWithParen, Diagnostics.Correct_all_return_expressions); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/// <reference path='fourslash.ts' /> | ||
Kingwl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
//// function Foo (): number { | ||
//// 1 | ||
//// } | ||
|
||
verify.codeFixAvailable([ | ||
{ description: 'Add a return statement' }, | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// const a: ((() => number) | (() => undefined)) = () => { 1 } | ||
|
||
verify.codeFixAvailable([ | ||
{ description: 'Add a return statement' }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a statement has proven to be non-trivial in the past because people have strong feelings about semicolon usage. Personally, I think I'd only offer that fix if the expression to be returned were on its own line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think @andrewbranch did a bunch of work to make semicolons match the surrounding style automatically. 😄 |
||
{ description: 'Remove block body braces' }, | ||
{ description: 'Replace braces with parentheses' }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd find it helpful to see an example of where someone used braces when they meant to use parentheses. I'm not convinced it's common enough to warrant a code fix. |
||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
//// interface A { | ||
//// bar: string | ||
//// } | ||
//// | ||
//// function Foo (): A { | ||
//// { bar: '123' } | ||
//// } | ||
|
||
verify.codeFixAvailable([ | ||
{ description: 'Add a return statement' }, | ||
amcasey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ description: 'Remove unused label' }, | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user, I'm not sure how I'd expect this to behave. It seems more likely that I'd want to apply one of the fixes above in all places where it could be applied, rather than apply some fix anywhere the diagnostic is produced.