Skip to content

Don't ignore completions at source file locations #61909

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

When investigating Corsa's completions, I learned that it can't find completions for this case:
https://github.com/microsoft/typescript-go/blob/6e03d625d15ccaf727cb9a44b455eefb16f3d450/internal/fourslash/tests/gen/asOperatorCompletion_test.go#L13

In Corsa, there is no EndOfFileToken included in the SourceFile (yet? see microsoft/typescript-go#1257 ) and thus that token can't be found as the location (computed by getTouchingPropertyName(sourceFile, position)). So the location becomes simply the SourceFile itself.

But then, it turns out those completions stop working in Strada as soon as we include an extra newline at the end of the file, or if we add a comment after the as. In such situations, the found location becomes the SourceFile. I can't find a reason why the SourceFile locations would be deliberately ignored by this whole shouldIncludeSymbol function. Only 2 paths in it are actually considering location at all. I have a feeling, this condition might have been put in place to avoid checking location.parent as that's undefined for SourceFiles.

@Copilot Copilot AI review requested due to automatic review settings June 22, 2025 09:32
@github-project-automation github-project-automation bot moved this to Not started in PR Backlog Jun 22, 2025
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 22, 2025
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

1 similar comment
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Copy link

@Copilot Copilot AI left a 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 revises the logic for completions handling at source file locations and updates test baselines accordingly.

  • Adjusts the condition in completions.ts to account for export assignments without prematurely excluding SourceFile locations.
  • Updates several fourslash and baseline test files to reflect the new behavior.
  • Adds a null-check in utilities.ts to improve robustness when accessing a node’s parent.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/cases/fourslash/asOperatorCompletion3.ts Adds a test case checking completions when a comment follows the as operator.
tests/cases/fourslash/asOperatorCompletion2.ts Updates the test case to verify completions without extra newline/comment.
tests/baselines/reference/tsserver/completions/in-project-reference-setup-with-existing-import.js Modifies baseline output by replacing a class entry with an alias entry for MyClass.
tests/baselines/reference/tsserver/completions/in-project-reference-setup-with-existing-import-without-includeCompletionsForModuleExports.js Aligns baseline output by substituting the removed class entry with an alias entry for MyClass.
src/services/utilities.ts Introduces an explicit null-check for node.parent in internal import equals declaration.
src/services/completions.ts Refactors the export assignment check to handle SourceFile locations by verifying location.parent exists.
Comments suppressed due to low confidence (2)

src/services/completions.ts:2742

  • The updated condition now safely checks for location.parent before evaluating export assignment. Ensure that this change correctly handles cases where location is a SourceFile by design.
        if (location.parent && isExportAssignment(location.parent)) {

src/services/utilities.ts:523

  • The added null check for node.parent improves robustness; consider verifying that similar patterns elsewhere in the codebase adhere to this safe-check approach.
    if (!node.parent) {

// export = /**/ here we want to get all meanings, so any symbol is ok
if (isExportAssignment(location.parent)) {
return true;
// export = /**/ here we want to get all meanings, so any symbol is ok
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this is simply outdented because I removed the wrapping if statement. Within the outdented codepaths I had to guard 2 places against missing location.parent and that's it.

cc @gabritto as you have a fresh context on all of this and related topics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

2 participants