Skip to content

fix(semantic): incorrect SymbolFlags of TSModuleDeclaration#10350

Merged
graphite-app[bot] merged 1 commit intomainfrom
04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration
Apr 11, 2025
Merged

fix(semantic): incorrect SymbolFlags of TSModuleDeclaration#10350
graphite-app[bot] merged 1 commit intomainfrom
04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration

Conversation

@Dunqing
Copy link
Copy Markdown
Member

@Dunqing Dunqing commented Apr 10, 2025

Based on TypeScript's implementation to correct SymbolFlags of TSModuleDeclaration.

The SymbolFlags::NamespaceModule and SymbolFlags::ValueModule have a significant difference,

NamespaceModule: can only be referenced as a type.
ValueModule: can only be referenced as a value.

Let's take an example to see:

namespace NamespaceModule {
  export type A = string
}
namespace ValueModule {
  export const A = 0;
}

The following code is the JS output of the above example.

"use strict";
var ValueModule;
(function (ValueModule) {
    ValueModule.A = 0;
})(ValueModule || (ValueModule = {}));

Only ValueModule will be preserved and transformed.

That means whether a TSModuleDeclaration needs to be transformed or removed directly, we can determine by its SymbolFlags. We can use it to simplify the current TSModuleDeclaration transformation later.

Copy link
Copy Markdown
Member Author

Dunqing commented Apr 10, 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.

@github-actions github-actions bot added A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-bug Category - Bug labels Apr 10, 2025
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 10, 2025

CodSpeed Instrumentation Performance Report

Merging #10350 will degrade performances by 3.47%

Comparing 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration (dd2aef0) with main (4a6bb21)

Summary

❌ 1 regressions
✅ 35 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
formatter[antd.js] 7.8 ms 8.1 ms -3.47%

@Dunqing Dunqing force-pushed the 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration branch from b1d454d to 2625fff Compare April 10, 2025 11:36
@Dunqing Dunqing force-pushed the 04-10-refactor_transformer_typescript_simplify_typescriptannotations_has_value_reference_method branch from 8790921 to e82bd5c Compare April 10, 2025 11:36
@Dunqing Dunqing force-pushed the 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration branch from 2625fff to fc8acf4 Compare April 11, 2025 01:31
@github-actions github-actions bot added the A-ast Area - AST label Apr 11, 2025
@Dunqing Dunqing force-pushed the 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration branch from fc8acf4 to c67d431 Compare April 11, 2025 01:33
@Dunqing Dunqing changed the base branch from 04-10-refactor_transformer_typescript_simplify_typescriptannotations_has_value_reference_method to graphite-base/10350 April 11, 2025 01:54
@Dunqing Dunqing force-pushed the graphite-base/10350 branch from e82bd5c to 0a2fed4 Compare April 11, 2025 01:54
@Dunqing Dunqing force-pushed the 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration branch from c67d431 to 5ef7f81 Compare April 11, 2025 01:54
@Dunqing Dunqing changed the base branch from graphite-base/10350 to 04-10-refactor_semantic_simplify_bind_logic_for_tsmoduledeclaration April 11, 2025 01:54
@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented Apr 11, 2025

Wow, this can bring a 1% performance improvement in Semantic! Probably comes from removing namespace_stack.

image

@Dunqing Dunqing marked this pull request as ready for review April 11, 2025 08:17
@Dunqing Dunqing requested a review from Boshen as a code owner April 11, 2025 08:17
@Dunqing Dunqing changed the base branch from 04-10-refactor_semantic_simplify_bind_logic_for_tsmoduledeclaration to graphite-base/10350 April 11, 2025 09:45
@Dunqing Dunqing force-pushed the 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration branch from 5ef7f81 to ec0539d Compare April 11, 2025 09:47
@Dunqing Dunqing force-pushed the graphite-base/10350 branch from 0a2fed4 to d4bcd57 Compare April 11, 2025 09:47
@Dunqing Dunqing changed the base branch from graphite-base/10350 to 04-10-refactor_semantic_simplify_bind_logic_for_tsmoduledeclaration April 11, 2025 09:47
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Apr 11, 2025
@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app bot commented Apr 11, 2025

Merge activity

Based on TypeScript's [implementation](https://github.com/microsoft/TypeScript/blob/15392346d05045742e653eab5c87538ff2a3c863/src/compiler/binder.ts#L2384-L2393) to correct `SymbolFlags` of `TSModuleDeclaration`.

The `SymbolFlags::NamespaceModule` and `SymbolFlags::ValueModule` have a significant difference,

`NamespaceModule`: can only be referenced as a type.
`ValueModule`: can only be referenced as a value.

Let's take an example to see:

```ts
namespace NamespaceModule {
  export type A = string
}
namespace ValueModule {
  export const A = 0;
}
```

The following code is the JS output of the above example.

```js
"use strict";
var ValueModule;
(function (ValueModule) {
    ValueModule.A = 0;
})(ValueModule || (ValueModule = {}));
```

Only `ValueModule` will be preserved and transformed.

That means whether a `TSModuleDeclaration`  needs to be transformed or removed directly, we can determine by its `SymbolFlags`. We can use it to simplify the current `TSModuleDeclaration` transformation later.
@graphite-app graphite-app bot force-pushed the 04-10-refactor_semantic_simplify_bind_logic_for_tsmoduledeclaration branch from d4bcd57 to c37f048 Compare April 11, 2025 09:58
@graphite-app graphite-app bot force-pushed the 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration branch from ec0539d to dd2aef0 Compare April 11, 2025 09:59
Base automatically changed from 04-10-refactor_semantic_simplify_bind_logic_for_tsmoduledeclaration to main April 11, 2025 10:11
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 11, 2025
@graphite-app graphite-app bot merged commit dd2aef0 into main Apr 11, 2025
27 checks passed
@graphite-app graphite-app bot deleted the 04-10-fix_semantic_incorrect_symbolflags_of_tsmoduledeclaration branch April 11, 2025 10:12
graphite-app bot pushed a commit that referenced this pull request Apr 12, 2025
…Declaration` if is a `NamespaceModule` (#10366)

Benefits from #10350. `NamespaceModule` means this `TSModuleDeclaration` is a type-only declaration, so that we can get rid of it early without going through its block.
Dunqing added a commit that referenced this pull request Apr 14, 2025
…Declaration` if is a `NamespaceModule` (#10366)

Benefits from #10350. `NamespaceModule` means this `TSModuleDeclaration` is a type-only declaration, so that we can get rid of it early without going through its block.
graphite-app bot pushed a commit that referenced this pull request Apr 14, 2025
…Declaration` if is a `NamespaceModule` (#10366)

Benefits from #10350. `NamespaceModule` means this `TSModuleDeclaration` is a type-only declaration, so that we can get rid of it early without going through its block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant