Revert PR 2044 #2062

jakebailey · 2025-11-11T20:07:59Z

I guess to replace my big hammer with a smaller hammer.

#2044

Copilot

Pull Request Overview

This PR reverts PR 2044 and replaces it with a more targeted fix by removing an overly broad check for reparsed nodes in external module indicator detection. The change allows reparsed nodes (those synthesized during parsing) to be properly recognized as external module indicators, which improves CommonJS/ES module handling and type inference.

Key Changes

Removed the NodeFlagsReparsed check from isAnExternalModuleIndicatorNode() function
Updated 60+ test baselines showing improved compiler behavior and convergence with TypeScript reference implementation

Reviewed Changes

Copilot reviewed 82 out of 82 changed files in this pull request and generated no comments.

File	Description
`internal/ast/parseoptions.go`	Removed overly restrictive check that prevented reparsed nodes from being external module indicators
Test baseline files (`.types`, `.symbols`, `.errors.txt`, `.js`)	Updated to reflect improved module detection, better type inference, and proper ES module marker emission
Test diff files (`.diff`)	Many show reduction or elimination of differences from TypeScript reference behavior

jakebailey · 2025-11-11T21:55:49Z

Going to wait on the other PR since this exposes more problems that would have to be fixed... I am not super content with reverting it

jakebailey · 2025-11-11T22:07:58Z

Updated; there are still many regressions by removing this.

ahejlsberg · 2025-11-12T00:55:37Z

What is the pattern to the regressions you're seeing? I'm still surprised we'd consider excluding reparsed nodes from module determination, just seems wrong to do so.

jakebailey · 2025-11-12T00:58:45Z

I don't see why we would include them; the old algorithm did not walk into JSDoc nodes, and any other new stuff we stick in the tree to handle expandos or CJS exports and whatnot were not there either, right?

If the PR is filtered to diffs, you can see what's regressing: https://github.com/microsoft/typescript-go/pull/2062/files?file-filters%5B%5D=.go&file-filters%5B%5D=.diff

I can annotate the PR with a few examples.

jakebailey · 2025-11-12T00:59:37Z

To be clear, I agree that ideally we wouldn't do this, but for the most part the problem is that there are other bugs that this is sort of helping with. I don't think we need to keep it forever.

jakebailey · 2025-11-12T01:00:13Z

testdata/baselines/reference/submodule/conformance/assignmentToVoidZero1.js.diff

+"use strict";
+Object.defineProperty(exports, "__esModule", { value: true });
 +// #38552
+export var y = exports.x = void 0;
+ // #38552
 exports.y = exports.x = void 0;
 +export var x = 1;
 exports.x = 1;
 +export var y = 2;
 exports.y = 2;


This is a regression but you'll note that the emit is already wrong; we are emitting actual export var x when we shouldn't. These nodes existing in the tree means we are both detecting it as a module and then printing them back.

jakebailey · 2025-11-12T01:02:19Z

testdata/baselines/reference/submodule/compiler/modulePreserve4.errors.txt.diff

+       ~~~~~~~
+!!! error TS2339: Property 'default' does not exist on type '0'.


Strange error appearing here. g.js is exports.default = 0 which breaks for some reason.

jakebailey · 2025-11-12T01:04:16Z

testdata/baselines/reference/submodule/conformance/jsDeclarationsClassExtendsVisibility.js.diff

+"use strict";
+Object.defineProperty(exports, "__esModule", { value: true });


This file is becoming strict when it shouldn't be; strict is off, alwaysStrict is off, so the determination is only whether or not there's an external module indicator, which Strada must not have had.

jakebailey · 2025-11-12T01:05:50Z

testdata/baselines/reference/submodule/conformance/moduleExportAliasImported.types.diff

 ->import('./bug28014') : Promise<{ version: 1; default: { (): void; version: 1; }; }>
-+>import('./bug28014') : Promise<{ default: () => void; }>
+>import('./bug28014') : Promise<() => void>
 >'./bug28014' : "./bug28014"


The type here changes.

jakebailey · 2025-11-12T01:06:11Z

testdata/baselines/reference/submodule/conformance/untypedModuleImport_allowJs.errors.txt.diff

+-/a.ts(2,5): error TS2339: Property 'bar' does not exist on type 'typeof import("/node_modules/foo/index")'.
+-
+-
+-==== /a.ts (1 errors) ====
+-    import foo from "foo";
+-    foo.bar();
+-        ~~~
+-!!! error TS2339: Property 'bar' does not exist on type 'typeof import("/node_modules/foo/index")'.
+-    
+-==== /node_modules/foo/index.js (0 errors) ====
+-    exports.default = { bar() { return 0; } }
+-    
+<no content>


This error disappears.

ahejlsberg · 2025-11-12T01:48:52Z

I think a lot of these issues are caused by the re-parser adding a synthetic export modifier to KindCommonJSExport nodes. I don't know why that code is there, but it is causing the isAnExternalModuleIndicatorNode function to return true when it shouldn't. I think a first step should be to remove that export modifier.

ahejlsberg · 2025-11-12T02:14:54Z

Ok, it appears the only reason for that synthetic export modifier is so that the binder considers KindCommonJSExport nodes to declare exported symbols. When I remove the export modifier and change the code in declareModuleMember in the binder to always consider KindCommonJSExport nodes to be exports, there are no baseline changes. I will put up a PR to that effect, I think it is a better solution than the blunt instrument of ignoring all reparsed nodes.

Also, I agree there are still issues that need to be fixed in this JS module related code.

Revert PR 2044

3115bb5

jakebailey requested review from ahejlsberg and Copilot November 11, 2025 20:07

Copilot started reviewing on behalf of jakebailey November 11, 2025 20:08 View session

jakebailey mentioned this pull request Nov 11, 2025

Ignore reparsed nodes when determining external module indicator #2044

Merged

Copilot finished reviewing on behalf of jakebailey November 11, 2025 20:09

Copilot AI reviewed Nov 11, 2025

View reviewed changes

jakebailey mentioned this pull request Nov 11, 2025

Don't add export modifier to JSTypeAliasDeclaration from @callback #2063

Merged

ahejlsberg approved these changes Nov 11, 2025

View reviewed changes

Merge branch 'main' into jabaile/revert-2044

df70ead

jakebailey commented Nov 12, 2025

View reviewed changes

ahejlsberg mentioned this pull request Nov 12, 2025

Don't add export modifier to KindCommonJSExport reparsed nodes #2066

Merged

jakebailey closed this Nov 12, 2025

jakebailey deleted the jabaile/revert-2044 branch November 12, 2025 06:11

		+ ~~~~~~~
		+!!! error TS2339: Property 'default' does not exist on type '0'.

		+"use strict";
		+Object.defineProperty(exports, "__esModule", { value: true });

Revert PR 2044 #2062

Revert PR 2044 #2062

Uh oh!

Conversation

jakebailey commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

jakebailey commented Nov 11, 2025

Uh oh!

jakebailey commented Nov 11, 2025

Uh oh!

ahejlsberg commented Nov 12, 2025

Uh oh!

jakebailey commented Nov 12, 2025

Uh oh!

jakebailey commented Nov 12, 2025

Uh oh!

jakebailey Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

jakebailey Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

jakebailey Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

jakebailey Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

jakebailey Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

ahejlsberg commented Nov 12, 2025

Uh oh!

ahejlsberg commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jakebailey commented Nov 11, 2025 •

edited

Loading