Uh oh!
There was an error while loading. Please reload this page.
Drop unnecessary type arguments in the isolated declarations quick fix - #59665
Conversation
This is the TypeNode representation of a ReferenceType that includes the minimal number of typeArguments that are still semantically equivalent to the full type. Also use this functionality in the isolatedDeclaration autofixer to fixmicrosoft#58449
These are the cases where our heuristic makes some compromise that isn't what we would have preferred, but add the unit tests as a way of documenting the current behavior.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f4eea52 to
de61d6dCompareCo-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
Also, mark typeToMinimizedReferenceType as `@internal`.
Ben Lickly (blickly)
commented
Aug 20, 2024
OK, I switched to the more idiomatic way of creating a new AST node, rather than trying to mutate anything. PTAL |
| const genericType = type as GenericType; | ||
| if (genericType.typeArguments) { | ||
| const cutoff = endOfRequiredTypeParameters(checker, genericType); | ||
| if (cutoff !== undefined && typeNode.typeArguments) { |
There was a problem hiding this comment.
cutoff can't be undefined; should this instead be a check that skips this block if the length is different than the existing node?
There was a problem hiding this comment.
Good catch. Done.
| description: "Add annotation of type 'MyIterator<number>'", | ||
| index: 0, | ||
| newFileContent: | ||
| `interface MyIterator<T, TReturn = any, TNext = undefined> {} |
There was a problem hiding this comment.
Do we have tests for generators elsewhere? I seem to recall us auto-adding type long type annotations for those...
There was a problem hiding this comment.
Sure, I can add a case with Generator.
For reasons I don't fully understand, Generator<T> and Iterator<T> get generated with the full 3 type arguments, whereas Iterable<T> and IterableIterator<T> are special cased inside the TypeChecker to omit the extra arguments when unnecessary.
The special casing inside typeReferenceToTypeNode:
// Maybe we should do this for more types, but for now we only elide type arguments that are
// identical to their associated type parameters' defaults for `Iterable`, `IterableIterator`,
// `AsyncIterable`, and `AsyncIterableIterator` to provide backwards-compatible .d.ts emit due
// to each now having three type parameters instead of only one.
if (
isReferenceToType(type, getGlobalIterableType(/*reportErrors*/ false)) ||
isReferenceToType(type, getGlobalIterableIteratorType(/*reportErrors*/ false)) ||
isReferenceToType(type, getGlobalAsyncIterableType(/*reportErrors*/ false)) ||
isReferenceToType(type, getGlobalAsyncIterableIteratorType(/*reportErrors*/ false))
) {
if (
!type.node || !isTypeReferenceNode(type.node) || !type.node.typeArguments ||
type.node.typeArguments.length < typeParameterCount
) {
while (typeParameterCount > 0) {
const typeArgument = typeArguments[typeParameterCount - 1];
const typeParameter = type.target.typeParameters[typeParameterCount - 1];
const defaultType = getDefaultFromTypeParameter(typeParameter);
if (!defaultType || !isTypeIdenticalTo(typeArgument, defaultType)) {
break;
}
typeParameterCount--;
}
}
}
There was a problem hiding this comment.
That was a part of #58243.
There was a problem hiding this comment.
Makes sense. Thanks for the context.
Also include test cases with the global `Generator` and `Iterator` which, unlike `Iterable` and `IterableIterator`, are not special cased in the type system and were previously always generated with 3 type arguments.
| description: "Add return type 'Generator<number>'", | ||
| index: 0, | ||
| newFileContent: | ||
| `export function foo(x: Generator<number>): Generator<number> { |
There was a problem hiding this comment.
To be clear I was meaning to write function *foo() { } here and then see what happens.
There was a problem hiding this comment.
Ah, OK, added.
It's not technically related to the rest of this PR since it gets inferred a precise return that needs all 3 type arguments, but it's a good case to include in the test suite anyway.
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
LGTM I think, but would be good to get a couple of other eyes on it.
Ben Lickly (blickly)
commented
Aug 28, 2024
Any advice on who would make a good secondary reviewer? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…gerenrics-oversimplification.ts Fix typo Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
Simplify loop Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
ee3d89e to
cd0cd4eCompareJake Bailey (jakebailey)
commented
Sep 16, 2024
Tested locally with main and it merges and passes cleanly. |
Drops unnecessary type arguments in the isolated declarations quick fix by repeatedly trying to resolve a resulting reference type with a prefix of the full type arguments until finding the minimal number of arguments that work.
Fixes#58449
(I'm sure that the code at present isn't using the proper APIs, but I'd appreciate feedback)