-
Notifications
You must be signed in to change notification settings - Fork 640
Implement externalModuleIndicator #979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the externalModuleIndicator
and related metadata onto the SourceFile
during parse, removes the old sidecar structure, and wires metadata through the parser, host, and program layers.
- Updated parser API signatures to accept compiler options and file metadata, and set both on each
SourceFile
infinishSourceFile
. - Added
getExternalModuleIndicator
and helper functions to compute module indicators at parse time. - Refactored
CompilerHost
,Program
, and file loader to pass metadata through parsing, and removed legacy metadata maps.
Reviewed Changes
Copilot reviewed 121 out of 121 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
internal/parser/parser.go | Added options /metadata fields to Parser , set Metadata and ExternalModuleIndicator in finishSourceFile , and implemented getExternalModuleIndicator and helpers. |
internal/parser/parser_test.go | Updated benchmarks and fuzz tests to pass SourceFileAffectingCompilerOptions and metadata into ParseSourceFile . |
internal/compiler/host.go | Changed CompilerHost.GetSourceFile signature to accept metadata, and updated host implementation to panic if options are unset. |
internal/compiler/program.go | Wired metadata into NewProgram , UpdateProgram , added equalMetaData , and removed old metadata getters. |
internal/compiler/fileloader.go | Removed legacy metadata maps and parse tasks’ metadata, now loads metadata just before parsing each file. |
Comments suppressed due to low confidence (2)
internal/parser/parser.go:114
- ParseJSONText does not set the new
Metadata
orExternalModuleIndicator
fields on itsSourceFile
, so JSON inputs will lack module detection metadata. Consider invokingfinishSourceFile
or manually assigningresult.Metadata
andresult.ExternalModuleIndicator
before returning.
return result
internal/parser/parser.go:101
- [nitpick] The parameter name
metadata
shadows the parser's internalmetadata
field and may be confused with other metadata. Rename this parameter (e.g., tofileMetadata
) for clarity.
func ParseSourceFile(fileName string, path tspath.Path, sourceText string, options *core.SourceFileAffectingCompilerOptions, metadata *ast.SourceFileMetaData, jsdocParsingMode scanner.JSDocParsingMode) *ast.SourceFile {
I need to look in more detail later, but my immediate thought is that the cache key could have the pre-computed value of “force this file to be a module” instead of all the compiler options that might contribute to that value. Right now, differing |
That was my thought too, but the trouble is that the indicator is an AST node, because we need to be able to point diagnostics at it in some situations. But, for these new cases they're basically all just what used to be "true", so potentially that would work as a plain boolean passed in. I'm just not sure that we'll actually end up in the situation where that reuse would have happened, though. |
I think it’s pretty common for monorepos to have projects where these settings differ, but the actual parse doesn’t. It seems silly to not be able to share common declaration files between those projects, when the declaration files themselves are never affected by |
This PR is pretty broken now; I'm going to redo it, not eliminate the metadata sidecar and instead still pass stuff into the parser as an input, which should produce the smallest diff. We discussed a new way to do reparsing, however that's a much more difficult undertaking, not just because it means redoing things, but also because reparsing happening in the parser during the initial parse means that the Parser struct is still there and populated with stuff like the identifier list and so on (maybe other important state), so it needs a little bit of thinking. |
@@ -1068,7 +1070,7 @@ func (p *Project) Close() { | |||
|
|||
if p.program != nil { | |||
for _, sourceFile := range p.program.GetSourceFiles() { | |||
p.host.DocumentRegistry().ReleaseDocument(sourceFile, p.program.Options()) | |||
p.host.DocumentRegistry().ReleaseDocument(sourceFile, p.program.Options(), p.program.GetSourceFileMetaData(sourceFile.Path())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like how much jumping through hoops I have to do for this.
Honestly, it seems to me like the the entire set of parameters to ParseSourceFile
are the key, and that we could store a copy on the source files, rather than constantly asking the program for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, that would not work out for that differing parse plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correct that the key and the parse params are the same and storing a copy/pointer on the file makes sense. It's just that we may want to try to trim down the key/params later, with SourceFileMetadata
in particular containing some stuff we might want to pull off. But I think it's fine to make a type that is the cache key and store it on the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I make that change now? Would theoretically simplify things, though at that point I would find it strange that we'd also have the sidecar and plumb that access through everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I might see if I can follow up with a way to just pass in the module status, so let’s do that first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the change to make the above work is extensive, I spent a while trying to make it work and it exposed a bunch of other weirdness. I'll send a different PR for that.
The infamous
externalModuleIndicator
field requires access to info like the module setting of the closest package.json in order to work in some situations. This is normally "fine"; just make a sidecar that the Program holds onto. But unfortunately,externalModuleIndicator
is needed to know whether or not a given file needs to be reparsed to deal with top-level await, so needs to be there at parse time. This means that it's technically an input to parse, not something on the side.Add this back in again, passing the metadata into parse.
This isn't great, and we have a plan for how to improve this later, but it does fix a bunch.