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

Josh-Cena · 2024-06-02T13:50:45Z

PR Checklist

Addresses an existing open issue: part of AST: provide better discriminated unions for ambiguous node types #6434
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

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

Also added a bunch of documentation.

typescript-eslint · 2024-06-02T13:50:48Z

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-02T13:51:03Z

✅ Deploy Preview for typescript-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 your Netlify site configuration.

nx-cloud · 2024-06-02T13:51:55Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit fc730f1. 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 💌 from NxCloud.

Josh-Cena

Some comments

Josh-Cena · 2024-06-02T14:01:30Z

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

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


TS now allows export type * from

Josh-Cena · 2024-06-02T14:01:53Z

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

   */
-  exportKind: ExportKind;
+  exportKind: 'value';


Tightened: export type default is not legal

Josh-Cena · 2024-06-02T14:02:33Z

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

@@ -64,9 +71,14 @@ export interface ExportNamedDeclarationWithoutSourceWithMultiple
  attributes: ImportAttribute[];
  declaration: null;
  source: null;
-  specifiers: ExportSpecifier[];


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

Josh-Cena · 2024-06-02T14:03:19Z

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

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


For the future: specifiers should be able to be null, like how typeParameters can be both empty or null

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.

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

Josh-Cena · 2024-06-02T14:03:36Z

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

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


Tightened: I think this is safe?

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

playground

Josh-Cena · 2024-06-02T14:03:47Z

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

+  /**
+   * 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;


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

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

Josh-Cena · 2024-06-02T14:05:02Z

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

@@ -19,7 +19,45 @@ export interface TSImportEqualsDeclaration extends BaseNode {
   * import F3 = require('mod');
   * ```
   */
-  moduleReference: EntityName | TSExternalModuleReference;
-  // TODO(#1852) - breaking change remove this as it is invalid
+  moduleReference: Identifier | TSQualifiedName | TSExternalModuleReference;


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

Josh-Cena · 2024-06-02T14:05:48Z

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

+export type TSImportEqualsDeclaration =
+  | TSImportEqualsNamespaceDeclaration
+  | TSImportEqualsRequireDeclaration;


Changed to union

Josh-Cena · 2024-06-02T14:06:30Z

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

+  // 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


Worth pointing out

Josh-Cena · 2024-06-02T14:07:02Z

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

+export type LetOrConstOrVarDeclaration =
+  | LetOrVarDeclaredDeclaration
+  | LetOrVarNonDeclaredDeclaration
+  | ConstDeclaredDeclaration
+  | ConstNonDeclaredDeclaration;


The biggest change: made LetOrConstOrVarDeclaration much stricter.

The thing that needs to be checked with these changes is like const 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 of const x: - we produce an AST so the types must allow the case of const with an init: null. playground

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

bradzacher

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!

Josh-Cena · 2024-06-02T16:40:46Z

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

bradzacher · 2024-06-03T00:14:47Z

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.

Josh-Cena · 2024-06-03T05:06:24Z

#9219, #9220, #9221

Josh-Cena · 2024-06-03T11:54:14Z

@bradzacher I am splitting normative changes out into separate PRs

bradzacher

bradzacher · 2024-06-23T12:12:20Z

a few lint errors @Josh-Cena otherwise ready to ship

Josh-Cena requested review from JoshuaKGoldberg and bradzacher June 2, 2024 13:52

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

4407bad

Josh-Cena force-pushed the ast-spec-tighten-1 branch from d02a303 to 4407bad Compare June 2, 2024 13:59

Josh-Cena commented Jun 2, 2024

View reviewed changes

Josh-Cena added 2 commits June 2, 2024 22:33

Fix

391172f

Lint fixes

1ffdb16

bradzacher requested changes Jun 2, 2024

View reviewed changes

Josh-Cena marked this pull request as draft June 3, 2024 05:06

Reset normative changes

53aa274

Josh-Cena marked this pull request as ready for review June 3, 2024 11:53

Josh-Cena added 2 commits June 3, 2024 20:29

Remove TODO

7feb479

Unset change

19374ea

Josh-Cena requested a review from bradzacher June 4, 2024 06:52

Josh-Cena changed the title feat(ast-spec): tighter types for declaration/* feat(ast-spec): tighter types and documentation for declaration/* Jun 4, 2024

bradzacher previously approved these changes Jun 23, 2024

View reviewed changes

Merge branch 'main' into ast-spec-tighten-1

091cc9e

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 Jun 23, 2024

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

Fix lint

fc730f1

Josh-Cena dismissed bradzacher’s stale review via fc730f1 June 23, 2024 13:17

bradzacher approved these changes Jun 23, 2024

View reviewed changes

bradzacher merged commit 5c4a5de into typescript-eslint:main Jun 23, 2024
61 checks passed

Josh-Cena deleted the ast-spec-tighten-1 branch June 23, 2024 13:58

github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2024

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

Uh oh!

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

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

Uh oh!

Conversation

Josh-Cena commented Jun 2, 2024

PR Checklist

Overview

Uh oh!

typescript-eslint bot commented Jun 2, 2024

Uh oh!

netlify bot commented Jun 2, 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 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

Josh-Cena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradzacher Jun 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradzacher Jun 2, 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 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!

Josh-Cena commented Jun 2, 2024

Uh oh!

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

Uh oh!

Uh oh!

Josh-Cena commented Jun 3, 2024

Uh oh!

Josh-Cena commented Jun 3, 2024

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

bradzacher commented Jun 23, 2024

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Jun 2, 2024 •

edited

Loading

nx-cloud bot commented Jun 2, 2024 •

edited

Loading

bradzacher Jun 2, 2024 •

edited

Loading

bradzacher Jun 2, 2024 •

edited

Loading

bradzacher left a comment •

edited

Loading

bradzacher commented Jun 3, 2024 •

edited

Loading