typescript-eslint/typescript-eslintPublic

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

feat(ast-spec): tighter types and documentation for declaration/*#9211

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 8 commits intotypescript-eslint:mainfromJosh-Cena:ast-spec-tighten-1

Jun 23, 2024

Merged

feat(ast-spec): tighter types and documentation for declaration/*#9211

bradzacher merged 8 commits intotypescript-eslint:mainfromJosh-Cena:ast-spec-tighten-1

Jun 23, 2024

Conversation

Copy link

Member

Josh-Cena commentedJun 2, 2024

PR Checklist

Addresses an existing open issue: part ofAST: provide better discriminated unions for ambiguous node types #6434
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Although the types are tighter, it contains no breaking tree shape changes.

Also added a bunch of documentation.

Copy link

Contributor

typescript-eslintbot commentedJun 2, 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.

Copy link

netlifybot commentedJun 2, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`fc730f1`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/667820724fdaf50008c178e7
😎 Deploy Preview	https://deploy-preview-9211--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 2 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 2, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitfc730f1. 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 1 target

nx run repo-tools:postinstall-script

Sent with 💌 fromNxCloud.

Josh-Cena requested review fromJoshuaKGoldberg andbradzacher

June 2, 2024 13:52

feat(ast-spec): tighter types for declaration/*

4407bad

Josh-Cena force-pushed theast-spec-tighten-1 branch fromd02a303 to4407badCompare

June 2, 2024 13:59

Josh-Cena commented

Jun 2, 2024

View reviewed changes

Copy link

MemberAuthor

Josh-Cena left a comment

Choose a reason for hiding this comment

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

Some comments

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

		/**
		* The kind of the export.
		*/
		// TODO(#1852) - breaking change remove this because it is a semantic error to have it

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

TS now allowsexport type * from

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

		* The kind of the export. Always `value` for default exports.
		*/
		exportKind:ExportKind;
		exportKind:'value';

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Tightened:export type default is not legal

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

		attributes:ImportAttribute[];
		declaration:null;
		source:null;
		specifiers:ExportSpecifier[];

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Removals here are not breaking because these properties are already declared on the base

packages/ast-spec/src/declaration/ImportDeclaration/spec.ts Outdated

Comment on lines 44 to 45

		// TODO: we should have a way to differentiate between these two cases
		specifiers:ImportClause[];

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

For the future:specifiers should be able to benull, like howtypeParameters can be both empty ornull

Copy link

Member

Choose a reason for hiding this comment

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

This won't ever happen, sadly. The ESTree spec is pretty well locked in once defined and so we're stuck with the decisions of the past.

If we change something like this then we're liable to break all sorts of rules that assume it's always defined.

In general we can only ever change our AST extension shapes and the "core" AST is fixed.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Hhhh, I forgot this is not TS specific. Fine then.

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

Comment on lines 6 to 9

		/**
		* TS1183: An implementation cannot be declared in ambient contexts.
		*/
		body:undefined;

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Tightened: I think this is safe?

Copy link

Member

Choose a reason for hiding this comment

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

This is not correct to do.
The error you cited is asemantic error, not asyntax error.
This means that TS produces an AST for the error and us we do too.

playground

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

Comment on lines 18 to 26

		/**
		* TS1040: 'async' modifier cannot be used in an ambient context.
		*/
		async:false;
		declare:true;
		/**
		* TS1221: Generators are not allowed in an ambient context.
		*/
		generator:false;

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Tightened

Copy link

Member

bradzacherJun 2, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Ditto above - the errors are semantic and TS still produces an AST - AND it includes these flags correctly.
So we can't refine these.

playgroubd

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Aren't we supposed to throw on semantic errors? Wasn't fisker fixing some of these before?

packages/ast-spec/src/declaration/TSImportEqualsDeclaration/spec.ts Outdated

		*/
		moduleReference:EntityName\|TSExternalModuleReference;
		// TODO(#1852) - breaking change remove this as it is invalid
		moduleReference:Identifier\|TSQualifiedName\|TSExternalModuleReference;

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Slight tightening:EntityName is a moving target and doesn't really make sense here. I will continue to fix uses of it. It broke whenThisExpression became a validEntityName.

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

Comment on lines +61 to +63

		exporttypeTSImportEqualsDeclaration=
		\|TSImportEqualsNamespaceDeclaration
		\|TSImportEqualsRequireDeclaration;

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Changed to union

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

Comment on lines +114 to +116

		// TODO: we emit the wrong AST for `module A.B {}`
		// https://github.com/typescript-eslint/typescript-eslint/pull/6272 only fixed namespaces
		// Maybe not worth fixing since it's legacy

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Worth pointing out

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

Comment on lines 94 to 98

		exporttypeLetOrConstOrVarDeclaration=
		\|LetOrVarDeclaredDeclaration
		\|LetOrVarNonDeclaredDeclaration
		\|ConstDeclaredDeclaration
		\|ConstNonDeclaredDeclaration;

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

The biggest change: madeLetOrConstOrVarDeclaration much stricter.

Copy link

Member

bradzacherJun 2, 2024•
edited
Loading

Choose a reason for hiding this comment

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

The thing that needs to be checked with these changes is likeconst x; - do we have a syntax error or do we produce an AST.

If it's the latter then we can't refine the types as the types have to reflect the AST we output.

For the specific example ofconst x: - we produce an AST so the types must allow the case ofconst with aninit: null.playground

or egconst x! = 1; - we produce an AST so the tyoes nust allow the case.playground

Josh-Cena added2 commits

June 2, 2024 22:33

Fix

391172f

Lint fixes

1ffdb16

bradzacher requested changes

Jun 2, 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.

A lot of this is great!
But there's a chunk that I think causes drift between the types and the ASTs we produce.

The important distinction when looking at TS errors is "does TS crash, or does it produce an AST"? In most cases it's the latter.
TS's parser is very forgiving and most errors are semantic errors, not syntactic errors - meaning it produces an AST even if the code has errors.

If TS produces an AST then in most cases we do too. So we need to ensure that the types respect that.

We can always add hard errors to our AST translation step to enforce invariants so that we can change the types! But doing so will be a breaking change for the v8 branch.

Please also add tests for any cases / permutations that you think are missing. More AST snapshots is always good!

Copy link

MemberAuthor

Josh-Cena commentedJun 2, 2024

We can always add hard errors to our AST translation step to enforce invariants so that we can change the types! But doing so will be a breaking change for the v8 branch.

Really, even when the AST is nonsense and no program is not going to work? For example#7202 was not breaking

Copy link

Member

bradzacher commentedJun 3, 2024•
edited
Loading

It generally would require a quick chat with the prettier folk. We want to make sure that we're not breaking their experience by throwing on parsable code. If they're okay then we can land it non-breaking.

In the past a lot of the invariants we've encoded were just the invariants that prettier itself encoded (allowing them to delete the checks and make assumptions).

I'm all for a stricter AST transform - just don't want to kill downstream usecases with it.