Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jul 31, 2025

  • Initial plan to implement LocationLink support in go-to-definition
  • Implement LocationLink support in LanguageService.ProvideDefinition
  • Add default enablement of LinkSupport when client capabilities are not specified
  • Support both LocationLink and Location responses based on client capabilities
  • Add comprehensive fourslash test coverage for LocationLink end-to-end functionality
  • Address PR feedback: Remove unnecessary tests and fix fourslash normalization
  • Address PR feedback: Enable default LinkSupport and use core.Map
  • Address PR feedback: Rename function, use ptrTo(true), fix capabilities copying
  • Merge from main branch and resolve conflicts
  • Regenerate and accept all baselines after merge

Key Implementation Details

LocationLink Support:

  • Implemented createDefinitionResponse method that checks client capabilities
  • Added createLocationLinksFromDeclarations method for rich LocationLink responses
  • Enhanced ProvideDefinition to accept client capabilities parameter
  • Added definitionCapabilities helper with default LinkSupport=true

Default Enablement:
LocationLink support is now enabled by default when clients don't specify definition capabilities, ensuring users get the enhanced experience without requiring explicit client configuration.

Backward Compatibility:
Maintains full backward compatibility by falling back to traditional Location responses when clients explicitly set linkSupport: false.

The implementation provides richer navigation information including origin selection ranges, full target declaration ranges, and precise target selection ranges as specified in LSP 3.14.0+.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 2 commits July 31, 2025 08:06
- Add client capability checking for DefinitionClientCapabilities.LinkSupport
- Modify ProvideDefinition to accept client capabilities parameter
- Create createDefinitionResponse function to decide between LocationLink[] and Location[]
- Implement createLocationLinksFromDeclarations with proper origin selection range
- Update fourslash test infrastructure to handle DefinitionLinks gracefully
- Add helper function getDefinitionClientCapabilities in server
- Ensure backward compatibility with existing Location[] responses

Co-authored-by: DanielRosenwasser <[email protected]>
- Created TestLocationLinkEndToEnd to validate full functionality
- Test uses client capabilities with LinkSupport enabled
- Verifies that cross-file go-to-definition works with LocationLink format
- Demonstrates proper integration between server, client capabilities, and definition provider

Co-authored-by: DanielRosenwasser <[email protected]>
@Copilot Copilot AI changed the title [WIP] Support LocationLink in go-to-definition Support LocationLink in go-to-definition Jul 31, 2025
@Copilot Copilot AI requested a review from DanielRosenwasser July 31, 2025 08:12
Copilot finished work on behalf of DanielRosenwasser July 31, 2025 08:12
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of this test, we have coverage elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the test as requested. Commit 694ad3b.

Copy link
Member

Choose a reason for hiding this comment

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

Get rid of this test, it's not a language service test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the compiler test as requested. Commit 694ad3b.

} else if result.DefinitionLinks != nil {
t.Fatalf("Unexpected definition response type at marker '%s': %T", *f.lastKnownMarkerName, result.DefinitionLinks)
// Convert DefinitionLinks to Locations for baseline comparison
resultAsLocations = make([]lsproto.Location, len(*result.DefinitionLinks))
Copy link
Member

Choose a reason for hiding this comment

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

The right thing is not to normalize to Locations. Instead, what the original code did was roughly checking if there was a single result, and if so, set the appropriate additionalSpan for getBaselineContentForFile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the normalization logic to use additionalSpan for single DefinitionLink results instead of converting to Locations. Now properly sets the origin selection range as additionalSpan for baseline comparison. Commit 694ad3b.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but switch this to use core.Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to use core.Map instead of manual loop. Commit 0abc02d.

@DanielRosenwasser
Copy link
Member

@copilot address feedback.

…lization

- Remove internal/fourslash/tests/gen/locationlink_endtoend_test.go (coverage exists elsewhere)
- Remove testdata/tests/cases/compiler/gotoDefinitionLocationLink.ts (not a language service test)
- Fix fourslash.go to properly handle DefinitionLinks by setting additionalSpan instead of normalizing to Locations
- When single DefinitionLink with origin selection range, use it as additionalSpan for baseline comparison

Co-authored-by: DanielRosenwasser <[email protected]>
Copy link
Contributor Author

Copilot AI commented Aug 4, 2025

@copilot address feedback.

All feedback addressed in commit 694ad3b.

@Copilot Copilot AI requested a review from DanielRosenwasser August 4, 2025 18:22
Copilot finished work on behalf of DanielRosenwasser August 4, 2025 18:22
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Good foundation, but you need to enable the default capabilities for definition requests and update the baselines.

} else if result.DefinitionLinks != nil {
t.Fatalf("Unexpected definition response type at marker '%s': %T", *f.lastKnownMarkerName, result.DefinitionLinks)
// Convert DefinitionLinks to Locations for baseline comparison
resultAsLocations = make([]lsproto.Location, len(*result.DefinitionLinks))
Copy link
Member

Choose a reason for hiding this comment

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

Okay, but switch this to use core.Map

@DanielRosenwasser
Copy link
Member

@copilot address feedback

Copy link
Contributor Author

Copilot AI commented Aug 4, 2025

@copilot address feedback

All feedback addressed in commit 0abc02d.

Copy link
Contributor Author

Copilot AI commented Aug 4, 2025

Good foundation, but you need to enable the default capabilities for definition requests and update the baselines.

Default LinkSupport capabilities enabled and baselines updated. Commit 0abc02d.

@Copilot Copilot AI changed the title Support LocationLink in go-to-definition Support LocationLink in go-to-definition with default enablement Aug 4, 2025
@Copilot Copilot AI requested a review from DanielRosenwasser August 4, 2025 20:11
Copilot finished work on behalf of DanielRosenwasser August 4, 2025 20:11
return params.Capabilities.TextDocument.Completion
}

func getDefinitionClientCapabilities(params *lsproto.InitializeParams) *lsproto.DefinitionClientCapabilities {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be in getCapabilitiesWithDefaults, but maybe it's good to be targeted. Don't change it for now, but wondering thoughts from others on the team (e.g. @jakebailey @gabritto)

}
}

capabilities := params.Capabilities.TextDocument.Definition
Copy link
Member

Choose a reason for hiding this comment

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

call this definitionCapabilities because it looks like it's checked above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to definitionCapabilities. Commit a8ba72c.

Comment on lines 894 to 904
linkSupport := true
return &lsproto.DefinitionClientCapabilities{
LinkSupport: &linkSupport,
}
}

// If capabilities exist but LinkSupport is not specified, default to true
if capabilities.LinkSupport == nil {
linkSupport := true
return &lsproto.DefinitionClientCapabilities{
LinkSupport: &linkSupport,
Copy link
Member

Choose a reason for hiding this comment

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

We have stuff like ptrTrue to use instead of &linkSupport

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to use ptrTo(true) instead of &linkSupport. Commit a8ba72c.

// If capabilities exist but LinkSupport is not specified, default to true
if capabilities.LinkSupport == nil {
linkSupport := true
return &lsproto.DefinitionClientCapabilities{
Copy link
Member

Choose a reason for hiding this comment

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

Here the capabilities are set, but without link support. That means everything else is being dropped. The object has to be copied and returned with an overridden field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed capabilities copying to preserve existing fields. Now using result := *capabilities; result.LinkSupport = ptrTo(true); return &result instead of creating a new object. Commit a8ba72c.

return lsproto.LocationOrLocationsOrDefinitionLinksOrNull{DefinitionLinks: &links}
}

func (l *LanguageService) createLocationsFromDeclarations(declarations []*ast.Node) lsproto.DefinitionResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Who is using this function anymore? Can it be used in the new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The createLocationsFromDeclarations function is still being used as the fallback when LinkSupport is false in the createDefinitionResponse method. It provides backwards compatibility for clients that don't support LocationLink.

@DanielRosenwasser
Copy link
Member

@copilot resolve feedback

Copy link
Contributor Author

Copilot AI commented Aug 4, 2025

@copilot resolve feedback

All feedback addressed in commit a8ba72c:

  • Renamed function to definitionCapabilities
  • Used ptrTo(true) instead of manual boolean pointer creation
  • Fixed capabilities copying to preserve existing fields
  • Confirmed createLocationsFromDeclarations is still used for backwards compatibility

Copilot finished work on behalf of DanielRosenwasser August 4, 2025 21:17
@Copilot Copilot AI requested a review from DanielRosenwasser August 4, 2025 21:17
@DanielRosenwasser
Copy link
Member

@copilot merge from main, regenerate/accept all baselines

Copy link
Contributor Author

Copilot AI commented Sep 12, 2025

@copilot merge from main, regenerate/accept all baselines

Merged from main and regenerated/accepted all baselines. Commit 86784bf.

Copilot finished work on behalf of DanielRosenwasser September 12, 2025 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants