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

Quick fix for no-implicit-any errors to add explicit type annotation #14786

Merged
merged 45 commits into from
Oct 12, 2017

Conversation

mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Mar 22, 2017

Add a new quick fix for no-implicit-any errors for variables, parameters, and members. The fix tries to "infer" the type from assignments to the symbol in question, and serializes the type in an explicit type annotation addressing the no-implicit-any error.

@DanielRosenwasser DanielRosenwasser changed the title Quick fix for no-implicit-any errors to ad explicit type annotation Quick fix for no-implicit-any errors to add explicit type annotation Mar 22, 2017
@aluanhaddad
Copy link
Contributor

I am delighted by this. Thank you @mhegazy!

@@ -205,6 +205,24 @@ namespace ts {
return tryFindAmbientModule(moduleName, /*withAugmentations*/ false);
},
getApparentType,

getUnionType,
Copy link
Member

@DanielRosenwasser DanielRosenwasser May 9, 2017

Choose a reason for hiding this comment

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

Nit: Maybe makeUnionType or createUnionType? This works too though.

case Diagnostics.Variable_0_implicitly_has_type_1_in_some_locations_where_its_type_cannot_be_determined.code:
return getCodeActionForVariableDeclaration(<PropertyDeclaration | PropertySignature | VariableDeclaration>token.parent);
case Diagnostics.Variable_0_implicitly_has_an_1_type.code:
return getCodeActionForVariableUsage(<Identifier>token);
Copy link
Member

@DanielRosenwasser DanielRosenwasser May 9, 2017

Choose a reason for hiding this comment

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

I feel like there should be assertions when you cast based on the error code you're matching on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We asserted a few lines earlier that it has to be an identifier, DotDotDotToken, PublicKeyword, ProtectedKeyword, or ReadonlyKeyword. since this is a variable declaration error, we know it can not be anything that is not an identifier (i.e. binding pattern).

case Diagnostics.Variable_0_implicitly_has_an_1_type.code:
return getCodeActionForVariableUsage(<Identifier>token);

// Paramtert declarations
Copy link
Member

Choose a reason for hiding this comment

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

Parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return typeString;
}

function getParameterIndexInList(parameter: ParameterDeclaration, list: NodeArray<ParameterDeclaration>) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just indexOf?

for (const child of node.getChildren(sourcefile)) {
if (child.kind === kind) return child;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

explicit return undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

return -1;
}

function getFirstChildOfKind(node: Node, sourcefile: SourceFile, kind: SyntaxKind) {
Copy link
Member

Choose a reason for hiding this comment

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

There's a findChildOfKind which is nice and slow and allocates the lamdba you were trying to avoid here.


// Property declarations
case Diagnostics.Member_0_implicitly_has_an_1_type.code:
return getCodeActionForVariableDeclaration(<VariableDeclaration>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.

Is there ever a time that you won't have a symbol that you can't call this directly? What about in the case of a computed property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We filter these earlier on. it can only be identifier for a var declarations

Conflicts:
	src/compiler/checker.ts
	src/compiler/diagnosticMessages.json
	src/compiler/types.ts
	src/services/tsconfig.json
getUndefinedType: () => undefinedType,
getNullType: () => nullType,
getESSymbolType: () => esSymbolType,
getNeverType: () => neverType,
Copy link
Member

Choose a reason for hiding this comment

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

As a concern, we have multiple never types internally. I feel somewhat uneasy about exposing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made them all them internal for now.

@DovydasNavickas
Copy link

Add new lines to the end of files?

@mhegazy
Copy link
Contributor Author

mhegazy commented Oct 7, 2017

@DanielRosenwasser any more comments?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

A few questions about inference, plus I noticed some typos.

case Diagnostics.Property_0_implicitly_has_type_any_because_its_set_accessor_lacks_a_parameter_type_annotation.code:
return isSetAccessor(containingFunction) ? getCodeActionForSetAccessor(containingFunction) : undefined;

// Property declarations
Copy link
Member

Choose a reason for hiding this comment

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

This looks redundant with the very first case of this switch.

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

errorCodes: [
// Variable declarations
Diagnostics.Variable_0_implicitly_has_type_1_in_some_locations_where_its_type_cannot_be_determined.code,
Diagnostics.Variable_0_implicitly_has_an_1_type.code,
Copy link
Member

Choose a reason for hiding this comment

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

this error maps to getCodeActionForVariableUsage, so I think it is actually should be commented with // variable uses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure :)

return undefined;
}

const types = inferTypeForParametersFromUsage(containingFunction) ||
Copy link
Member

Choose a reason for hiding this comment

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

so it's either from usage from calls or from intra-function usages, but not both? Why have inferences from call sites win?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do the function body first we are guranteed to manifacture something that the makes the funciton happy, but it usually will be too general. giving usage precedence allows us to get more specific types.. e.g.:

function length(a) { return a.length; }

length([1,2,3]);

inferring from function body will give a a type { length: any}. which is accurate, but rarely what the user would write manually.

inferring from the call site gives us number[] which is more like it..

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you should infer <T extends { length }>(a: T) => ... instead. In combination with return type inference, this should yield <T extends { length }>(a: T) => T["length"], which will work correctly when applied to arrays (or indeed anything with a length property).

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 tried that.. the types get unwieldy very fast.. and you have this structural type that has length, push, splice, with an element that is has kind, instead of just Node[]. the call sites provided the more user-identifiable name.. though the first one is more correct.

const types = inferTypeForParametersFromUsage(containingFunction) ||
map(containingFunction.parameters, p => isIdentifier(p.name) && inferTypeForVariableFromUsage(p.name));

if (types) {
Copy link
Member

Choose a reason for hiding this comment

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

it would be easier to read both nested blocks as early returns (if (!types) return undefined and if (!textChanges) return undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


if (types) {
const textChanges = reduceLeft(containingFunction.parameters, (memo, parameter, index) => {
if (types[index] && !parameter.type && !parameter.initializer) {
Copy link
Member

Choose a reason for hiding this comment

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

containingFunction.parameters.length === types.length, right?
If so, I don't think we need the efficiency of reduce and lazy initialisation of memo. It would be easier to read as a zip followed by filtering out undefined:
zipWith(containingFunction.parameters, types, (param, type) => ...).filter(change => change !== undefined)

(zipWith is in core.ts so it might need to be moved into the public API?)

}

if (usageContext.callContexts) {
for (const callConext of usageContext.callContexts) {
Copy link
Member

Choose a reason for hiding this comment

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

typo:callContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return checker.createSignature(/*declaration*/ undefined, /*typeParameters*/ undefined, /*thisParameter*/ undefined, parameters, returnType, /*typePredicate*/ undefined, callContext.argumentTypes.length, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false);
}

function inferTypeFromContext(node: Expression, checker: TypeChecker, usageContext: UsageContext): void {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this before getTypeFromUsageContext since it's used before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, moved get* after infer*

return;
}
else {
const indextType = checker.getTypeAtLocation(parent);
Copy link
Member

Choose a reason for hiding this comment

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

probably is a typo for indexType ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

function getSignatureFromCallContext(callContext: CallContext, checker: TypeChecker): Signature {
const parameters: Symbol[] = [];
for (let i = 0; i < callContext.argumentTypes.length; i++) {
const symbol = checker.createSymbol(SymbolFlags.FunctionScopedVariable, escapeLeadingUnderscores(`p${i}`));
Copy link
Member

Choose a reason for hiding this comment

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

I would call this arg${i} to match our other names for unnamed parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

else if (usageContext.properties && hasCallContext(usageContext.properties.get("push" as __String))) {
return checker.createArrayType(getParameterTypeFromCallContexts(0, usageContext.properties.get("push" as __String).callContexts, /*isRestParameter*/ false, checker));
}
else if (usageContext.properties || usageContext.callContexts || usageContext.constructContexts || usageContext.numberIndexContext || usageContext.stringIndexContext) {
Copy link
Member

Choose a reason for hiding this comment

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

what happens if you combine object types made up of the synthetic properties (et al) with the candidate types? Are the candidate types strictly better than the synthetic object type?

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 would think so.. a user-defined type seems more like what you would put on a function, e.g. Node or Symbol, and not {kind: SyntaxKind} or {flags: SymbolFlags} to use an example of our code base.

@mhegazy mhegazy merged commit 4487917 into master Oct 12, 2017
@mhegazy mhegazy deleted the inferFromUsage branch October 12, 2017 17:15
@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants