Fix code that should have used IsRequireCall #1195
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR corrects module resolution by using IsRequireCall instead of IsVariableDeclarationInitializedToRequire, ensuring that all require(...) calls are properly recognized rather than only those in variable declarations.
- Replace
IsVariableDeclarationInitializedToRequirechecks withIsRequireCallin utilities, binder, and checker. - Refine
resolveExternalModuleto detect require calls via initializer on variable declarations. - Update baselines to reflect the new symbol and error expectations for JSON and module imports.
Reviewed Changes
Copilot reviewed 92 out of 92 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ls/utilities.go | Use IsRequireCall to detect require calls in JS files |
| internal/checker/checker.go | Swap out variable-init checks for IsRequireCall and IsInJSFile |
| internal/binder/nameresolver.go | Refactor require detection to use IsRequireCall |
| internal/binder/binder.go | Mark CommonJS modules on raw require calls, not var inits |
| testdata/.../requireOfJsonFileInJsFile.* | Update baselines for JSON require behavior |
| testdata/.../modulePreserve4.* | Update baselines for module preserve output |
| testdata/.../jsExportMemberMerged...* | Refresh symbol and error baselines for exports |
| testdata/.../importNonExportedMember12.* | Reflect updated import baselines for require calls |
| ->require("./something").o : Symbol(o, Decl(something.ts, 0, 12)) | ||
| ->require : Symbol(require) | ||
| >require("./something").o : Symbol(o, Decl(something.ts, 0, 12)) | ||
| >require : Symbol(require) | ||
| ->"./something" : Symbol("something", Decl(something.ts, 0, 0)) |
There was a problem hiding this comment.
Still missing something here, as this is mad about the actual string not having resolved to the other module (which would break something like go to def).
There was a problem hiding this comment.
It's probably still missing because getSymbolAtLocation doesn't support js declaration lookups yet (since it's bound entirely differently), or something similar.
.../reference/submoduleAccepted/conformance/emitModuleCommonJS(module=commonjs).errors.txt.diff
Outdated
Show resolved
Hide resolved
| -/main3.cjs(17,8): error TS1293: ESM syntax is not allowed in a CommonJS module when 'module' is set to 'preserve'. | ||
| - | ||
| - | ||
| +/main3.cjs(1,13): error TS1293: ESM syntax is not allowed in a CommonJS module when 'module' is set to 'preserve'. |
There was a problem hiding this comment.
This is probably a piece of the checker that hasn't been updated to ignore reparsed exports.
| ->require("./something").o : Symbol(o, Decl(something.ts, 0, 12)) | ||
| ->require : Symbol(require) | ||
| >require("./something").o : Symbol(o, Decl(something.ts, 0, 12)) | ||
| >require : Symbol(require) | ||
| ->"./something" : Symbol("something", Decl(something.ts, 0, 0)) |
There was a problem hiding this comment.
It's probably still missing because getSymbolAtLocation doesn't support js declaration lookups yet (since it's bound entirely differently), or something similar.
IsVariableDeclarationInitializedToRequireis not an equivalent replacement forIsRequireCall. They have differing behavior.Noticed some of this in looking at new errors exposed by #1189.
Best to filter this PR to just diffs.