Skip to content

Conversation

johnfav03
Copy link
Contributor

@johnfav03 johnfav03 commented Sep 9, 2025

Fixes #1500

This PR implements provideDocumentHighlights and the corresponding fourslash tests/functions; the implementation covers syntactic highlights fully, but is missing several cases in semantic highlights due to unimplemented cases in getReferencedSymbolsForNode. Some prevalent examples from the failing test cases include:

  • export logic in getReferencedSymbolsForSymbol
    • cases: getDocumentHighlightInExport, getDocumentHighlightInTypeExport`
  • getReferencesForStringLiteral in getReferencedSymbolsForNode
    • cases: getDocumentHighlightTemplateStrings, getOccurrencesStringLiterals, getOccurrencesStringLiteralTypes
  • constructor logic in getReferencesAtLocation
    • cases: getOccurencesClassExpressionConstructor, getOccurrencesConstructor, getOccurrencesConstructor2

@johnfav03
Copy link
Contributor Author

@johnfav03 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

@johnfav03 johnfav03 marked this pull request as ready for review September 17, 2025 21:04
Copy link
Contributor

@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 adds support for "Document highlights" functionality in the testing framework. Document highlights show related occurrences of symbols in a document when the user hovers or selects a symbol.

  • Adds a new baseline test type for documentHighlights test cases
  • Creates extensive test coverage for various TypeScript language constructs including classes, interfaces, keywords, modifiers, and control flow statements
  • Includes test cases for edge cases like inherited properties, parameter properties, and JSX elements

Reviewed Changes

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

Show a summary per file
File Description
testdata/baselines/reference/submodule/fourslash/findRenameLocations/renameDefaultImportDifferentName.baseline.jsonc.diff Diff file showing baseline changes for rename functionality
testdata/baselines/reference/submodule/fourslash/findRenameLocations/renameDefaultImportDifferentName.baseline.jsonc Baseline for testing rename operations on default imports with different names
testdata/baselines/reference/fourslash/findAllReferences/renameDefaultImportDifferentName.baseline.jsonc Baseline for find all references functionality on default imports
testdata/baselines/reference/fourslash/findAllReferences/findReferencesJSXTagName3.baseline.jsonc Baseline for finding references in JSX tag names
testdata/baselines/reference/fourslash/documentHighlights/*.baseline.jsonc 100+ baseline files covering document highlights for various TypeScript constructs including keywords, modifiers, classes, interfaces, control flow, and edge cases

@johnfav03 johnfav03 requested a review from iisaduan September 17, 2025 21:10
Copy link
Member

Choose a reason for hiding this comment

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

Is there a pattern among the failing tests, i.e. something they’re blocked on or a specific follow-up task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are giving an incorrect output due to a missing case in getReferencedSymbolsForNode() in findAllReferences - I'll update the PR description with cases that could be updated as a follow-up task

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

This is a great first PR 👏

Copy link
Member

@iisaduan iisaduan left a comment

Choose a reason for hiding this comment

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

Great work! Seems like you've quickly gotten a good hang of the codebase :D

Requesting changes because I noticed an incorrect port of some logic, and I also left comments on some refactors and code/comment clean up

@johnfav03 johnfav03 requested a review from iisaduan September 24, 2025 20:12
Co-authored-by: Isabel Duan <isabelduan@microsoft.com>
Copy link
Member

@iisaduan iisaduan left a comment

Choose a reason for hiding this comment

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

Looks good! Great work!

@johnfav03 johnfav03 requested a review from iisaduan September 24, 2025 21:20
Comment on lines 212 to 219
TestGetOccurrencesAsyncAwait3
TestGetOccurrencesClassExpressionConstructor
TestGetOccurrencesConstructor
TestGetOccurrencesConstructor2
TestGetOccurrencesIfElseBroken
TestGetOccurrencesOfAnonymousFunction
TestGetOccurrencesStringLiteralTypes
TestGetOccurrencesStringLiterals
Copy link
Member

Choose a reason for hiding this comment

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

Curious, what kinds of failures are these? Crashes? If they're crashes, I would definitely want to double check these because document highlight is something that happens automatically on cursor move, which means we could be spamming panics.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, these must be https://github.com/microsoft/typescript-go/pull/1699/files#r2360590494, which I didn't notice since it was attached to an old rev.

Copy link
Member

Choose a reason for hiding this comment

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

They are because of unimplemented cases in FAR, so the test fails when no highlights are returned (which is fine in editor)

Copy link
Member

Choose a reason for hiding this comment

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

String references and a lot of the keyword references aren't implemented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only two tests fail, and it's due to unexpected responses in fourslash.go from modules that arent documentHighlights. The tests are FindAllRefsForModule and TestJsdocTypedefTagServices, which return an unexpected response from FindAllReferences and QuickInfo respectively.

@jakebailey
Copy link
Member

I think this looks good, but in checker.ts it does look like sometimes things take more time than expected, though it's not clear if that's inherent or just because it was doing a hover or something at the same time:

image

I filtered the LSP output to Received response 'textDocument/documentHighlight and then moved my cursor / mouse around.

To be clear, it usually isn't this way, it's just sometimes randomly it takes 100ms, or is cancelled, which is also fine too.

It does make me wonder if there's an implementation of this that could avoid FAR and the checker entirely. Or a FAR modification to say "no checker use" that is mostly accurate. Especially because we are scoping to one file.

@johnfav03 johnfav03 added this pull request to the merge queue Sep 26, 2025
Merged via the queue into microsoft:main with commit b55c80c Sep 26, 2025
22 checks passed
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.

[lsp] Same variable syntax highlight over-highlighting variables
5 participants