Add completion helpers and port more completion fourslash tests#1290
Merged
Conversation
gabritto
commented
Jun 25, 2025
| @@ -4025,22 +4087,6 @@ func (l *LanguageService) createLSPCompletionItem( | |||
| Range: *replacementSpan, | |||
| }, | |||
| } | |||
| } else { | |||
Member
Author
There was a problem hiding this comment.
This moved to setItemDefaults.
gabritto
changed the title
jakebailey
reviewed
Jun 26, 2025
| @@ -6452,7 +6452,7 @@ func extractPragmas(commentRange ast.CommentRange, text string) []ast.Pragma { | |||
| } | |||
| } | |||
| if commentRange.Kind == ast.KindMultiLineCommentTrivia { | |||
| text = text[:len(text)-2] | |||
| text = strings.TrimSuffix(text, "*/") | |||
Member
There was a problem hiding this comment.
When would the old code have failed?
Member
Author
There was a problem hiding this comment.
If you have an unclosed multiline comment, like /* ..., then it is wrong to unconditionally drop the last two characters, because they won't be */, and if the comment is empty, e.g. /*, this later causes an out of bounds panic in the call to skipTo, since we start at pos := 2.
jakebailey
approved these changes
Jun 26, 2025
Copilot AI
added a commit
that referenced
this pull request
May 18, 2026
Add a `packageInfo.Exists()` guard before accessing `packageInfo.Contents` at line 1132 in `loadModuleFromSpecificNodeModulesDirectory`. Without this check, a nil `Contents` field (possible under concurrent resolution races where `LoadOrStore` returns a stale cache entry) causes a fatal nil-pointer dereference. This matches the defensive pattern already used at other `Contents` access sites in the same function (lines 1078, 1101, 1124). Fixes #1290 Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Copilot AI
added a commit
that referenced
this pull request
May 18, 2026
Change isJSDocLikeText in the printer to use >= 5 instead of > 5 for the length check, matching TypeScript's behavior of preserving empty JSDoc comments like /***/ in declaration output. Fixes #1290 Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/814ee36f-78e4-4428-a2eb-b12e40bd2a45 Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR mainly adds completion helpers like
completionGlobalsandcompletionGlobalsPlusto Corsa. Those helpers are ininternalfourslash/tests/util_test.go(and copied tointernalfourslash/tests/gen/util_test.go).It also ports Strada tests that use those helpers.
Another main change is that, to make the
completionGlobalshelper work, this PR also makes a change to completion such that whenever the old code would set anoptionalReplacementSpan, we set theeditRangeinCompletionList.ItemDefaults. Before, we were using the optional replacement span to set aTextEditon each completion item, so we couldn't reliably usecompletionGlobalsbecause depending on the completion position, each global completion item would have aTextEdit(or not).There are, like always, a number of bug fixes needed to get the tests to at least not crash. This PR ports 68 new completion tests, 38 of which are failing tests added to the failing test list, and removes 13 tests from the failing list. A lot of the new failing tests are JS/JSDoc stuff that I think isn't properly implemented yet.