Skip to content

Cache expression type when checking assertion #54224

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 13, 2023
Merged

Cache expression type when checking assertion #54224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1175676
Cache expression type
gabritto May 12, 2023
c790579
use assertIsDefined
gabritto May 12, 2023
372a1ab
inline helper
gabritto May 12, 2023
bb14838
style change
gabritto May 12, 2023
5b536d5
avoid assertion check when possible
gabritto May 12, 2023
460c36b
Revert "avoid assertion check when possible"
gabritto May 12, 2023
bb2b5f7
Revert "inline helper"
gabritto May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34523,6 +34523,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
return getRegularTypeOfLiteralType(exprType);
}
const links = getNodeLinks(node);
Copy link
Member

Choose a reason for hiding this comment

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

By moving the checkExpression for the as const case into the if, it's no longer used in the deferred-check case at all; so you shouldn't actually need to cache it like this (which is a tad unsafe, so if it's avoidable, I'd like to avoid it), since you'll either do it once early or once deferred.

Copy link
Member

Choose a reason for hiding this comment

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

So what're you thinking @weswigham? Place this in an explicit else?

Copy link
Member

Choose a reason for hiding this comment

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

Or I guess, keep it in the same as before, but then cache it on links unconditionally?

That makes sense, you're calling it twice in some cases anyway now, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly, that's what I was doing in https://github.com/microsoft/TypeScript/pull/53261/files/45124b1447f2ff88bdf2f9e65f0e1766a80a70a3, and that's what caused xstate checking perf to increase by 10-13% (#53261 (comment)).

Copy link
Member

@DanielRosenwasser DanielRosenwasser May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@gabritto but aren't you calling checkExpression twice now? whoops, duh, it's an early return.

Copy link
Member

Choose a reason for hiding this comment

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

I guess there's no "checking twice" but there's some code deduplication - checkExpression is going to happen anyway. I'd rather we got rid of that and go back to having exprType

That said, I don't see why avoiding deferral is a problem in that case.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we got rid of that and go back to having exprType

I wouldn't. The CheckMode-independent unconditional cache is unsafe and could lead to weird inconsistencies between signature checking states, which can lead to odd editor/command line error inconsistencies. We do it in one or two other places for perf reasons, but it's to be avoided where possible.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const links = getNodeLinks(node);
function checkAssertionWorker(node: JSDocTypeAssertion | AssertionExpression, checkMode: CheckMode | undefined) {
if (isConstTypeReference(type)) {
if (!isValidConstAssertionArgument(expression)) {
error(expression, Diagnostics.A_const_assertions_can_only_be_applied_to_references_to_enum_members_or_string_number_boolean_array_or_object_literals);
}
return getRegularTypeOfLiteralType(checkExpression(expression, checkMode));
}
checkSourceElement(type);
checkNodeDeferred(node);
return getTypeFromTypeNode(type);
}

and no changes in checkAssertionDeferred from current main should result in the same number of checkExpression calls as the unsafe cache here, but without needing an unsafe cache, is what I'm trying to say. :P

Copy link
Member

@DanielRosenwasser DanielRosenwasser May 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So

  1. getting rid of assertionExpressionType on NodeLinks entirely
  2. just only doing checkExpression in const contexts and in the deferred check.

Makes sense.

links.assertionExpressionType = exprType;
checkSourceElement(type);
checkNodeDeferred(node);
return getTypeFromTypeNode(type);
Expand All @@ -34547,9 +34549,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function checkAssertionDeferred(node: JSDocTypeAssertion | AssertionExpression) {
const { type, expression } = getAssertionTypeAndExpression(node);
const { type } = getAssertionTypeAndExpression(node);
const errNode = isParenthesizedExpression(node) ? type : node;
const exprType = getRegularTypeOfObjectLiteral(getBaseTypeOfLiteralType(checkExpression(expression)));
const links = getNodeLinks(node);
Debug.assertIsDefined(links.assertionExpressionType);
const exprType = getRegularTypeOfObjectLiteral(getBaseTypeOfLiteralType(links.assertionExpressionType));
const targetType = getTypeFromTypeNode(type);
if (!isErrorType(targetType)) {
addLazyDiagnostic(() => {
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6060,6 +6060,7 @@ export interface NodeLinks {
spreadIndices?: { first: number | undefined, last: number | undefined }; // Indices of first and last spread elements in array literal
parameterInitializerContainsUndefined?: boolean; // True if this is a parameter declaration whose type annotation contains "undefined".
fakeScopeForSignatureDeclaration?: boolean; // True if this is a fake scope injected into an enclosing declaration chain.
assertionExpressionType?: Type; // Cached type of the expression of a type assertion
}

/** @internal */
Expand Down