Skip to content

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Apr 17, 2025

It wasn't possible for extends field to be Some containing an empty Vec because this is not legal code:

interface Foo extends {}

So use Vec for extends field, instead of Option<Vec>. This prevents creating an invalid AST, and simplifies code in various places.

@github-actions github-actions bot added A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic A-ast Area - AST A-codegen Area - Code Generation C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Apr 17, 2025
Copy link
Member Author

overlookmotel commented Apr 17, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@overlookmotel overlookmotel marked this pull request as ready for review April 17, 2025 13:12
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 17, 2025

CodSpeed Instrumentation Performance Report

Merging #10472 will not alter performance

Comparing 04-17-refactor_ast_change_tsinterfacedeclaration_extends_from_option_vec_to_vec_ (7212803) with main (fbf0ae2)

Summary

✅ 36 untouched benchmarks

@overlookmotel
Copy link
Member Author

overlookmotel commented Apr 17, 2025

The only remaining Option<Vec> in AST now is in ImportDeclaration's specifiers field:

pub struct ImportDeclaration<'a> {
pub span: Span,
/// `None` for `import 'foo'`, `Some([])` for `import {} from 'foo'`
#[estree(via = ImportDeclarationSpecifiers)]
pub specifiers: Option<Vec<'a, ImportDeclarationSpecifier<'a>>>,
pub source: StringLiteral<'a>,
#[estree(skip)]
pub phase: Option<ImportPhase>,
/// Some(vec![]) for empty assertion
#[estree(rename = "attributes", via = ImportDeclarationWithClause)]
pub with_clause: Option<Box<'a, WithClause<'a>>>,
/// `import type { foo } from 'bar'`
#[ts]
pub import_kind: ImportOrExportKind,
}

That one should stay as is, because None and Some([]) have different meanings.

@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Apr 18, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Apr 18, 2025

Merge activity

  • Apr 17, 10:23 PM EDT: Dunqing added this pull request to the Graphite merge queue.
  • Apr 18, 2:25 AM UTC: The Graphite merge queue couldn't merge this PR because it was not satisfying all requirements (Failed CI: 'Test (ubuntu-latest)', 'Test (macos-latest)', 'Test wasm32-wasip1-threads', 'Clippy', 'Doc', 'Benchmark (lexer)', 'Miri', 'Benchmark (parser)', 'Benchmark (transformer)', 'Benchmark (isolated_declarations)', 'Benchmark (semantic)', 'Benchmark (minifier)', 'Benchmark (codegen)', 'Benchmark (formatter)').
  • Apr 18, 5:33 AM EDT: Boshen added this pull request to the Graphite merge queue.
  • Apr 18, 5:46 AM EDT: Merged by the Graphite merge queue.

graphite-app bot pushed a commit that referenced this pull request Apr 18, 2025
…<Vec>` to `Vec` (#10472)

It wasn't possible for `extends` field to be `Some` containing an empty `Vec` because this is not legal code:

```ts
interface Foo extends {}
```

So use `Vec` for `extends` field, instead of `Option<Vec>`. This prevents creating an invalid AST, and simplifies code in various places.
@graphite-app graphite-app bot force-pushed the 04-17-refactor_ast_change_tsinterfacedeclaration_extends_from_option_vec_to_vec_ branch from f809e9e to eda2c5f Compare April 18, 2025 02:23
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 18, 2025
Boshen pushed a commit that referenced this pull request Apr 18, 2025
…<Vec>` to `Vec` (#10472)

It wasn't possible for `extends` field to be `Some` containing an empty `Vec` because this is not legal code:

```ts
interface Foo extends {}
```

So use `Vec` for `extends` field, instead of `Option<Vec>`. This prevents creating an invalid AST, and simplifies code in various places.
@Boshen Boshen force-pushed the 04-17-refactor_ast_change_tsinterfacedeclaration_extends_from_option_vec_to_vec_ branch from eda2c5f to 6eb7faa Compare April 18, 2025 09:22
graphite-app bot pushed a commit that referenced this pull request Apr 18, 2025
…<Vec>` to `Vec` (#10472)

It wasn't possible for `extends` field to be `Some` containing an empty `Vec` because this is not legal code:

```ts
interface Foo extends {}
```

So use `Vec` for `extends` field, instead of `Option<Vec>`. This prevents creating an invalid AST, and simplifies code in various places.
@graphite-app graphite-app bot force-pushed the 04-17-refactor_ast_change_tsinterfacedeclaration_extends_from_option_vec_to_vec_ branch from 882b517 to 1dbe1c1 Compare April 18, 2025 09:34
…<Vec>` to `Vec` (#10472)

It wasn't possible for `extends` field to be `Some` containing an empty `Vec` because this is not legal code:

```ts
interface Foo extends {}
```

So use `Vec` for `extends` field, instead of `Option<Vec>`. This prevents creating an invalid AST, and simplifies code in various places.
@graphite-app graphite-app bot force-pushed the 04-17-refactor_ast_change_tsinterfacedeclaration_extends_from_option_vec_to_vec_ branch from 1dbe1c1 to 7212803 Compare April 18, 2025 09:38
@graphite-app graphite-app bot merged commit 7212803 into main Apr 18, 2025
30 checks passed
@graphite-app graphite-app bot deleted the 04-17-refactor_ast_change_tsinterfacedeclaration_extends_from_option_vec_to_vec_ branch April 18, 2025 09:46
This was referenced Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-codegen Area - Code Generation A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants