-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
add refactor of extract type #30562
add refactor of extract type #30562
Conversation
|
||
function visitor(node: Node) { | ||
if (isTypeReferenceNode(node)) { | ||
if (isIdentifier(node.typeName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it not necessary to handle the case where node.typeName
is not an identifier but is a qualified name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could TypeParameter be a QualifiedName
?
In checkAndCollectionTypeArguments
we only iter the TypeParameter whitch defined in the statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think in the common cases where you have to collect the type, the type name is an identifier and not a qualified name. but i'm really not sure, so that's why i asked.
update: came up with an example of a qualified name for the type query check. i think there could be unusual cases where this check can fail.
hasError = true; | ||
return; | ||
} | ||
else if (isTypeQueryNode(node) && isIdentifier(node.exprName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, why is it not necessary to handle the case where node.typeName
is not an identifier but is a qualified name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually, I think that QualifiedName
is not including in current statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And is not a TypeParameter and do not have TypeArguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like before, i think common cases are covered by the identifier check, but just came up with the following code:
interface I { a: string, f: (this: O, b: number) => typeof this.a };
something bad happens if we extract typeof this.a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Banned ThisTypeNode
, QualityName
and most left is ThisIdentifier
} | ||
} | ||
else if (isInferTypeNode(node)) { | ||
const conditionalTypeNode = findAncestor(node, isConditionalTypeNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this might be wrong. we are assuming that the infer type node is only allowed in the extends clause of a conditional type. so we want to check if selection
includes the conditional type node to whose extends clause the infer type node belongs. this conditional type node might not be the first conditional type ancestor we find because there could be nested conditional type nodes. for example:
type Crazy<T> = T extends [infer P, (infer R extends string ? string : never)] ? P & R : string;
we can apply the refactoring and extract (infer R extends string ? string : never)
to obtain this:
type NewType = (infer R extends string ? string : never); // error: 'infer' declarations are only permitted in the 'extends' clause of a conditional type
type Crazy<T> = T extends [infer P, NewType] ? P & R : string;
which gives an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, mis operation😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find type node which is the first condition that contains selected infer type node in extends node
Co-Authored-By: Kingwl <kingwenlu@gmail.com>
Co-Authored-By: Kingwl <kingwenlu@gmail.com>
also this one: #29539 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Kingwl, thanks so much for this! It's looking great! 🌟
I thought of two small additional cases I'd like to see, something along these lines:
// typeof other parameters within function signature?
function f(a: string, b: /*a*/typeof a/*b*/): /*a*/typeof b/*b*/ {
return '';
}
// Where do lines get inserted?
// The exact structure here doesn't matter,
// just want to see something within a block body
// to have the behavior defined in tests.
function id<T>(x: T): T {
return (() => {
const s: /*a*/typeof x/*b*/ = x;
return s;
})();
}
At a quick glance I think these will already produce the correct behavior, just want to make sure. Thanks!
🆙 |
⬆️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. The only thing is that this
is incorrectly refactored.
I also had a couple of questions and two small suggestions, although they are optional.
return rangeContainsStartEnd(r1, skipTrivia(file.text, node.pos), node.end); | ||
} | ||
|
||
function checkAndCollectionTypeArguments(checker: TypeChecker, selection: TypeNode, statement: Statement, file: SourceFile): TypeParameterDeclaration[] | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should be named collectTypeParameters
? I don't think you're really checking them, and it seems like they are intended to be type parameters in addition to arguments, so matching the type name TypeParameterDeclaration
feels a little better.
} | ||
|
||
function isStatementButNotBlock(n: Node): n is Statement { | ||
return n && isStatement(n) && !isBlock(n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in what cases will you encounter a block before a statement in findAncestor? That is, a block inside a statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
() => {
{
// here
}
}
And i'm cannot remember sure why i do that🤷🏻♂️
Is this unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I would have that that //here
would have its own statement, but I guess not. In that case, it seems necessary.
} | ||
|
||
function checkAndCollectionTypeArguments(checker: TypeChecker, selection: TypeNode, statement: Statement, file: SourceFile): TypeParameterDeclaration[] | undefined { | ||
let hasError = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems more elegant to turn this into a return value. It looks like the expected behaviour is to return undefined whenever hasError
is true, so stopping the tree walk early should be more efficient.
} | ||
} | ||
else if (isInferTypeNode(node)) { | ||
const conditionalTypeNode = findAncestor(node, n => isConditionalTypeNode(n) && rangeContainsSkipTrivia(n.extendsType, node, file)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the selection contains only a syntactically-correct infer type, then I think this code should (1) not set hasError
but also (2) not push any results into result
. What happens if you get an empty result list?
Test cases 31-33 show that this works, but I don't understand why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That caused:
- find the selections by largest and topmost type node
- Infer type node contains only a
TypeParameterDeclaration
in the following case:
type A = /*a*/Promise/*b*/
The selection is the TypeReference
which have the same range as the Identifier``Promise
.
In cases 31 and 32 were set the hasError
flag.
And the case33:
type Item<T> = T extends (infer /*a*/P/*b*/)[] ? P : never
The selection is the TypeParameter
(maybe, or an Identifier
), that is a declaration rather a TypeNode
. and we are already escaped before collectTypeParameters.
} | ||
} | ||
else { | ||
if (isThisIdentifier(node.exprName.left) && !rangeContainsSkipTrivia(selection, node.parent, file)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should skip this
elsewhere too:
class C {
m<T>(): /*a*/T | this | number/*b*/ {
return {} as any
}
}
currently produces type NewType<T> = T | this | number
@DanielRosenwasser you'll probably want to highlight this refactor in the release notes since it is the other half of the named-parameters refactor that @gabritto did. |
Fixes #23869
This pr implement a refactor to extract selected types into another type alias in ts or typedef jsdoc in js
follow things is WIP:
type name suggestion