Use correct ResolutionMode for files when resolving/loading files #267
Conversation
| @@ -7491,6 +7543,71 @@ func (node *SourceFile) LineMap() []core.TextPos { | |||
| return lineMap | |||
| } | |||
|
|
|||
| func (file *SourceFile) CollectDynamicImportOrRequireOrJsDocImportCalls() { | |||
There was a problem hiding this comment.
This method modifies the source file; I suspect we'll need some additional protection later if we start sharing these ASTs between programs so that two programs can't possibly race to be the first to update this info (note lineMap above). But I assume it doesn't matter at the moment.
There was a problem hiding this comment.
Though, I do wonder if we can come up with some method by which we don't do this here, but instead as a part of parsing or binding themselves...
There was a problem hiding this comment.
It looks to me like this could be done at parse time, but we are specifically just setting the NodeFlagsPossiblyContainsDynamicImport flag in parseImportType(), so I guess I'd presume there's some important reason to defer it. Maybe to do with incremental parsing (which might not be valid anymore)?
There was a problem hiding this comment.
It looks like this is already being handled in Parser.
internal/compiler/fileloader.go
Outdated
| ```ts | ||
| // tsc foo.mts --module nodenext |
There was a problem hiding this comment.
I don't know how much of this comment is relevant now, but this format isn't going to show correctly in gopls. I think 4-space indentation works for code snippets. JSDoc-style @param tags also don't do anything.
| } | ||
|
|
||
| func getDefaultResolutionModeForFile(item *sourceFileMetadata, options *core.CompilerOptions) core.ResolutionMode { | ||
| if importSyntaxAffectsModuleResolution(options) { |
There was a problem hiding this comment.
Do these functions belong in fileloader.go?
There was a problem hiding this comment.
Yeah, I'm not sure. I thought about making all of these methods on core.CompilerOptions but I'm not sure if that's the right place either
There was a problem hiding this comment.
If they accept sourceFileMetadata, that doesn't sounds like it'd be a method on the compiler options; I feel like this is definitely a feature of the Program and not CompilerOptions.
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- internal/ast/ast.go: Evaluated as low risk
Comments suppressed due to low confidence (4)
internal/compiler/fileloader.go:360
- The function resolveTripleslashPathReference was changed to remove the receiver *fileLoader, but the function name is still used with the receiver in the code. This might cause confusion or errors if the function is called with the receiver elsewhere in the code.
func resolveTripleslashPathReference(moduleName string, containingFile string) string {
internal/compiler/fileloader.go:390
- The getModeForUsageLocation function might not always return the correct mode if the usage node is not correctly identified. Ensure that the usage node is correctly identified before calling this function.
mode := getModeForUsageLocation(item, resolution.node, optionsForFile)
internal/ast/utilities.go:1150
- The new behavior introduced by IsRequireCall should be covered by tests. Please add relevant test cases to ensure the function works as expected.
func IsRequireCall(node *Node, requireStringLiteralLikeArgument bool) bool {
internal/ast/utilities.go:1211
- The new behavior introduced by IsExclusivelyTypeOnlyImportOrExport should be covered by tests. Please add relevant test cases to ensure the function works as expected.
func IsExclusivelyTypeOnlyImportOrExport(decl *Node) bool {
| @@ -1196,6 +1208,20 @@ func IsImportOrExportSpecifier(node *Node) bool { | |||
| return IsImportSpecifier(node) || IsExportSpecifier(node) | |||
| } | |||
|
|
|||
| func IsExclusivelyTypeOnlyImportOrExport(decl *Node) bool { | |||
There was a problem hiding this comment.
The function IsExclusivelyTypeOnlyImportOrExport is missing a case for KindImportEqualsDeclaration. Please add this case to ensure all relevant node kinds are covered.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| @@ -176,19 +183,21 @@ type parseTask struct { | |||
| func (t *parseTask) start(loader *fileLoader) { | |||
| loader.wg.Run(func() { | |||
| file := loader.parseSourceFile(t.normalizedFilePath) | |||
| metadata := loader.buildFileMetadata(file) | |||
There was a problem hiding this comment.
The emitter needs to know the implied node format to emit the correct output for .ts files. In Strada, this was available via SourceFile.impliedNodeFormat and was passed as an option to createSourceFile. Is there any way to access this information from the emitter?
Pass the correct ResolutionMode to the resolver when resolving imports/type directives, and for resolvedModules on the file.