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.
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.
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.
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.
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.
Might be neater to do this check outside and type the parameter as expression: BinaryExpression.
There was a problem hiding this comment.
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.
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.
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 { |
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.
This looks similar to the code for const expression = in getWidenedTypeFromJSSpecialPropertyDeclarations.
There was a problem hiding this comment.
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.
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.
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);
});
sandersn
deleted the
sandersn/simplify-getWidenedTypeFromJSSpecialPropertyDeclarations
branch
[1] That is, functions that manually take a state parameter and return the updated state.