-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(typescript-estree): throw on invalid update expressions #7202
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
feat(typescript-estree): throw on invalid update expressions #7202
Conversation
Thanks for the PR, @Josh-Cena! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I had a look at the TS codebase and it's a bit messy with regard to this class of errors. Here's a few snippets I can find: I can't link the function checkReferenceExpression(expr: Expression, invalidReferenceMessage: DiagnosticMessage, invalidOptionalChainMessage: DiagnosticMessage): boolean {
// References are combinations of identifiers, parentheses, and property accesses.
const node = skipOuterExpressions(expr, OuterExpressionKinds.Assertions | OuterExpressionKinds.Parentheses);
if (node.kind !== SyntaxKind.Identifier && !isAccessExpression(node)) {
error(expr, invalidReferenceMessage);
return false;
}
if (node.flags & NodeFlags.OptionalChain) {
error(expr, invalidOptionalChainMessage);
return false;
}
return true;
} export function isAccessExpression(node: Node): node is AccessExpression {
return node.kind === SyntaxKind.PropertyAccessExpression || node.kind === SyntaxKind.ElementAccessExpression;
} and function checkReferenceAssignment(target: Expression, sourceType: Type, checkMode?: CheckMode): Type {
const targetType = checkExpression(target, checkMode);
const error = target.parent.kind === SyntaxKind.SpreadAssignment ?
Diagnostics.The_target_of_an_object_rest_assignment_must_be_a_variable_or_a_property_access :
Diagnostics.The_left_hand_side_of_an_assignment_expression_must_be_a_variable_or_a_property_access;
const optionalError = target.parent.kind === SyntaxKind.SpreadAssignment ?
Diagnostics.The_target_of_an_object_rest_assignment_may_not_be_an_optional_property_access :
Diagnostics.The_left_hand_side_of_an_assignment_expression_may_not_be_an_optional_property_access;
if (checkReferenceExpression(target, error, optionalError)) {
checkTypeAssignableToAndOptionallyElaborate(sourceType, targetType, target, target);
}
if (isPrivateIdentifierPropertyAccessExpression(target)) {
checkExternalEmitHelpers(target.parent, ExternalEmitHelpers.ClassPrivateFieldSet);
}
return sourceType;
}
function checkReferenceExpression(expr: Expression, invalidReferenceMessage: DiagnosticMessage, invalidOptionalChainMessage: DiagnosticMessage): boolean {
// References are combinations of identifiers, parentheses, and property accesses.
const node = skipOuterExpressions(expr, OuterExpressionKinds.Assertions | OuterExpressionKinds.Parentheses);
if (node.kind !== SyntaxKind.Identifier && !isAccessExpression(node)) {
error(expr, invalidReferenceMessage);
return false;
}
if (node.flags & NodeFlags.OptionalChain) {
error(expr, invalidOptionalChainMessage);
return false;
}
return true;
} export function isPrivateIdentifierPropertyAccessExpression(node: Node): node is PrivateIdentifierPropertyAccessExpression {
return isPropertyAccessExpression(node) && isPrivateIdentifier(node.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.
LGTM - glad you're diving into this @Josh-Cena!
@bradzacher - since you commented in the issue I'll wait for your review for a bit.
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.
These are also semantically valid assignment:
declare let x: 1;
(x as 1) = 1;
(<1>x) = 1;
(x satisfies 1) = 1;
Which is covered by the skipOuterExpressions(expr, OuterExpressionKinds.Assertions | OuterExpressionKinds.Parentheses);
line in the TS codebase.
Also currently this is invalid (for now):
declare const y: { z: number };
y?.z = 1;
I don't know if we have handling for cases like this (if we don't - do we want to add them in this PR?):
declare const z: string[];
let [ ...a = [] ] = z;
let { ...b = {} } = z;
Other than that looking good!
This PR only deals with update expressions, not assignment or declaration, so not those atm. I'll add the assertion ones. |
Side question: when I work on invalid AST checks, I feel it's hard to debug, maybe we need find a better way to run test on single fixture, do you feel the same? |
@fisker I did build this into the infra to make it possible to filter the tests: But I agree that it's not a very good DevX and pretty hacky. Open to suggestions to improve it! |
I hope we run test from each directory, so we can Similar to Prettier's approach https://github.com/prettier/prettier/blob/6ed37ad85731e4e62d81481d40dfd98c6a675a10/tests/format/js/array-spread/jsfmt.spec.js#L1 |
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.
} | ||
return true; | ||
case SyntaxKind.ParenthesizedExpression: | ||
case SyntaxKind.TypeAssertionExpression: |
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 might have missed it but I think we're missing a case for this
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.
@bradzacher Wow, thanks for catching that! In doing so I realized TypeAssertionExpression
is only for <number>foo
and foo as number
is a separate syntax kind.
Co-authored-by: fisker Cheung <lionkay@gmail.com>
94cc251
to
a9918d3
Compare
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.
just need to get CI passing and we can merge this
PR Checklist
UpdateExpression
s #7196Overview
We should be able to report on things like
a() = 1
and1 = 1
as well, but I didn't do this because things like{ a } = 1
makes it a little more complicated.