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

mhegazy · 2017-03-22T05:29:35Z

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.

aluanhaddad · 2017-03-31T23:08:49Z

I am delighted by this. Thank you @mhegazy!

DanielRosenwasser · 2017-05-09T05:07:05Z

src/compiler/checker.ts

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


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

DanielRosenwasser · 2017-05-09T05:09:58Z

src/services/codefixes/inferFromUsage.ts

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


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

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

DanielRosenwasser · 2017-05-09T05:10:15Z

src/services/codefixes/inferFromUsage.ts

+            case Diagnostics.Variable_0_implicitly_has_an_1_type.code:
+                return getCodeActionForVariableUsage(<Identifier>token);
+
+            // Paramtert declarations


DanielRosenwasser · 2017-05-09T05:15:46Z

src/services/codefixes/inferFromUsage.ts

+            return typeString;
+        }
+
+        function getParameterIndexInList(parameter: ParameterDeclaration, list: NodeArray<ParameterDeclaration>) {


Isn't this just indexOf?

DanielRosenwasser · 2017-05-09T05:16:39Z

src/services/codefixes/inferFromUsage.ts

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


explicit return undefined

DanielRosenwasser · 2017-05-09T05:19:24Z

src/services/codefixes/inferFromUsage.ts

+            return -1;
+        }
+
+        function getFirstChildOfKind(node: Node, sourcefile: SourceFile, kind: SyntaxKind) {


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

DanielRosenwasser · 2017-05-09T05:22:11Z

src/services/codefixes/inferFromUsage.ts

+
+            // Property declarations
+            case Diagnostics.Member_0_implicitly_has_an_1_type.code:
+                return getCodeActionForVariableDeclaration(<VariableDeclaration>token.parent);


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?

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

DanielRosenwasser · 2017-08-01T21:19:38Z

src/compiler/checker.ts

+            getUndefinedType: () => undefinedType,
+            getNullType: () => nullType,
+            getESSymbolType: () => esSymbolType,
+            getNeverType: () => neverType,


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

made them all them internal for now.

DovydasNavickas · 2017-09-06T10:01:06Z

Add new lines to the end of files?

… in the fix itself.

…a time

mhegazy · 2017-10-07T05:26:15Z

@DanielRosenwasser any more comments?

sandersn

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

sandersn · 2017-10-10T21:14:15Z

src/services/codefixes/inferFromUsage.ts

+            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


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

thanks. removed.

sandersn · 2017-10-10T21:21:37Z

src/services/codefixes/inferFromUsage.ts

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


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

sandersn · 2017-10-10T21:24:47Z

src/services/codefixes/inferFromUsage.ts

+                return undefined;
+            }
+
+            const types = inferTypeForParametersFromUsage(containingFunction) ||


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

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

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

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.

sandersn · 2017-10-10T21:44:59Z

src/services/codefixes/inferFromUsage.ts

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


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

sandersn · 2017-10-10T21:50:36Z

src/services/codefixes/inferFromUsage.ts

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


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

sandersn · 2017-10-10T22:11:36Z

src/services/codefixes/inferFromUsage.ts

+                }
+
+                if (usageContext.callContexts) {
+                    for (const callConext of usageContext.callContexts) {


typo:callContext

sandersn · 2017-10-10T22:16:10Z

src/services/codefixes/inferFromUsage.ts

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


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

Ok, moved get* after infer*

sandersn · 2017-10-10T22:39:12Z

src/services/codefixes/inferFromUsage.ts

+                return;
+            }
+            else {
+                const indextType = checker.getTypeAtLocation(parent);


probably is a typo for indexType ?

sandersn · 2017-10-10T22:43:24Z

src/services/codefixes/inferFromUsage.ts

+        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}`));


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

sandersn · 2017-10-10T22:45:17Z

src/services/codefixes/inferFromUsage.ts

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


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?

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 added 29 commits February 25, 2017 21:05

Infer from usage quick fix

a4ce5e6

Change full function singature

d79fbc0

Add property/element access support

69a4b8c

Fix a few issues

33467cf

Merge branch 'master' into inferFromUsage

ab39813

Some cleanup

d87eae9

Expose getArrayType and getPromiseType

a18a950

Switch to collecting all usage before infering

bea4591

Infer array and promise type arguments

55ffe2d

Handel enums in binary operators

b82dc8d

consolidate usage of addCandidateTypes

7a52545

Handel rest paramters

8088415

Properly handel += and + inference for numbers and strings

0413165

Add print quickfixes debug helper

ea483a7

Add rest param tests

ea344b2

Add optional paramter tests

a1efc1a

Handel set accessors

ff15dc6

Support getters

e2e43d1

Support no implicit any error for variable at use site

2929af4

Support properties

b395057

Only offer quick fix if an infered type other than any is available

15eb7de

Rename functions

2a9032b

Move to a separate namespace

057016b

Check cancellation token

32aa07d

Cleanup

5da31bb

Check for accesibile symbols where serializing types

dc431e4

Remove JS support

561cfe6

Reorganize functions

002e8ad

Mark APIs as internal

3822d8d

msftclas added the cla-not-required label Mar 22, 2017

Merge remote-tracking branch 'origin/master' into inferFromUsage

c3071a8

DanielRosenwasser changed the title ~~Quick fix for no-implicit-any errors to ad explicit type annotation~~ Mar 22, 2017

This was referenced Apr 17, 2017

Control flow analysis based on operators applied to an argument #15196

Closed

Add a refactoring that annotates an implicit or explicit any with a best guess type inferred from its use sites #13243

Open

Merge branch 'master' into inferFromUsage

a12dc45

DanielRosenwasser requested changes May 9, 2017

View reviewed changes

DanielRosenwasser added 2 commits August 1, 2017 14:14

Merge branch 'master' into inferFromUsage

791bb4a

Conflicts: src/compiler/checker.ts src/compiler/diagnosticMessages.json src/compiler/types.ts src/services/tsconfig.json

Removed conflict markers.

0047ae3

DanielRosenwasser reviewed Aug 1, 2017

View reviewed changes

DanielRosenwasser added 4 commits September 19, 2017 00:43

Update 'createSymbol' to use '__String'.

5a15e93

Fixed most problems relating to '__String' and 'includeJsDocComments'…

9daaffa

… in the fix itself.

Addressed most API changes.

b5e94a7

Merge remote-tracking branch 'origin/master' into inferFromUsage

3b49776

msftgits removed the cla-not-required label Sep 26, 2017

mhegazy added 6 commits October 5, 2017 16:17

Merge branch 'master' into inferFromUsage

3f488ce

Make all helpers internal

6bdaf8a

Use a diffrent writer and not the built-in single line write

25f8aa6

Infer types for all parameters in a parameter list instead of one at …

761bd7b

…a time

Accept baselines

ffa8272

Code review commments

5abd72d

sandersn requested changes Oct 10, 2017

View reviewed changes

Respond to code review comments

1e39d48

sandersn approved these changes Oct 12, 2017

View reviewed changes

mhegazy merged commit 4487917 into master Oct 12, 2017

mhegazy deleted the inferFromUsage branch October 12, 2017 17:15

mhegazy mentioned this pull request Nov 8, 2017

Discussion: Parameter type inference from function body #17715

Closed

microsoft locked and limited conversation to collaborators Jun 21, 2018

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

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

Conversation

mhegazy commented Mar 22, 2017

aluanhaddad commented Mar 31, 2017

DanielRosenwasser May 9, 2017 • edited Loading

Choose a reason for hiding this comment

DanielRosenwasser May 9, 2017 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DovydasNavickas commented Sep 6, 2017

mhegazy commented Oct 7, 2017

sandersn left a 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser May 9, 2017 •

edited

Loading

DanielRosenwasser May 9, 2017 •

edited

Loading