typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star16k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

bradzacher merged 11 commits intotypescript-eslint:mainfromJosh-Cena:var-decl-ast-invariant

Aug 11, 2024

Merged

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

bradzacher merged 11 commits intotypescript-eslint:mainfromJosh-Cena:var-decl-ast-invariant

Aug 11, 2024

Conversation

Copy link

Member

Josh-Cena commentedJun 3, 2024

PR Checklist

Addresses an existing open issue:fixesAST tightening: around init/definite combinations on let/var/const declarators #9221
That issue was marked asaccepting prs
Steps inContributing 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.

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

7201c29

…ombinations

Copy link

Contributor

typescript-eslintbot commentedJun 3, 2024

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.

Josh-Cena requested a review frombradzacher

June 3, 2024 14:32

Copy link

netlifybot commentedJun 3, 2024•
edited
Loading

✅ Deploy Preview fortypescript-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 yourNetlify site configuration.

Copy link

nx-cloudbot commentedJun 3, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitbe1b324. 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 💌 fromNxCloud.

Fix test

cea3f68

Josh-Cena force-pushed thevar-decl-ast-invariant branch from16faace tocea3f68Compare

June 3, 2024 15:26

Ok linter

f894e21

Josh-Cena commented

Jun 3, 2024

View reviewed changes

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

Copy link

MemberAuthor

Josh-CenaJun 3, 2024•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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 commented

Jun 3, 2024

View reviewed changes

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

Comment on lines +20 to +27

		"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",

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

Josh-Cena commented

Jun 3, 2024

View reviewed changes

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

Comment on lines +59 to +75

		"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",

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

bradzacher added the ASTPRs and Issues about the AST structure label

Jun 4, 2024

bradzacher requested changes

Jun 4, 2024

View reviewed changes

packages/ast-spec/src/declaration/VariableDeclaration/spec.ts OutdatedShow resolvedHide resolved

packages/ast-spec/src/declaration/VariableDeclaration/spec.tsShow resolvedHide resolved

packages/ast-spec/src/declaration/VariableDeclaration/spec.ts OutdatedShow resolvedHide resolved

packages/ast-spec/src/declaration/VariableDeclaration/spec.tsShow resolvedHide resolved

packages/ast-spec/src/special/VariableDeclarator/spec.ts OutdatedShow resolvedHide resolved

packages/ast-spec/src/special/VariableDeclarator/spec.tsShow resolvedHide resolved

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

Copy link

Member

Choose a reason for hiding this comment

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

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.

Josh-Cena added3 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 responseIssues waiting for a reply from the OP or another party label

Jun 17, 2024

Copy link

Member

JoshuaKGoldberg commentedJul 8, 2024

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

Copy link

MemberAuthor

Josh-Cena commentedJul 12, 2024

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.

Josh-Cena requested a review frombradzacher

July 12, 2024 00:35

github-actionsbot removed the awaiting responseIssues 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

Copy link

Member

bradzacher left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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

for(usingx!of[]){}

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

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 strictparent types for nodes that have well-defined parents#6225

Closed

37 tasks

bradzacher reviewed

Jul 14, 2024

View reviewed changes

Copy link

Member

bradzacher left a comment

Choose a reason for hiding this comment

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

Can we also add this

interfaceVariableDeclarator{parent:VariableDeclaration;}

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

Copy link

MemberAuthor

Josh-Cena commentedJul 14, 2024

Will come back to it next week...!

Add snapshot

a088f9c

Josh-Cena dismissedbradzacher’sstale review viaa088f9c

July 19, 2024 15:36

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

9ae5c9c

Copy link

MemberAuthor

Josh-Cena commentedJul 19, 2024

@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 mergeparent into individual constituents. Does that sound fine?

Copy link

codecovbot commentedJul 19, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗Current headd765c76 differs from pull request most recent headbe1b324
Pleaseupload reports for the commitbe1b324 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