Infer from usage by similarity to builtin types #33263

sandersn · 2019-09-05T16:06:26Z

I think this PR is ready for review now.

This PR:

creates a list of "important" types. Right now: string, number, Array and Promise.
checks whether the members of an inferred type are assignable to any of the members of those types.
if 1 or 2 types match, these are added to the set of inferred types.

It also infers type parameters for Array and Promise, using an algorithm that is disturbingly close to the real inference algorithm. On the whole, infer-from-usage is like a shadow checker with subtly different rules. I don't really like this, but it may be inherent in the way infer-from-usage is designed. Thoughts?

Notes:

This needs a lot more tests. Edit: This is done; at least to a bare minimum.
There are three drive-by fixes:
a. In + inference, T + any no longer infers T = string | number.
b. inference type "unification" (it is more like aggregation and selection) no longer infers literal types and now uses subtype reduction when forming unions.
c. Return type inference no longer defaults to void; being in an expression statement infers void instead.

I'm not sure (2a) is correct; #30093 tracks problems with + inference and has some examples I need to try.

Follow up work:

Better inference: I want to look at particular failures from the user test suite for ideas. Already I noticed that concat doesn't work because it has two overloads.
Inference from named types: I plan to scrape source files and the global symbol table for symbols with SymbolFlags.Class, then add them to the list of types that can be inferred.
Caching: inferFromUsage should be able to cache its results. I'm not doing it here because cache invalidation is one of two hard things in CS.

None of these are particularly required but (2) should help a lot and not be too hard.

Basically, drop "Context" from all names, because it just indicates that it's an implementation of the State monad.

1. Everything explodes! Out of stack space! 2. Results aren't used yet. 3. But call and construct use the new getSignatureFromCalls, so I expect some baseline changes after I get the infinite recursion fixed.

Type parameter inference is special-cased, just moved from its previous place with no improvement.

It's a smeary copy of the checker's type parameter, so I feel bad about duplicating that code. Not sure what the solution is, architecturally.

sandersn · 2019-09-05T16:07:15Z

src/services/codefixes/inferFromUsage.ts

        }

        interface Usage {
-            isNumber?: boolean;
-            isString?: boolean;
+            isNumber: boolean | undefined;


cleanup: all Usage properties are now required and Usages are created with createEmptyUsage.

sandersn · 2019-09-05T16:07:46Z

src/services/codefixes/inferFromUsage.ts

@@ -626,6 +640,9 @@ namespace ts.codefix {
                    else if (otherOperandType.flags & TypeFlags.StringLike) {
                        usage.isString = true;
                    }
+                    else if (otherOperandType.flags & TypeFlags.Any) {


driveby fix (2a)

sandersn · 2019-09-05T16:07:56Z

src/services/codefixes/inferFromUsage.ts

@@ -772,7 +790,7 @@ namespace ts.codefix {
                good = good.filter(i => !(checker.getObjectFlags(i) & ObjectFlags.Anonymous));
                good.push(unifyAnonymousTypes(anons));
            }
-            return checker.getWidenedType(checker.getUnionType(good));
+            return checker.getWidenedType(checker.getUnionType(good.map(checker.getBaseTypeOfLiteralType), UnionReduction.Subtype));


driveby fix (2b)

sandersn · 2019-09-05T16:08:36Z

src/services/codefixes/inferFromUsage.ts

@@ -875,45 +885,183 @@ namespace ts.codefix {

                types.push(checker.createAnonymousType(/*symbol*/ undefined!, members, callSignatures, constructSignatures, stringIndexInfo, /*numberIndexInfo*/ undefined)); // TODO: GH#18217
            }
-            return types;
+            return types; // TODO: Should cache this since I HOPE it doesn't change


like normal checking, caching should be fine here. I'll do that in a followup PR.

This makes inferences a lot better.

void is explicitly inferred now, never used as a fallback.

sandersn · 2019-09-06T18:35:04Z

src/services/codefixes/inferFromUsage.ts

@@ -490,6 +551,9 @@ namespace ts.codefix {
            }

            switch (node.parent.kind) {
+                case SyntaxKind.ExpressionStatement:
+                    addCandidateType(usage, checker.getVoidType());


driveby fix (2c)

sandersn · 2019-09-06T21:01:56Z

src/services/codefixes/inferFromUsage.ts


-            if (usage.properties && hasCalls(usage.properties.get("then" as __String))) {
-                const paramType = getParameterTypeFromCalls(0, usage.properties.get("then" as __String)!.calls!, /*isRestParameter*/ false)!; // TODO: GH#18217
-                const types = paramType.getCallSignatures().map(sig => sig.getReturnType());


this was wrong--for the expression numberPromise.then(n => n.toString()), it would infer Promise<string> not Promise<number> for numberPromise.

sandersn · 2019-09-06T22:20:41Z

@andrewbranch @orta @weswigham I think this is ready for review now. Not sure who is most interested in this.

weswigham · 2019-09-10T00:02:00Z

src/compiler/checker.ts

@@ -524,6 +524,9 @@ namespace ts {
            },
            getApparentType,
            getUnionType,
+            isTypeAssignableTo: (source, target) => {


Even @internal, some people are going to be ecstatic that this finally found its way into our API surface and will therefore finally be callable by people using the API (even if via cast).

Uhhhh should I be worried about exposing too much? I feeling like assignability is a State Secret or something.

I'm really interested in the reason why it should be marked as @internal. Don't get me wrong, I'm one of those people ecstatic that this found it's way to the API, now I can ditch my custom typescript version from my library which exposed this. I'm just super curious about the reasoning behind this decision :)

Effectively this function now has to cross conceptual boundaries in TypeScript. Going from being useful internally only inside checker to being used inside the TSServices, which meant it needed to be exposed as a public API on the checker internally within our codebase,

Ok, but why the @internal annotation? It looks like the method is still kinda private?

I'm not 100% convinced that arbitrary uses of this function are safe and won't corrupt compiler state. Marking it @internal allows me to use it inside Typescript where I know how it's being used. Using it outside is an any cast away, but that way it's obvious that it's not technically supported.

orta · 2019-09-10T18:53:31Z

src/services/codefixes/inferFromUsage.ts

@@ -680,7 +747,7 @@ namespace ts.codefix {
                }
            }

-            calculateUsageOfNode(parent, call.returnType);
+            calculateUsageOfNode(parent, call.return_);


What is a return_?

return_: Usage, which is the information we inferred about a signature's return type from calls.

The old name, returnType, implies that it's a type already, which it's not; that comes from inferring a usage and unifying the resulting list of types.

Unifying is a bad name too, it should probably be reconcile or aggregate.

orta · 2019-09-10T18:53:50Z

src/services/codefixes/inferFromUsage.ts

@@ -694,7 +761,7 @@ namespace ts.codefix {
            if (!usage.properties) {
                usage.properties = createUnderscoreEscapedMap<Usage>();
            }
-            const propertyUsage = usage.properties.get(name) || { };
+            const propertyUsage = usage.properties.get(name) || createEmptyUsage();


Yeah, this feels nicer to me 👍

orta · 2019-09-10T18:57:04Z

src/services/codefixes/inferFromUsage.ts

-                return unifyFromUsage(inferFromUsage(innerUsage));
+        /**
+         * inference is limited to
+         * 1. generic types with a single parameter


I'm impressed we can do this 👍 - I assume it's use by the promise and array implementations

Yep, plus I intend to expand this to other types later.

sandersn · 2019-09-12T18:20:18Z

Tests were failing because I forgot to update a couple of baselines with the new inference code, which is unable to infer from arguments Array.concat because it's overloaded. So it always suggests any[], not string[], etc.

andrewbranch · 2019-09-16T21:47:44Z

src/services/codefixes/inferFromUsage.ts

-                    }
+                if (propUsage.calls) {
+                    const sigs = checker.getSignaturesOfType(source, SignatureKind.Call);
+                    result = result && !!sigs.length && checker.isTypeAssignableTo(source, getFunctionFromCalls(propUsage.calls));


Here’s an interesting test case:

function test(a) { a.toString(2) }

Expected result: a is number
Actual result: a is { toString: (arg0: number) => void; }

It happens because every builtin’s toString call signature is assignable to the inferred structural one, and then the inference to builtins bails because of the limit of 3 union constituents. I briefly thought source and target might be backwards here—that you actually want to see if the usage is assignable to the builtin’s property—but that’s too restrictive; the parameter type of arg0: number isn’t assignable to radix?: number under strictFunctionTypes because of the optionality of the latter, and the inferred return type void is not assignable to string. (Interestingly, if you swap target and source, turn off strictFunctionTypes, and change the line to let x = a.toString(2), the inference to number occurs.)

I was able to hack up a fix for this something like:

const sigs = checker.getSignaturesOfType(source, SignatureKind.Call); const usageType = getFunctionFromCalls(propUsage.calls); const maxSourceParams = sigs.reduce((max, sig) => Math.max(max, sig.parameters.length), 0); const maxUsageParams = usageType.getCallSignatures().reduce((max, sig) => Math.max(max, sig.parameters.length), 0); result = result && !!sigs.length && maxUsageParams <= maxSourceParams && checker.isTypeAssignableTo(source, usageType);

but admittedly it feels kind of bad that checking assignability doesn’t Just Work™. Maybe I’m missing something clever?

Errr, could we get better results by checking subtype first, then assignability, like we do for overload resolution?

I think the root problem is that we expect to infer signatures whose lengths match, and have that beat the signatures whose lengths don't, right? I think an rule in inferFromUsage would be better than trying to get subtype or assignability to do the right thing, since they have other concerns to worry about -- they are part of the Real World Checker that has to be correct, but I feel like this preference is part of the Dream World Inference we have going on here, where you choose the best candidate on what feels right a lot of the time.

This would require maintaining some additional information -- specifically a list of source signatures matched with usage-signatures, and then that matched list would be used to rank the built-in types instead of just using filter. Basically, combineNamedTypes, by analogy with combineTypes. Maybe the decision could be even deferred to combineTypes.

Let's take this PR as-is and I'll think about it for the future.

Oh, I mean, I just suggested subtype before assignability because i thought the difference in rules would already do the right thing (tm).

src/services/codefixes/inferFromUsage.ts

sandersn added 10 commits August 26, 2019 11:17

Improve names in infer-from-usage

3be6e75

Basically, drop "Context" from all names, because it just indicates that it's an implementation of the State monad.

Merge branch 'master' into infer-from-usage/similarity-to-builtins

2a8ee1f

Merge branch 'master' into infer-from-usage/similarity-to-builtins

0f215fd

Copied from old branch

d347b08

1. Everything explodes! Out of stack space! 2. Results aren't used yet. 3. But call and construct use the new getSignatureFromCalls, so I expect some baseline changes after I get the infinite recursion fixed.

Merge branch 'master' into infer-from-usage/similarity-to-builtins

c93f919

Fix bugs in combineUsages/getSignatureFromCalls

945d423

Turn on findBuiltinTypes

37150d9

Type parameter inference is special-cased, just moved from its previous place with no improvement.

Add type parameter inference

383286f

It's a smeary copy of the checker's type parameter, so I feel bad about duplicating that code. Not sure what the solution is, architecturally.

Merge branch 'master' into infer-from-usage/similarity-to-builtins

17d1a7e

Merge branch 'master' into infer-from-usage/similarity-to-builtins

ff38a1b

sandersn commented Sep 5, 2019

View reviewed changes

sandersn requested a review from andrewbranch September 5, 2019 16:09

sandersn added 4 commits September 5, 2019 16:16

Infer void from expr statement usage, not calls

052a3d9

This makes inferences a lot better.

Fallback type is always any now

d32c6b2

void is explicitly inferred now, never used as a fallback.

Tonnes of cleanup

f394190

Renames and more cleanup

1703ae0

sandersn commented Sep 6, 2019

View reviewed changes

Add test + reshuffle/rename new code

330e51f

weswigham reviewed Sep 10, 2019

View reviewed changes

orta reviewed Sep 12, 2019

View reviewed changes

orta added the Update Docs on Next Release Indicates that this PR affects docs label Sep 12, 2019

sandersn added 2 commits September 12, 2019 10:50

Merge branch 'master' into infer-from-usage/similarity-to-builtins

b69f5af

Update baselines with any[] inferences

3c79225

Even more renaming

de7d68a

andrewbranch reviewed Sep 16, 2019

View reviewed changes

use forEachEntry

84e857b

andrewbranch approved these changes Sep 23, 2019

View reviewed changes

sandersn merged commit f110480 into master Sep 24, 2019

sandersn deleted the infer-from-usage/similarity-to-builtins branch September 24, 2019 14:44

Infer from usage by similarity to builtin types #33263

Infer from usage by similarity to builtin types #33263

Uh oh!

Conversation

sandersn commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Sep 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sandersn commented Sep 5, 2019 •

edited

Loading

sandersn Sep 5, 2019 •

edited

Loading

sandersn Sep 6, 2019 •

edited

Loading