-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Simplify getWidenedTypeFromJSSpecialPropertyDeclarations #25868
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
Simplify getWidenedTypeFromJSSpecialPropertyDeclarations #25868
Conversation
1. Handle this-property assignments 2. Handle other special property assignment I have only started simplifying [2].
1. Look for jsdoc 2. Look for type of initializers Both can be broken into their own functions.
Then, convert the 2 extracted functions to reduce-style functions that take state and return the updated state.
src/compiler/checker.ts
Outdated
@@ -4703,6 +4703,12 @@ namespace ts { | |||
return undefined; | |||
} | |||
|
|||
const enum JSThisAssignmentLocation { |
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'm not sure this really is neater than two booleans. Here's the diff to convert to booleans:
- let definedIn = JSThisAssignmentLocation.None;
+ let definedInConstructor = false, definedInMethod = false;
- definedIn |= isDeclarationInConstructor(expression) ? JSThisAssignmentLocation.Constructor : JSThisAssignmentLocation.Method;
+ if (isDeclarationInConstructor(expression)) { definedInConstructor = true; } else { definedInMethod = true; }
- let constructorTypes = definedIn & JSThisAssignmentLocation.Constructor ? getConstructuredDefinedThisAssignmentTypes(types!, symbol.declarations) : undefined;
+ let constructorTypes = definedInConstructor ? getConstructuredDefinedThisAssignmentTypes(types!, symbol.declarations) : undefined;
- if (definedIn & JSThisAssignmentLocation.Method) {
+ if (definedInMethod) {
- definedIn |= JSThisAssignmentLocation.Constructor;
+ definedInConstructor = true;
- const widened = getWidenedType(addOptionality(type, definedIn === JSThisAssignmentLocation.Method));
+ const widened = getWidenedType(addOptionality(type, definedInMethod && !definedInConstructor));
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.
Fair enough. I started getting rid of booleans and couldn't stop. :)
src/compiler/checker.ts
Outdated
} | ||
else if (declaredType !== errorType && type !== errorType && | ||
!isTypeIdenticalTo(declaredType, type) && | ||
!(symbol.flags & SymbolFlags.JSContainer)) { |
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 don't understand this condition? Why do we ignore the type when it's in a JSContainer
?
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's ... uh ... a good question. Removing it doesn't cause any tests to fail. I added it in #20198, but I think there's enough test coverage in that PR that removal is safe.
|
||
/** If we don't have an explicit JSDoc type, get the type from the initializer. */ | ||
function getInitializerTypeFromSpecialDeclarations(symbol: Symbol, resolvedSymbol: Symbol | undefined, expression: Expression, special: SpecialPropertyAssignmentKind) { | ||
if (isBinaryExpression(expression)) { |
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.
Might be neater to do this check outside and type the parameter as expression: BinaryExpression
.
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.
Putting the check as an inline isBinaryExpression(expression) ? getInitializer...
is a bit unwieldy, and I expect that we may end up doing some real work in the non-binary expression-case instead of just return neverType
. (You need to return some type so that types.length === symbol.declarations.length
in getConstructorDefinedThisAssignmentTypes.)
src/compiler/checker.ts
Outdated
members.set(name, s); | ||
} | ||
}); | ||
type = createAnonymousType( |
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.
The type we create here is definitely not isEmptyArrayLiteralType
, right? So can just return immediately?
if (members.has(name)) { | ||
const exportedMember = exportedType.members.get(name)!; | ||
const union = createSymbol(s.flags | exportedMember.flags, name); | ||
union.type = getUnionType([getTypeOfSymbol(s), getTypeOfSymbol(exportedMember)]); |
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.
There are a lot of getUnionType
calls taking arrays with exactly two elements, maybe there are performance improvements to be had if there were a version specialized for that.
src/compiler/checker.ts
Outdated
(thisContainer.kind === SyntaxKind.FunctionExpression && !isPrototypePropertyAssignment(thisContainer.parent)); | ||
} | ||
|
||
function getConstructuredDefinedThisAssignmentTypes(types: Type[], declarations: Declaration[]): Type[] | 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.
Typo: "Constructured"
src/compiler/checker.ts
Outdated
const constructorTypes = [] | ||
for (let i = 0; i < types.length; i++) { | ||
const declaration = declarations[i]; | ||
const expression = isBinaryExpression(declaration) ? declaration : |
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.
This looks similar to the code for const expression =
in getWidenedTypeFromJSSpecialPropertyDeclarations
.
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.
Also, since we're about to test isBinaryExpression(expression)
, might as well make if BinaryExpression | undefined
by using : undefined
instead of : declaration
.
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.
Yeah, the declaration/expression
handling is still a bit of mess. I took your second suggestion to make the second check more concise, but I'm out of time for this refactoring so I think I'll leave it for now.
src/compiler/checker.ts
Outdated
function getConstructuredDefinedThisAssignmentTypes(types: Type[], declarations: Declaration[]): Type[] | undefined { | ||
Debug.assert(types.length === declarations.length); | ||
const constructorTypes = [] | ||
for (let i = 0; i < types.length; i++) { |
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.
Not obvious but this works as a filter:
return types.filter((_, i) => {
const declaration = declarations[i];
const expression = isBinaryExpression(declaration) ? declaration :
isBinaryExpression(declaration.parent) ? declaration.parent : declaration;
return isBinaryExpression(expression) && isDeclarationInConstructor(expression);
});
[1] That is, functions that manually take a state parameter and return the updated state.