Fixes #30507 #32100

timsuchanek · 2019-06-26T07:29:29Z

I saw, that when the object expression has incorrect properties, that don't exist on the type, the autocompletion works fine.
The decision, if the constraint type should be taken as the default or not is being made in checker.ts:15733 and as this is about 10 function calls deep when coming from https://github.com/microsoft/TypeScript/blob/master/src/services/completions.ts#L1457 I had to introduce a parameter in all these functions that can be passed down.
I saw, that there was already a ContextFlags type, which can be passed in to getContextualType, so I extended it and to make it accessible from the completions.ts, I had to add it to the TSChecker types in https://github.com/microsoft/TypeScript/compare/master...timsuchanek:genericObjectExpressionCompletion?expand=1#diff-4b8bd1eea29904f1be39cd864e1a45c0R3335

Let me know if I could have achieved this in a different way, but after many hours of debugging, this looked like the "most minimal" change to achieve the goal.

I added 2 tests, all tests are green.

msftclas · 2019-06-26T07:29:43Z

All CLA requirements met.

schickling · 2019-06-26T08:59:42Z

Very excited to see this fix! Thanks for your work on this @timsuchanek!

timsuchanek · 2019-06-27T15:58:09Z

Sorry to bother, but maybe @ahejlsberg you can have a look as you introduced the ContextFlags. Not sure if I'm "misusing" them.

AviVahl · 2019-07-10T19:23:57Z

Does it also fix #28470?

timsuchanek · 2019-07-11T16:36:24Z

@AviVahl from my understanding yes! I'll fix the new conflicts tomorrow, then it's again ready to get merged.

I'll also add your example as a test case.

timsuchanek · 2019-07-12T09:57:15Z

@AviVahl I added a fix so that your use-case with the constructor arguments is also supported and also added the 2 examples from #28470 as test cases.

@weswigham @ahejlsberg can you prioritize this PR so that it doesn't get conflicts with master again? Thanks 🙏

src/compiler/checker.ts

DanielRosenwasser · 2019-07-15T23:46:25Z

@timsuchanek would you also be able to add a test for string literal completions too?

declare function get<T, key: K extends keyof T>(obj: T, key: K): T[K];

get({ hello: 123, world: 456 }, "/**/");

This is one of the most-maddening and longest-standing issues we've had in completions. It would be really great to get this reviewed @ahejlsberg @weswigham @andrewbranch.

timsuchanek · 2019-07-16T13:29:22Z

@DanielRosenwasser I just simplified the "binary and" statement and also added your example as a test.

Important to note: Your example also worked before my PR, I added it to the tests nonetheless. I didn't touch the code path for string literals, just the code path for object expressions when coming from the completion service.

andrewbranch

Looks good to me, so long as others are happy with the public API changes. The failed builds are just API baseline issues after the latest merge with master.

src/compiler/checker.ts

timsuchanek · 2019-08-14T12:55:17Z

Thanks @andrewbranch! Is there a way that I can accept the baselines and add a commit for that?
Locally on my machine the tests succeed, can't reproduce the baseline failure.

weswigham · 2019-08-14T14:12:07Z

Pass --skipPercent=0, and, failing that, set the CI environment variable.

weswigham · 2019-08-14T14:12:48Z

(also rebase/merge with master)

andrewbranch · 2019-09-04T22:34:28Z

I fixed the baseline merge. @weswigham @ahejlsberg any reservations about exporting ContextFlags in the public API?

schickling · 2019-09-05T15:14:06Z

Thanks a lot @andrewbranch. Can’t wait to see this merged! (This is a big deal for us at @prisma.) 🎉

sheetalkamat · 2019-09-05T18:03:40Z

tests/baselines/reference/api/typescript.d.ts

@@ -2026,6 +2026,11 @@ declare namespace ts {
         */
        runWithCancellationToken<T>(token: CancellationToken, cb: (checker: TypeChecker) => T): T;
    }
+    export enum ContextFlags {


I think this as well as new signature need to be internal

Do you mean we should use something different than the ContextFlags when calling the code from the completion service to prevent this type being exposed?

I think Sheetal means to move ContextFlags into one of the ts namespaces that’s already marked /** @internal */ and then to mark the second parameter of TypeChecker['getContextualType'] as /* @internal */ (or declare it as an internal overload, but the linter might call that an unnecessary overload)

Ok, makes sense! The overload option indeed makes the linter complain:

ERROR: /Users/tim/code/TypeScript/src/compiler/types.ts:3224:61 unified-signatures These overloads can be combined into one signature with an optional parameter.

The non-overload option, which I wrote like this:

getContextualType( node: Expression, /* @internal */ contextFlags?: ContextFlags ): Type | undefined;

spits out an invalid typescript.d.ts:

getRootSymbols(symbol: Symbol): ReadonlyArray<Symbol>; getContextualType(node: Expression, : Type | undefined; /** * returns unknownSignature in the case of an error. * returns undefined if the node is not valid. * @param argumentCount Apparent number of arguments, passed in case of a possibly incomplete call. This should come from an ArgumentListInfo. See `signatureHelp.ts`. */

Is there a way to ignore the linter in this case?
Or what is the right way to add @internal to a specific argument?

Just filed #33350. I think it would be ok to ignore the line from the linter. @sheetalkamat?

Yes in past we have marked the overload internal and disabled the rule for that line.

Done. Had to adjust the tests because of #32266

Anything else you need from my side to get this merged?
@sheetalkamat @andrewbranch

Cool, this doesn’t even touch the public API baselines now. Would be good to get an approval from another person on the team but I think this is good.

weswigham · 2019-09-13T17:18:34Z

src/compiler/checker.ts

@@ -16201,7 +16196,8 @@ namespace ts {
                const constraint = getConstraintOfTypeParameter(inference.typeParameter);
                if (constraint) {
                    const instantiatedConstraint = instantiateType(constraint, context.nonFixingMapper);
-                    if (!inferredType || !context.compareTypes(inferredType, getTypeWithThisArgument(instantiatedConstraint, inferredType))) {
+                    const isCompletionContext = contextFlags && (contextFlags & ContextFlags.Completion);
+                    if (!inferredType || isCompletionContext || !context.compareTypes(inferredType, getTypeWithThisArgument(instantiatedConstraint, inferredType))) {


This doesn't seem like the right place to do this - rather than passing the flag all the way thru signature resolution, if this flag is set, we should just look at the "base signature" (getBaseSignature) rather than starting inference at all, I think. Then again, if we did that we wouldn't get an instantiated constraint which reports more detailed information on circular constraints. Then again again, I'm pretty sure as-is this is caching into the resolvedSignature of a node, which this very much is not - which means requesting completions poisons the cache with incorrect types for later checking.

Thanks for the feedback @weswigham! Do you have a suggestion how we still could fix the autocompletion for this? This was, unfortunately, the only way I found (with my limited knowledge of the codebase).

Maybe introducing a new method as an alternative to getContextualType just to be used by the completion service?

I think getContextualType is still right, we just need to avoid using any resolved signature caches.

orta · 2019-09-24T19:42:41Z

@typescript-bot pack this

typescript-bot · 2019-09-24T19:42:45Z

Heya @orta, I've started to run the tarball bundle task on this PR at 96a782e. You can monitor the build here. It should now contribute to this PR's status checks.

andrewbranch · 2019-10-10T23:06:31Z

Hey @timsuchanek, I took a stab at addressing @weswigham’s concerns with some guidance from @DanielRosenwasser. I didn’t want to push a more substantial change here so I branched off of this PR and pushed a new one up at #33937. Would greatly appreciate your input there. Thanks!

andrewbranch · 2019-10-17T17:47:55Z

@timsuchanek, thanks for championing this through! Your commits here have been merged into master via #33937. 🏆

timsuchanek · 2019-10-18T09:43:19Z

Thanks a lot, @andrewbranch for pushing this over the finish line!

timsuchanek mentioned this pull request Jun 26, 2019

Autocompletion broken for generic function parameter with optional properties #30507

Closed

weswigham requested a review from ahejlsberg June 27, 2019 20:35

timsuchanek force-pushed the genericObjectExpressionCompletion branch from 4aaccf6 to 0c38f21 Compare July 12, 2019 09:08

DanielRosenwasser assigned weswigham and ahejlsberg Jul 15, 2019

DanielRosenwasser reviewed Jul 15, 2019

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

DanielRosenwasser assigned andrewbranch Jul 15, 2019

andrewbranch mentioned this pull request Jul 16, 2019

react-router Make Route autocomplete its props DefinitelyTyped/DefinitelyTyped#36765

Closed

9 tasks

timsuchanek mentioned this pull request Jul 18, 2019

Improve type inference for types like 'T | Promise<T>' #32460

Merged

andrewbranch approved these changes Aug 2, 2019

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

timsuchanek force-pushed the genericObjectExpressionCompletion branch from d5667fa to ff6ceb5 Compare August 14, 2019 12:17

janpio mentioned this pull request Aug 26, 2019

Typescript compiler has a bug that needs to be fixed prisma/prisma#414

Closed

sheetalkamat reviewed Sep 5, 2019

View reviewed changes

timsuchanek force-pushed the genericObjectExpressionCompletion branch from a118dc5 to 85ee123 Compare September 11, 2019 08:36

timsuchanek added 3 commits September 11, 2019 10:47

fixes microsoft#30507

8e37e0a

Add test case for generic Partial type

5c561ea

Fixes microsoft#28470

d674ee0

timsuchanek and others added 6 commits September 11, 2019 10:47

Simplify contextFlags binary check

ad166f7

Add string literal completion test

50286b9

Fix ContextFlags typings

ba1e478

Speed up inference expression for completion

adf8a54

Fix baseline merge

25fa027

Make contextFlags internal

7883ec6

timsuchanek force-pushed the genericObjectExpressionCompletion branch from 85ee123 to 7883ec6 Compare September 11, 2019 08:48

timsuchanek added 3 commits September 11, 2019 10:51

Reapply readonly array changes

7e7dc08

accept baselines

c1a47a5

Fix generic completion tests

96a782e

weswigham requested changes Sep 13, 2019

View reviewed changes

orta and others added 3 commits September 25, 2019 08:13

Merge master

5ae31ff

Merge branch 'master' into genericObjectExpressionCompletion

b863e01

Re-merge ContextFlags

d1cb5cb

andrewbranch mentioned this pull request Oct 10, 2019

Fix completions of optional properties in generic positions #33937

Merged

andrewbranch closed this Oct 17, 2019

andrewbranch mentioned this pull request Dec 9, 2019

Improve some completions on generic object literals #34855

Merged

andrewbranch mentioned this pull request Dec 20, 2019

Properties in object literal key completion list aren't filtered when a prior argument supplies a mapped type key #35702

Closed

andrewbranch mentioned this pull request Jan 23, 2020

Don’t mix in base constraint completions on indexed access types with type parameter index types #36364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30507 #32100

Fixes #30507 #32100

timsuchanek commented Jun 26, 2019 •

edited

Loading

msftclas commented Jun 26, 2019 •

edited

Loading

schickling commented Jun 26, 2019

timsuchanek commented Jun 27, 2019

AviVahl commented Jul 10, 2019

timsuchanek commented Jul 11, 2019 •

edited

Loading

timsuchanek commented Jul 12, 2019 •

edited

Loading

DanielRosenwasser commented Jul 15, 2019 •

edited

Loading

timsuchanek commented Jul 16, 2019 •

edited

Loading

andrewbranch left a comment

timsuchanek commented Aug 14, 2019 •

edited

Loading

weswigham commented Aug 14, 2019

weswigham commented Aug 14, 2019

andrewbranch commented Sep 4, 2019

schickling commented Sep 5, 2019

sheetalkamat Sep 5, 2019

timsuchanek Sep 10, 2019 •

edited

Loading

andrewbranch Sep 10, 2019

timsuchanek Sep 10, 2019 •

edited

Loading

andrewbranch Sep 10, 2019

sheetalkamat Sep 10, 2019

timsuchanek Sep 11, 2019 •

edited

Loading

andrewbranch Sep 13, 2019

weswigham Sep 13, 2019

timsuchanek Sep 16, 2019

weswigham Sep 16, 2019 •

edited

Loading

orta commented Sep 24, 2019

typescript-bot commented Sep 24, 2019 •

edited

Loading

andrewbranch commented Oct 10, 2019

andrewbranch commented Oct 17, 2019

timsuchanek commented Oct 18, 2019

Fixes #30507 #32100

Fixes #30507 #32100

Conversation

timsuchanek commented Jun 26, 2019 • edited Loading

msftclas commented Jun 26, 2019 • edited Loading

schickling commented Jun 26, 2019

timsuchanek commented Jun 27, 2019

AviVahl commented Jul 10, 2019

timsuchanek commented Jul 11, 2019 • edited Loading

timsuchanek commented Jul 12, 2019 • edited Loading

DanielRosenwasser commented Jul 15, 2019 • edited Loading

timsuchanek commented Jul 16, 2019 • edited Loading

andrewbranch left a comment

Choose a reason for hiding this comment

timsuchanek commented Aug 14, 2019 • edited Loading

weswigham commented Aug 14, 2019

weswigham commented Aug 14, 2019

andrewbranch commented Sep 4, 2019

schickling commented Sep 5, 2019

sheetalkamat Sep 5, 2019

Choose a reason for hiding this comment

timsuchanek Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

andrewbranch Sep 10, 2019

Choose a reason for hiding this comment

timsuchanek Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

andrewbranch Sep 10, 2019

Choose a reason for hiding this comment

sheetalkamat Sep 10, 2019

Choose a reason for hiding this comment

timsuchanek Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

andrewbranch Sep 13, 2019

Choose a reason for hiding this comment

weswigham Sep 13, 2019

Choose a reason for hiding this comment

timsuchanek Sep 16, 2019

Choose a reason for hiding this comment

weswigham Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

orta commented Sep 24, 2019

typescript-bot commented Sep 24, 2019 • edited Loading

andrewbranch commented Oct 10, 2019

andrewbranch commented Oct 17, 2019

timsuchanek commented Oct 18, 2019

timsuchanek commented Jun 26, 2019 •

edited

Loading

msftclas commented Jun 26, 2019 •

edited

Loading

timsuchanek commented Jul 11, 2019 •

edited

Loading

timsuchanek commented Jul 12, 2019 •

edited

Loading

DanielRosenwasser commented Jul 15, 2019 •

edited

Loading

timsuchanek commented Jul 16, 2019 •

edited

Loading

timsuchanek commented Aug 14, 2019 •

edited

Loading

timsuchanek Sep 10, 2019 •

edited

Loading

timsuchanek Sep 10, 2019 •

edited

Loading

timsuchanek Sep 11, 2019 •

edited

Loading

weswigham Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 24, 2019 •

edited

Loading