Skip to content

Fixed an issue with write type being left as non-instantiated when coming from a merged instantiation #56322

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

Merged

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Nov 6, 2023

fixes #56320

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 6, 2023
@@ -14501,6 +14501,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
clone.parent = singleProp.valueDeclaration?.symbol?.parent;
clone.links.containingType = containingType;
clone.links.mapper = links?.mapper;
clone.links.writeType = getWriteTypeOfSymbol(singleProp);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. This merged instantiation doesn't carry over CheckFlags.Instantiated so getWriteTypeOfSymbol ended up calling getWriteTypeOfAccessors instead of getWriteTypeOfInstantiatedSymbol
  2. the same problem for read types is mitigated by the createSymbolWithType call here. It just sets the .type so further down the road the CheckFlags.Instantiated doesn't matter since the .type is already cached
  3. so I just followed the same solution here
  4. I tried to do it only when links?.type !== writeType but links?.type is an instantiated one here... and getWriteTypeOfSymbol(singleProp) is also an instantiated one. So both of them are just number and that's why I decided, for now, to just assign this unconditionally here

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be links?.writeType, so the clone here doesn't eagerly cause the prop write type to be made, if it hasn't been yet?

This merged instantiation doesn't carry over CheckFlags.Instantiated

Shouldn't it just... do that? The symbol has target and mapper set, so Instantiated symbol type lookup logic should work - the flag just needs to be carried over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't it just... do that? The symbol has target and mapper set, so Instantiated symbol type lookup logic should work - the flag just needs to be carried over.

I tried that at first but it affects some displays, see the diff below:

diff --git a/tests/baselines/reference/completionEntryForUnionMethod.baseline b/tests/baselines/reference/completionEntryForUnionMethod.baseline
index 6ee4743746..9d346a9dbf 100644
--- a/tests/baselines/reference/completionEntryForUnionMethod.baseline
+++ b/tests/baselines/reference/completionEntryForUnionMethod.baseline
@@ -11,7 +11,7 @@
 // | (method) Array<T>.indexOf(searchElement: never, fromIndex?: number): number
 // | (method) Array<T>.join(separator?: string): string
 // | (method) Array<T>.lastIndexOf(searchElement: never, fromIndex?: number): number
-// | (property) Array<T>.length: number
+// | (property) Array<string>.length: number
 // | (method) Array<T>.map<unknown>(callbackfn: ((value: string, index: number, array: string[]) => unknown) & ((value: number, index: number, array: number[]) => unknown), thisArg?: any): unknown[]
 // | (method) Array<T>.pop(): string | number
 // | (method) Array<T>.push(...items: never[]): number
@@ -1792,8 +1792,8 @@
               "kind": "punctuation"
             },
             {
-              "text": "T",
-              "kind": "typeParameterName"
+              "text": "string",
+              "kind": "keyword"
             },
             {
               "text": ">",

What happens here is this:

var a: Array<string> | Array<number>;
a./*1*/length
// actual: (property) Array<string>.length: number
// expected: (property) Array<T>.length: number

This directly relates to the comment above the assignment to mergedInstantiations, see here:

                            // If we merged instantiations of a generic type, we replicate the symbol parent resetting behavior we used
                            // to do when we recorded multiple distinct symbols so that we still get, eg, `Array<T>.length` printed
                            // back and not `Array<string>.length` when we're looking at a `.length` access on a `string[] | number[]`

Overall, the whole createUnionOrIntersectionProperty doesn't propagate CheckFlags.Instantiated. I can't judge if this is right or not but it looks somewhat deliberate here - and it seems to me that if that would be needed for more than this then there would be a lot of other bug reports related to read types. That's why I went with this solution

Shouldn't this be links?.writeType, so the clone here doesn't eagerly cause the prop write type to be made, if it hasn't been yet?

That could only be done if I'd carry over CheckFlags.Instantiated. Otherwise, I don't see a way to compute this lazily for the merged instantiation if the links.writeType is not available at this time here.

But even apart of that, those write types are eagerly computed in the other branch below - the one that iterates over props. So I don't think this additional eager call here is something to be worried about

@Andarist Andarist force-pushed the fix/merged-instantiation-write-type branch from 4699623 to 1795f52 Compare November 6, 2023 11:45
@typescript-bot typescript-bot removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 21, 2023
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug labels Nov 21, 2023
@weswigham weswigham merged commit 3b1db10 into microsoft:main Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Property assignments are not valid for intersections of generic class instances with union types
3 participants