Skip to content
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

Use flight experiment to allow skipping semantic token for html syntax #9372

Closed
wants to merge 5 commits into from

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Oct 6, 2023

Summary

Currently the LSP semantic tokens call identifies html syntax rather than purely relying on TextMate.
This PR helps use flight experiments that would help understand whether or not we would regress performance when using only TextMate for html syntax.

  • Skips adding semantic token for html syntax markups
  • Allows TextMate Name/Value capability

Implementation Detail:

  • Uses IsCachedFlightEnabled to check for flight experiment
  • Uses the flight experiment to add SkipHtmlSyntaxSemanticTokens feature flag
  • Uses the feature flag to skip tokens for html syntax by adding a property for LanguageServerFeatureOptions
  • No need for user toggle option (the value decided based on the existence of flight experiment, so it can only get enabled via flight experiments)

TODO (next)

  • Check if we could get away with just using IVsExperimentationService.IsCachedFlightEnabled w/out needing a feature flag
  • Update unit tests
  •  Check for TextMate behavioral regressions (Refer to comment below)
  • Check if we'd still get a repro of Enum member coloration wrong in Razor files #5614
  • Inspect failing integration test

TODO (After reviewing the flight experiment) we may decide to

  • Remove markups from legend

Fixes #9256

- [x] Skip adding semantic token for markups
- [x] Allow TextMate Name/Value capability

TODO
- [ ] Remove markups from legend
- [ ] Add feature flag for AB Flight Experiment
STEP 2:

- Using IsCachedFlightEnabled to check for flight experiment
- Adds SkipHtmlSyntaxSemanticTokens feature flag
- Uses the feature flag to skip tokens for html syntax
- Added for LanguageServerFeatureOptions
- Defaults to false
- No need for user toggle option
- Only can get enabled via flight experiments

TODO
- [ ] Run instance see if the TextMate issues are still present
@maryamariyan maryamariyan self-assigned this Oct 6, 2023
Reveals which semantic token test results would be affected
when we start skipping semantic token generation
for html syntax for razor documents
@maryamariyan
Copy link
Member Author

maryamariyan commented Oct 6, 2023

Comparisons (WIP - Will add more screenshots later)

There would be some pretty noticable differences between having the html syntax skipping tokens (only TextMate) vs not:

(TextMate on right hand side):
image

Few examples from the above screenshot:

  • TextMate:
    image

  • Tokens for html syntax:
    image

  • TextMate:
    image

  • Tokens for html syntax
    image

@ToddGrun
Copy link
Contributor

ToddGrun commented Oct 7, 2023

I assume these differences aren't easily captured in the textmate grammar. If not, it looks like textmate classification isn't an option.

Given that, is there something that can be done to prevent the textmate grammar from contributing to colorization?

@maryamariyan maryamariyan deleted the dev/maryamariyan/flight-skiphtmlsyntax branch April 1, 2024 21:43
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.

Investigate if the semantic tokens returned by razor for html can be safely removed
2 participants