Module specifier completions #2587
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements module specifier completions for the language server, fixing issue #2383 where path suggestions were missing in import/export statements. The implementation adds support for relative paths, non-relative modules, package.json exports/imports, typings, and triple-slash reference directives.
Changes:
- Added comprehensive module specifier completion logic (~1300 lines) including support for relative paths, node_modules, package.json exports/imports, path mappings, and triple-slash references
- Bypassed snapshot immutability to access real-time filesystem state for completion suggestions
- Updated test infrastructure to properly handle empty implicit first files in fourslash tests
- Fixed numerous test expectations and moved previously failing tests to passing status
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ls/string_completions.go | Core completion logic with support for module specifiers, path mappings, package.json, and triple-slash references (~1300 new lines) |
| internal/ls/languageservice.go | Added DirectoryExists, ReadDirectory, and GetDirectories methods for completions |
| internal/ls/host.go | Extended Host interface with filesystem query methods that bypass snapshot immutability |
| internal/project/snapshot.go | Added DirectoryExists, GetDirectories, and ReadDirectory methods delegating to underlying filesystem |
| internal/ls/completions.go | Removed file extension detail logic from completion item creation |
| internal/tspath/extension.go | Added GetPossibleOriginalInputExtensionForExtension utility function |
| internal/module/resolver.go | Moved extension utility to tspath package and improved directory separator handling |
| internal/testrunner/test_case_parser.go | Fixed logic to properly handle empty implicit first files in fourslash tests |
| internal/astnav/tokens.go | Fixed isValidPrecedingNode to check JSDoc for EndOfFile tokens |
| internal/fourslash/_scripts/*.txt | Updated failing/manual test lists reflecting newly passing tests |
| internal/fourslash/tests/manual/*.go | Updated test expectations and file paths to match new completion behavior |
| internal/fourslash/tests/gen/*.go | Deleted tests for path completion features that now work correctly |
| internal/fourslash/fourslash.go | Added t.Helper() calls and improved nil completion handling |
| // !!! HERE: only use directoryExists if we're resolving relative imports? Is it ok to do this if the above call to `addCompletionEntriesFromPaths` failed? | ||
| if !l.host.DirectoryExists(baseDirectory) { | ||
| return result |
There was a problem hiding this comment.
TODO comment indicates uncertainty about whether DirectoryExists should only be used for relative imports and whether it's OK to use it after a previous path completion attempt fails. This suggests the logic may have edge cases that need clarification. Consider documenting the intended behavior or removing the uncertainty.
| // Used for module specifier completions. | ||
| // ! Do not use for anything else, as this violates the principle that | ||
| // the host is a snapshot-in-time. | ||
| ReadDirectory(currentDir string, path string, extensions []string, excludes []string, includes []string, depth *int) []string | ||
| GetDirectories(path string) []string | ||
| DirectoryExists(path string) bool |
There was a problem hiding this comment.
The comment suggests these methods violate the principle that the host is a snapshot-in-time, bypassing immutability guarantees. While this is documented as intentional per the PR description, consider whether a more architecturally sound solution exists. For example, a separate "FileSystemQuery" interface could make this distinction clearer and prevent accidental misuse of these methods for other purposes where snapshot consistency is critical.
Fixes #2383
Note:
We bypass the snapshot's immutable view of files to be able to see what files and directories exist. This means there could be inconsistencies between the module specifier completions and the state of the current snapshot, but the worst that could happen here is extra or missing module specifiers in completions, so that seems fine.
PR TODO:
Future work:
ReadDirectoryas ported from Strada that uses regexes. We'll need Rewrite matchFiles to not use regexp2, remove other regexp uses #2407 to get rid of regex use. The usage ofReadDirectoryby module specifier completions probably doesn't need full fidelity with the original Strada code, the only nice to haves I can think of are the filtering of*.min.jsfiles and maybe of paths starting with..