Avoid excess property check for inference blocked source

[Andarist]

fixes #50818

cc @jakebailey @andrewbranch

[andrewbranch]

A flag set by a completions call should have no effect on errors. The checker state must be getting corrupted earlier; this is just silencing the problem when we need to fix it at the source instead. Unfortunately, I really don’t know where to start with this 😕

[Andarist]

This makes sense conceptually - especially this part:

A flag set by a completions call should have no effect on errors.

However, I debugged this quite a bit and this is roughly what happens:

1. completions request a contextual type for this particular argument (with inference blocked for it)
2. to obtain that contextual type we have to resolve the signature
3. since the inference is blocked - we simply get the constraint, so we get { foo: 1 } whereas if the inference wouldn't get blocked we'd get { foo: 1; bar: number; }
4. and since the "inferred" type has just a foo property we end up comparing our (fresh?) argument type against that and we end up reporting the excess property error (as it has more properties than just foo)

We need to resolve the signature to obtain potential completions from the contextual type and resolving the signature has some logic related to reporting errors etc.

I don't see how this could be caused by the "corrupted checker state". I actually started this investigation by looking for such a corrupted state but later on realized that it's perhaps not the reason behind this. Every step that I listed here is required for obtaining completions and, to the best of my understanding, this doesn't rely on any corrupted state - the problem is that we obtain T's constraint instead of the inferred type when the inference is blocked for it.

Alternatively, I could try to suppress getSignatureApplicabilityError but that seems even worse. I think it's better to return an OK result from chooseOverload here.

I might be missing some nuance here - but so far (and I've reexamined the situation again after your comment) I stand by the initial change :)

[andrewbranch]

Oh, so is the error issued during the call made from completions.ts?

@DanielRosenwasser have we figured out a general approach for these services-calls-messing-up-unified-checker issues?

[andrewbranch]

I feel like pretty much any checker API call besides getDiagnostics shouldn’t produce diagnostics, and maybe we need a general way of doing that.

[DanielRosenwasser]

A flag set by a completions call should have no effect on errors

The only thing I know of is that there are certain check-modes that are used to make sure that we don't contextually type certain arguments during inference, and there's something like that for signature help (which clearly isn't fixing the error-reporting issue).

@weswigham and @ahejlsberg might have a better idea on this one.

[Andarist]

Oh, so is the error issued during the call made from completions.ts?

Yes, this is the call stack:

[andrewbranch]

@DanielRosenwasser yeah, I would say we need something similar here, but we’re having so many of this brand of problem that I wonder if we should come up with a similar, but more general, mechanism and apply it across most/all of the API entrypoints, similar to how we wrap most of the entrypoints that take a Node with getParseNode.

Alternatively, I could try to suppress getSignatureApplicabilityError but that seems even worse. I think it's better to return an OK result from chooseOverload here.

@Andarist can you explain your thoughts behind this? If we detect we’re coming from a checker API call, we definitely want to avoid reporting (or doing the work to compute) errors. But is the assignability failure you fixed in your proposed change affecting the contextual type in an observable way?

[Andarist]

@Andarist can you explain your thoughts behind this? I

I'm not sure what I had exactly in mind right now. I've re-examined the situation once again and I don't think this was any good idea 😅

If we detect we’re coming from a checker API call, we definitely want to avoid reporting (or doing the work to compute) errors.

We still need to check for assignability etc - so we want to "error" within chooseOverload and stuff. We could suppress errors from getCandidateForOverloadFailure and the code after that (basically after chooseOverload fails to select anything) but then... we still need to report those errors at some point, so what would be the point of suppressing them then? They are legit errors after all.

OTOH, the blocked inference... can impact other arguments and we might "accidentally" report an error even though, at a given moment, there is no error. I was just able to do this with this code:

type Constraint = { type: "foo" } | { type: "bar"; extra?: string };

declare function test<T extends Constraint>(
  a: T,
  b: Constraint extends T ? never : 1
): void;

test({ type: "bar" }, 1);

When trying to add extra property to the first argument I got the error on the second one (like I anticipated):

Argument of type 'number' is not assignable to parameter of type 'never'.(2345)

This one is much harder to reproduce. I was only able to do it once and now I can't repro it anymore. I'm sure that I didn't actually change the source code anyhow - I was just editing the first argument of this function and somehow ended up with this situation.

I was also able to end up in the opposite situation. I ended up with this not displaying any error - even though the error should be there:

test({ type: "bar", extra: 1 }, 1); // extra has an incompatible type now, so the error should be reported here

But is the assignability failure you fixed in your proposed change affecting the contextual type in an observable way?

I wasn't able to observe an unwanted change anyhow. I think though there is a slight potential for a behavior change here in some convoluted situations with multiple overloads. If I understand correctly - the contextual type here is still sourced from a single overload so if we'd craft a situation in which the first overload would report this error but the second one wouldn't... then we would end up with the contextual type from the second one whereas after my change we could successfully match the first overload and thus we would end up with contextual type from the first overload. That would be an improvement though because the scenario without the inference blocked would actually select the first overload and not the second one in this scenario.

[andrewbranch]

we still need to report those errors at some point, so what would be the point of suppressing them then? They are legit errors after all.

At least one of us is not quite understanding the other and I’m not sure which of us it is 😅. The editor displays red squigglies by calling checker.getDiagnostics(sourceFile), which does basically what tsc would do, and obviously throughout this call we are producing and reporting errors. Nothing during this call will have the InferenceBlockedSource flag. But when you request completions (this can happen before or after requesting diagnostics AFAIK—it’s likely to be before, if it’s triggered by typing a trigger character), we need to call checker.getContextualType(node). This call is messing with the normal rules of inference in order to provide better completions on partially-typed code, so while it needs to do some stuff like overload resolution and assignability checking to come up with an answer, any errors generated here absolutely cannot be considered “legit errors.” Some may be, but those have been / will be reported by checker.getDiagnostics() in a separate request.

It seems like the trouble happens when this completions request happens before errors are requested, and something about the InferenceBlockedSource flag causes an error to be produced and cached that would not have been produced under normal checking. Then, diagnostics are requested, and it goes through and checks the whole file for errors, pushing new ones into the collection that already contains some from the completions request. Those are deduped before returning, so you don’t see doubles of any legit errors, but you do see the false positive error that was produced during completions.

That’s my understanding of it, anyway, without debugging it. I could be wrong. Does that fit what you’ve seen?

[Andarist]

Then, diagnostics are requested, and it goes through and checks the whole file for errors, pushing new ones into the collection that already contains some from the completions request.

This looks like an important bit to focus on here. I'm not saying that it should work like that - but I will go into what currently happens:

1. we request completions for a script+sourceFile of a given A version
2. this produces the excess property error
3. this error is added to the fileDiagnostics cache~
4. then we request diagnostics
5. we even try to synchronizeHostData before going into the Program APIs - but there is nothing to synchronize/restart. We still have the same script/sourceFile version A
6. so we proceed to step into this program - we might produce some new diagnostics or we might not produce any, but the excess property error is already in the cache. So we end up reporting it

Now, there might be some missing step here - something I didn't observe or something that should be called but isn't. Perhaps the said cache should be cleared before going into a fresh "type check".

However, according to the quote from your comment, it is expected that we might have some diagnostics produced by the completions request and that we might end up deduping the gathered errors etc. So the sole fact that the diagnostics were grabbed from the cache isn't the problem.

Perhaps this particular diagnostic should be somehow cleared. That would however be, IMHO, harder to achieve - it's easier to prevent than it is to ignore something after the fact. To ignore you need to preserve somewhere some contextual information as metadata of the said error - to decide later on if it should be ignored or not. And if it should be ignored - what's even the point of adding it to the cache in the first place?

On top of that, I still think that this particular error should not be added to the cache at all. It's a result of the getSignatureApplicabilityError call - and without inference blocked this check wouldn't fail. If it wouldn't fail then the given signature would be selected as a valid one. And if this signature is "rejected" with the inference blocked - that means that a different (latter) signature might get selected and this just doesn't match the "regular" typechecking call and might lead to weird results (the contextual type is coming primarily from the matched overload, right? Not from all the available ones)

So I recognize that "suppressing" this error might not be the whole story, but I still think that it should be suppressed (of course, I'm always happy to be proven wrong here, I'm not an expert on this codebase and my findings might be somehow off). The additional question is if the mechanism used to suppress it is a good one and if it's applied at the correct level etc - that is something that you have to judge for yourself.

[andrewbranch]

However, according to the quote from your comment, it is expected that we might have some diagnostics produced by the completions request and that we might end up deduping the gathered errors etc. So the sole fact that the diagnostics were grabbed from the cache isn't the problem.

No no, I was only trying to state what I think is happening, not what should happen. I think we’re on the same page about what’s happening. What I’m proposing we do is essentially clear errors at the end of non-diagnostics API calls like those used in completions, or equivalently make diagnostics.add() a no-op during those calls, or set some checker-scoped local variable that says “don’t produce errors!” that we can check and avoid even doing the work to compute errors. I think something like this would be valuable regardless of whether we need to suppress excess property checking in this particular case. And, if such a fix is sufficient to fix this particular bug, and completions work fine, and regular checking works fine, I would suggest not messing with excess property checking in completions-specific code paths. If it’s not sufficient, then we can continue to tweak the actual checking logic; however, I’m still wary of this, because fileDiagnostics is far from the only cache we need to worry about—we wouldn’t want relationship caches to contain results that were influenced by the completions-specific flag either, since that flag is not part of any cache key. Does that make sense?

[jakebailey]

What I’m proposing we do is essentially clear errors at the end of non-diagnostics API calls like those used in completions, or equivalently make diagnostics.add() a no-op during those calls, or set some checker-scoped local variable that says “don’t produce errors!” that we can check and avoid even doing the work to compute errors.

This sort of thing would have prevented the error #52373 from being produced, so I feel like we should do something like that (in this case, it was a lazyDiagnostic, which should also probably be silenced). Although, that's effectively just what it was when we had two type checkers.

That being said, it's also tricky because without the diagnostic, the actual underlying bug would have never been found (but, if nobody observes a bug, did it exist?).

[Andarist]

There is already a flag telling the compiler bits that this is a completion request - but that's in the contextFlags and those are not passed everywhere (+ it's only about completions, we might want to change the logic for more requests).

I quickly checked if this could be used to fix this case if I would pass it along. The slight problem is that resolveCall doesn't really have access to that, it has access to checkMode. I figured out that perhaps I don't want to make it aware of both of those so I tried mapping this context flag to the existing CheckMode.IsForStringLiteralArgumentCompletions. I know those are not equivalent - it was just an experiment.

It fixed the case at hand while regressing on one of the tests (completionsObjectLiteralWithPartialConstraint). That probably could be fixed by introducing a proper new CheckMode etc. However, this required a lot of plumbing and I only touched a single code path leading to resolveCall etc and there might be more of those.

diff with the context flag mapped to a context mode

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 2f943279c9..ae1e76f1d1 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -20358,7 +20358,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                 isSimpleTypeRelatedTo(source, target, relation, reportErrors ? reportError : undefined)) return Ternary.True;
 
             if (source.flags & TypeFlags.StructuredOrInstantiable || target.flags & TypeFlags.StructuredOrInstantiable) {
-                const isPerformingExcessPropertyChecks = !isFromInferenceBlockedSource(source) && !(intersectionState & IntersectionState.Target) && (isObjectLiteralType(source) && getObjectFlags(source) & ObjectFlags.FreshLiteral);
+                const isPerformingExcessPropertyChecks = !(intersectionState & IntersectionState.Target) && (isObjectLiteralType(source) && getObjectFlags(source) & ObjectFlags.FreshLiteral);
                 if (isPerformingExcessPropertyChecks) {
                     if (hasExcessProperties(source as FreshObjectLiteralType, target, reportErrors)) {
                         if (reportErrors) {
@@ -28521,13 +28521,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
     }
 
     // In a typed function call, an argument or substitution expression is contextually typed by the type of the corresponding parameter.
-    function getContextualTypeForArgument(callTarget: CallLikeExpression, arg: Expression): Type | undefined {
+    function getContextualTypeForArgument(callTarget: CallLikeExpression, arg: Expression, contextFlags?: ContextFlags): Type | undefined {
         const args = getEffectiveCallArguments(callTarget);
         const argIndex = args.indexOf(arg); // -1 for e.g. the expression of a CallExpression, or the tag of a TaggedTemplateExpression
-        return argIndex === -1 ? undefined : getContextualTypeForArgumentAtIndex(callTarget, argIndex);
+        return argIndex === -1 ? undefined : getContextualTypeForArgumentAtIndex(callTarget, argIndex, contextFlags);
     }
 
-    function getContextualTypeForArgumentAtIndex(callTarget: CallLikeExpression, argIndex: number): Type {
+    function getContextualTypeForArgumentAtIndex(callTarget: CallLikeExpression, argIndex: number, contextFlags?: ContextFlags): Type {
         if (isImportCall(callTarget)) {
             return argIndex === 0 ? stringType :
                 argIndex === 1 ? getGlobalImportCallOptionsType(/*reportErrors*/ false) :
@@ -28536,7 +28536,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
 
         // If we're already in the process of resolving the given signature, don't resolve again as
         // that could cause infinite recursion. Instead, return anySignature.
-        const signature = getNodeLinks(callTarget).resolvedSignature === resolvingSignature ? resolvingSignature : getResolvedSignature(callTarget);
+        const signature = getNodeLinks(callTarget).resolvedSignature === resolvingSignature ? resolvingSignature : getResolvedSignature(callTarget, /*candidatesArray*/ undefined, (contextFlags || 0) & ContextFlags.Completions ? CheckMode.IsForStringLiteralArgumentCompletions : CheckMode.Normal);
 
         if (isJsxOpeningLikeElement(callTarget) && argIndex === 0) {
             return getEffectiveFirstArgumentForJsxSignature(signature, callTarget);
@@ -29017,7 +29017,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                 return getContextualTypeForAwaitOperand(parent as AwaitExpression, contextFlags);
             case SyntaxKind.CallExpression:
             case SyntaxKind.NewExpression:
-                return getContextualTypeForArgument(parent as CallExpression | NewExpression | Decorator, node);
+                return getContextualTypeForArgument(parent as CallExpression | NewExpression | Decorator, node, contextFlags);
             case SyntaxKind.Decorator:
                 return getContextualTypeForDecorator(parent as Decorator);
             case SyntaxKind.TypeAssertionExpression:
@@ -32559,7 +32559,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
         const isTaggedTemplate = node.kind === SyntaxKind.TaggedTemplateExpression;
         const isDecorator = node.kind === SyntaxKind.Decorator;
         const isJsxOpeningOrSelfClosingElement = isJsxOpeningLikeElement(node);
-        const reportErrors = !candidatesOutArray;
+        const reportErrors = !candidatesOutArray && !(checkMode & CheckMode.IsForStringLiteralArgumentCompletions);
 
         let typeArguments: NodeArray<TypeNode> | undefined;
 

So later I tried muting the diagnostics.add and that fixes the problem without regressing any tests (quite expected outcome):

diff with `diagnostics.add` muted with `noop`

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 2f943279c9..421ddcda8c 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -1808,26 +1808,33 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
     };
 
     function runWithInferenceBlockedFromSourceNode<T>(node: Node | undefined, fn: () => T): T {
-        const containingCall = findAncestor(node, isCallLikeExpression);
-        const containingCallResolvedSignature = containingCall && getNodeLinks(containingCall).resolvedSignature;
-        if (containingCall) {
-            let toMarkSkip = node!;
-            do {
-                getNodeLinks(toMarkSkip).skipDirectInference = true;
-                toMarkSkip = toMarkSkip.parent;
-            } while (toMarkSkip && toMarkSkip !== containingCall);
-            getNodeLinks(containingCall).resolvedSignature = undefined;
-        }
-        const result = fn();
-        if (containingCall) {
-            let toMarkSkip = node!;
-            do {
-                getNodeLinks(toMarkSkip).skipDirectInference = undefined;
-                toMarkSkip = toMarkSkip.parent;
-            } while (toMarkSkip && toMarkSkip !== containingCall);
-            getNodeLinks(containingCall).resolvedSignature = containingCallResolvedSignature;
+        const originalDiagnosticsAdd = diagnostics.add;
+        diagnostics.add = () => {};
+        try {
+            const containingCall = findAncestor(node, isCallLikeExpression);
+            const containingCallResolvedSignature = containingCall && getNodeLinks(containingCall).resolvedSignature;
+            if (containingCall) {
+                let toMarkSkip = node!;
+                do {
+                    getNodeLinks(toMarkSkip).skipDirectInference = true;
+                    toMarkSkip = toMarkSkip.parent;
+                } while (toMarkSkip && toMarkSkip !== containingCall);
+                getNodeLinks(containingCall).resolvedSignature = undefined;
+            }
+            const result = fn();
+            if (containingCall) {
+                let toMarkSkip = node!;
+                do {
+                    getNodeLinks(toMarkSkip).skipDirectInference = undefined;
+                    toMarkSkip = toMarkSkip.parent;
+                } while (toMarkSkip && toMarkSkip !== containingCall);
+                getNodeLinks(containingCall).resolvedSignature = containingCallResolvedSignature;
+            }
+            return result;
+        }
+        finally {
+            diagnostics.add = originalDiagnosticsAdd;
         }
-        return result;
     }
 
     function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode, editingArgument?: Node): Signature | undefined {
@@ -20358,7 +20365,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                 isSimpleTypeRelatedTo(source, target, relation, reportErrors ? reportError : undefined)) return Ternary.True;
 
             if (source.flags & TypeFlags.StructuredOrInstantiable || target.flags & TypeFlags.StructuredOrInstantiable) {
-                const isPerformingExcessPropertyChecks = !isFromInferenceBlockedSource(source) && !(intersectionState & IntersectionState.Target) && (isObjectLiteralType(source) && getObjectFlags(source) & ObjectFlags.FreshLiteral);
+                const isPerformingExcessPropertyChecks = !(intersectionState & IntersectionState.Target) && (isObjectLiteralType(source) && getObjectFlags(source) & ObjectFlags.FreshLiteral);
                 if (isPerformingExcessPropertyChecks) {
                     if (hasExcessProperties(source as FreshObjectLiteralType, target, reportErrors)) {
                         if (reportErrors) {

I think that both of those are not ideal. The first one would require a lot of plumbing and the second one merely mutes the diagnostics.add but the work to compute them is still done.

So I actually like your third idea the best - I propose that I add a new "global" (to the checker) flag and "consult" it at all reportErrors checks. How does it sound? Should I follow this path?

[andrewbranch]

I’d like to get @weswigham’s thoughts.

[weswigham]

Honestly, this is probably a reasonable starter approach, but we definitely need to make sure this is reflected in the relationship cache key of the parent relation, since, if this result is cached, this lack of an error result could pollute file errors in the same way the extra completions-triggered error is currently polluting the error list.

Because of how inference blocked sources are applied, I think you'd need to set up a new relation cache key part (a la intersectionState) that gets checked & passed in at the top level of the relationship check (something like an isOrContainsInferenceBlockedType which is called on the source & target to switch a bit in the cache key), since the inference-blocked-ness of a type isn't actually part of the type's identity (and so doesn't cascade into new identities for types which contain them).

[Andarist]

Honestly, this is probably a reasonable starter approach,

Could you disambiguate "this" here? 😅 I'm not sure if you refer to the current content of this PR (avoiding excess property check) or to one of the mentioned alternatives.

Because of how inference blocked sources are applied, I think you'd need to set up a new relation cache key part (a la intersectionState) that gets checked & passed in at the top level of the relationship check (something like an isOrContainsInferenceBlockedType which is called on the source & target to switch a bit in the cache key), since the inference-blocked-ness of a type isn't actually part of the type's identity (and so doesn't cascade into new identities for types which contain them).

I'll have to study this a little bit more to understand this properly. I didn't yet dive too much into the mentioned bits of the checker. Thanks for the pointers though!

[weswigham]

Could you disambiguate "this" here? 😅 I'm not sure if you refer to the current content of this PR (avoiding excess property check) or to one of the mentioned alternatives.

The current approach in the PR.

I'll have to study this a little bit more to understand this properly. I didn't yet dive too much into the mentioned bits of the checker. Thanks for the pointers though!

Yeah, I'm just saying that results that depend on this new check need to be cached separately from normal results. (If they should be cached at all - given you could get multiple completions requests in a row that inference block and unblock different things, it's very possible that any cached comparisons done during an inference-blocked request should get thrown out entirely - I just don't know how negative that could be for perf.)

[jakebailey]

I've merged #52728; should this be closed, or is there still something to do here?