Skip to content

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

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jul 23, 2018

  1. Extract reduce-style [1] functions for two of the major components (jsdoc type checking and initializer type checking)
  2. Switch this-assignment location checking to the same style. Also introduce an enum to aid readability.
  3. Filter initializer types down to constructor-defined initializer types after the main loop.

[1] That is, functions that manually take a state parameter and return the updated state.

sandersn added 4 commits July 22, 2018 14:46
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.
@sandersn sandersn requested a review from a user July 23, 2018 17:34
@@ -4703,6 +4703,12 @@ namespace ts {
return undefined;
}

const enum JSThisAssignmentLocation {
Copy link

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));

Copy link
Member Author

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. :)

}
else if (declaredType !== errorType && type !== errorType &&
!isTypeIdenticalTo(declaredType, type) &&
!(symbol.flags & SymbolFlags.JSContainer)) {
Copy link

@ghost ghost Jul 23, 2018

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?

Copy link
Member Author

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)) {
Copy link

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.

Copy link
Member Author

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.)

members.set(name, s);
}
});
type = createAnonymousType(
Copy link

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)]);
Copy link

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.

(thisContainer.kind === SyntaxKind.FunctionExpression && !isPrototypePropertyAssignment(thisContainer.parent));
}

function getConstructuredDefinedThisAssignmentTypes(types: Type[], declarations: Declaration[]): Type[] | undefined {
Copy link

Choose a reason for hiding this comment

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

Typo: "Constructured"

const constructorTypes = []
for (let i = 0; i < types.length; i++) {
const declaration = declarations[i];
const expression = isBinaryExpression(declaration) ? declaration :
Copy link

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.

Copy link

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.

Copy link
Member Author

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.

function getConstructuredDefinedThisAssignmentTypes(types: Type[], declarations: Declaration[]): Type[] | undefined {
Debug.assert(types.length === declarations.length);
const constructorTypes = []
for (let i = 0; i < types.length; i++) {
Copy link

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);
});

@sandersn sandersn merged commit 2146a91 into master Jul 23, 2018
@sandersn sandersn deleted the sandersn/simplify-getWidenedTypeFromJSSpecialPropertyDeclarations branch July 23, 2018 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant