feat(typescript-estree): throw on invalid update expressions

[Josh-Cena]

PR Checklist

• Addresses an existing open issue: fixes Enhancement: Throw errors for invalid UpdateExpressions #7196
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

We should be able to report on things like a() = 1 and 1 = 1 as well, but I didn't do this because things like { a } = 1 makes it a little more complicated.

[typescript-eslint]

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.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	d6d6d70
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/657a67374fea510008a9a791
😎 Deploy Preview	https://deploy-preview-7202--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 96 (🔴 down 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 94cc251. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

🟥 Failed Commands
nx test eslint-plugin --coverage=false

✅ Successfully ran 19 targets

• nx test typescript-estree --coverage=false
• nx test type-utils --coverage=false
• nx test visitor-keys --coverage=false
• nx run types:build
• nx run repo-tools:postinstall-script
• nx test scope-manager --coverage=false
• nx test utils --coverage=false
• nx run-many --target=lint --parallel
• nx test parser --coverage=false
• nx test eslint-plugin-tslint --coverage=false
• nx test rule-schema-to-typescript-types --coverage=false
• nx test ast-spec --coverage=false
• nx test repo-tools --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx build website
• nx run integration-tests:test
• nx run-many --target=typecheck --parallel
• nx run eslint-plugin:generate:configs
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint

---

Sent with 💌 from NxCloud.

[bradzacher]

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 checker.ts file in GH cos it's too big but here are the relevant lines:

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

[JoshuaKGoldberg]

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.

[bradzacher]

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!

[Josh-Cena]

let [ ...a = [] ] = z;
let { ...b = {} } = z;

This PR only deals with update expressions, not assignment or declaration, so not those atm.

I'll add the assertion ones.

[fisker]

@Josh-Cena

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?

[bradzacher]

@fisker I did build this into the infra to make it possible to filter the tests:

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/tests/fixtures.test.ts#L21-L25

But I agree that it's not a very good DevX and pretty hacky. Open to suggestions to improve it!

[fisker]

I hope we run test from each directory, so we can
jest packages/ast-spec/src/expression/UpdateExpression/fixtures/_error_/literal/.

Similar to Prettier's approach https://github.com/prettier/prettier/blob/6ed37ad85731e4e62d81481d40dfd98c6a675a10/tests/format/js/array-spread/jsfmt.spec.js#L1

[bradzacher]

just one comment otherwise

[bradzacher]

I might have missed it but I think we're missing a case for this

[Josh-Cena]

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

[bradzacher]

just need to get CI passing and we can merge this