feat(typescript-estree): restrict variable declarator definite/init combinations #9228

Josh-Cena · 2024-06-03T14:32:27Z

PR Checklist

Addresses an existing open issue: fixes AST tightening: around init/definite combinations on let/var/const declarators #9221
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

I added A LOT of fixtures (programmatically of course) and I noticed that Babel didn't give some errors while IMO they should.

…ombinations

typescript-eslint · 2024-06-03T14:32:29Z

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 · 2024-06-03T14:32:42Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`be1b324`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66b8181951c1680008e2042f
😎 Deploy Preview	https://deploy-preview-9228--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 4 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (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 · 2024-06-03T14:33:46Z

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Josh-Cena · 2024-06-03T15:57:37Z

packages/ast-spec/src/declaration/VariableDeclaration/fixtures/generate-fixtures.mjs

Script for generating this pile of fixtures. Let me know if I should trim it down (especially the error combinations)

Nit: I wouldn't bother committing this - just leaving it as a note in the PR description should be enough for us to find it later if we want to reproduce this later.

Ok but I designed this script to allow us to:

Quickly identify error condition combinations

Quickly move a bunch of fixtures around valid/invalid cases

Quickly generate more combinations,

so I think overall it helps with maintenance

Josh-Cena · 2024-06-03T16:00:11Z

packages/ast-spec/tests/fixtures-with-differences-errors.shot

+    "declaration/VariableDeclaration/fixtures/_error_/const-destructure-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/const-destructure-type-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/const-id-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/const-id-type-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/let-destructure-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/let-destructure-type-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/var-destructure-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/var-destructure-type-no-init/fixture.ts",


These are because we don't have the ability to detect whether a declaration is in an ambient context unless itself has declare

Josh-Cena · 2024-06-03T16:01:45Z

packages/ast-spec/tests/fixtures-with-differences-errors.shot

+    "declaration/VariableDeclaration/fixtures/_error_/const-id-definite-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/const-id-definite-type-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/declare-const-id-definite-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/declare-const-id-definite-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/declare-const-id-definite-type-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/declare-let-id-definite-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/declare-let-id-definite-type-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/declare-var-id-definite-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/declare-var-id-definite-type-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/let-id-definite-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/let-id-definite-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/let-id-definite-type-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/using-id-definite-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/using-id-definite-type-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/var-id-definite-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/var-id-definite-no-init/fixture.ts",
+    "declaration/VariableDeclaration/fixtures/_error_/var-id-definite-type-init/fixture.ts",


Babel doesn't detect whether a definite assignment is put on a non-declared let/var with type and no init. This is arguably a Babel bug

packages/ast-spec/src/declaration/VariableDeclaration/spec.ts

packages/ast-spec/src/special/VariableDeclarator/spec.ts

bradzacher · 2024-06-04T07:10:38Z

packages/ast-spec/src/declaration/VariableDeclaration/fixtures/generate-fixtures.mjs

Nit: I wouldn't bother committing this - just leaving it as a note in the PR description should be enough for us to find it later if we want to reproduce this later.

JoshuaKGoldberg · 2024-07-08T12:36:13Z

👋 @Josh-Cena did you mean to re-request review on this?

Josh-Cena · 2024-07-12T00:35:48Z

I actually don't remember where the discussion left off. But yeah @bradzacher I think another look is worthwhile. I still want to keep the fixture generation script.

bradzacher

I can't see one - could you please add an error fixture for

for (using x! of []) {}

Other than that - looking fantastic!
We can land once you've got that!

bradzacher

Can we also add this

interface VariableDeclarator {
  parent: VariableDeclaration;
}

to https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/types/src/ts-estree.ts for #6225

Josh-Cena · 2024-07-14T22:58:04Z

Will come back to it next week...!

Josh-Cena · 2024-07-19T15:40:38Z

@bradzacher: unfortunately, VariableDeclarator is no longer an interface, but a union, so we can no longer merge it. If we really want to, we have to merge parent into individual constituents. Does that sound fine?

codecov · 2024-07-19T15:57:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.27%. Comparing base (07700dd) to head (d765c76).

❗ Current head d765c76 differs from pull request most recent head be1b324

Please upload reports for the commit be1b324 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9228      +/-   ##
==========================================
+ Coverage   88.07%   88.27%   +0.20%     
==========================================
  Files         402      422      +20     
  Lines       13693    14679     +986     
  Branches     3982     4299     +317     
==========================================
+ Hits        12060    12958     +898     
- Misses       1324     1400      +76     
- Partials      309      321      +12

Flag	Coverage Δ
unittest	`88.27% <ø> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 115 files with indirect coverage changes

bradzacher · 2024-07-20T00:54:51Z

Yeah that's perfectly fine - if you have a look at the file you'll see that's the common pattern.

Eventually we'll just want to add parent to the AST and merge the file into the main ast.

bradzacher · 2024-07-20T02:07:59Z

Me, trying to review this PR.
Ffs github

bradzacher · 2024-07-20T10:48:02Z

Most of the other ast changes have gone into v8 - do we want to also merge this into v8 as well to avoid the inevitable merge conflicts?

Josh-Cena · 2024-07-20T14:52:31Z

Mmmm, that's a good point, although other AST changes I sent have already gone into main.

Josh-Cena · 2024-08-11T01:49:43Z

Oof, I didn't realize this PR is still unmerged. I've resolved the conflicts.

feat(typescript-estree): restrict variable declarator definite/init c…

7201c29

…ombinations

Josh-Cena requested a review from bradzacher June 3, 2024 14:32

Fix test

cea3f68

Josh-Cena force-pushed the var-decl-ast-invariant branch from 16faace to cea3f68 Compare June 3, 2024 15:26

Ok linter

f894e21

Josh-Cena commented Jun 3, 2024

View reviewed changes

bradzacher added the AST PRs and Issues about the AST structure label Jun 4, 2024

bradzacher requested changes Jun 4, 2024

View reviewed changes

Josh-Cena added 3 commits June 4, 2024 18:42

Fix for...of loop checking logic, other review

5220af0

Fix snapshots

eec9d96

Merge branch 'main' into var-decl-ast-invariant

0f2281e

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jun 17, 2024

Josh-Cena requested a review from bradzacher July 12, 2024 00:35

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jul 12, 2024

bradzacher previously approved these changes Jul 12, 2024

View reviewed changes

bradzacher added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jul 12, 2024

bradzacher mentioned this pull request Jul 14, 2024

Enhancement: add strict parent types for nodes that have well-defined parents #6225

Closed

37 tasks

bradzacher reviewed Jul 14, 2024

View reviewed changes

Add snapshot

a088f9c

Josh-Cena dismissed bradzacher’s stale review via a088f9c July 19, 2024 15:36

Merge branch 'master' into var-decl-ast-invariant

9ae5c9c

Add all parents

d6d7404

bradzacher previously approved these changes Jul 20, 2024

View reviewed changes

Fix lint

d765c76

Josh-Cena dismissed bradzacher’s stale review via d765c76 July 20, 2024 02:15

JoshuaKGoldberg requested a review from bradzacher July 29, 2024 15:43

JoshuaKGoldberg removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jul 29, 2024

Merge branch 'master' into var-decl-ast-invariant

be1b324

bradzacher approved these changes Aug 11, 2024

View reviewed changes

bradzacher merged commit a4992ff into typescript-eslint:main Aug 11, 2024
56 of 57 checks passed

Josh-Cena deleted the var-decl-ast-invariant branch August 11, 2024 03:35

github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2024

Uh oh!

feat(typescript-estree): restrict variable declarator definite/init combinations #9228

feat(typescript-estree): restrict variable declarator definite/init combinations #9228

Uh oh!

Conversation

Josh-Cena commented Jun 3, 2024

PR Checklist

Overview

Uh oh!

typescript-eslint bot commented Jun 3, 2024

Uh oh!

netlify bot commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

nx-cloud bot commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

Josh-Cena Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradzacher Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

Josh-Cena Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

Josh-Cena Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

Josh-Cena Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bradzacher Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg commented Jul 8, 2024

Uh oh!

Josh-Cena commented Jul 12, 2024

Uh oh!

bradzacher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

Josh-Cena commented Jul 14, 2024

Uh oh!

Josh-Cena commented Jul 19, 2024

Uh oh!

codecov bot commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bradzacher commented Jul 20, 2024

Uh oh!

bradzacher commented Jul 20, 2024

Uh oh!

bradzacher commented Jul 20, 2024

Uh oh!

Josh-Cena commented Jul 20, 2024

Uh oh!

Josh-Cena commented Aug 11, 2024

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Jun 3, 2024 •

edited

Loading

nx-cloud bot commented Jun 3, 2024 •

edited

Loading

Josh-Cena Jun 3, 2024 •

edited

Loading

bradzacher left a comment •

edited

Loading

codecov bot commented Jul 19, 2024 •

edited

Loading