Implement tryLoadInputFileForPath #1302
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the tryLoadInputFileForPath function to map output file paths back to input files for package self-names when outDir or declarationDir are used, addressing #1079. It also updates several baseline files to reflect the expected output changes and introduces new helper functions to compute common source directories and candidate output directories.
Reviewed Changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| testdata/baselines/reference/** | Baseline files for conformance, types, symbols, and errors have been updated to align with the self-name resolution changes. |
| internal/module/resolver.go | The tryLoadInputFileForPath function and several related helper methods have been added/modified to support output-to-input path mapping. |
Comments suppressed due to low confidence (1)
internal/module/resolver.go:773
- [nitpick] Given the multiple branching paths and the intricate logic for guessing the common source directory, additional compiler tests to cover edge cases for these calculations would help ensure stability as the project evolves.
var commonSourceDirGuesses []string
| @@ -748,10 +749,175 @@ func (r *resolutionState) loadModuleFromTargetExportOrImport(extensions extensio | |||
| } | |||
|
|
|||
| func (r *resolutionState) tryLoadInputFileForPath(finalPath string, entry string, packagePath string, isImports bool) *resolved { | |||
There was a problem hiding this comment.
[nitpick] The logic within tryLoadInputFileForPath is highly complex with deeply nested conditionals and loops (particularly lines 757–871). Consider extracting portions of this logic into smaller, well-named helper functions to improve readability and ease testing.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
That ship sailed long ago, pal
There was a problem hiding this comment.
I don’t think this complex logic needs to be ported at all—inferring the rootDir based on file inclusion is deprecated in 6.0, except in tsconfig-less compilations, which seems unlikely to happen with outDir / declarationDir anyway. I objected to the guessing logic when it went into Strada but was overruled; it seems even more unnecessary now.
There was a problem hiding this comment.
I guess I'm confused, because the reason I'm porting this is to make self-reference imports map properly, per the issue I'm fixing where if we don't do this we get output dts files in the program.
There was a problem hiding this comment.
Sorry, I wasn’t specific enough—this function needs to exist, but I think it should just error (which we won’t actually collect now, as you noted) and return a failed resolution if it can’t compute a rootDir instead of doing all the complicated guessing.
There was a problem hiding this comment.
I see; I'll give that a shot. This PR was me just asking copilot to port the whole thing which worked, so...
#1304 is my followup to actually report these diags, which I could flip to point at main first.
There was a problem hiding this comment.
Maybe I'm missing something, but how can I actually emit this error? If I stop the resolution here by adding an error and then return &resolved{}, then all other later resolution steps stop working and hundreds of tests break.
There was a problem hiding this comment.
If I just say to continue searching, then we end up with the same behavior again, where we end up with output files in the input set, and package import mappings stop working...
| @@ -5,21 +5,19 @@ | |||
| - | |||
| - | |||
| -!!! error TS2209: The project root is ambiguous, but is required to resolve export map entry '.' in file 'package.json'. Supply the `rootDir` compiler option to disambiguate. | |||
There was a problem hiding this comment.
This is missing because we currently aren't collecting resolution diagnostics (nor any program diagnostic).
Fixes #1079