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

Add special-case-highlighting parameter to parsing service #502

Conversation

sungshik
Copy link
Contributor

@sungshik sungshik commented Nov 5, 2024

Overview

Before this PR, the semantic highlighter applied a special case for syntax-in-syntax (sub)trees. However, this gave rise to unexpected behavior and caused #456.

After this PR:

To control whether or not the special case is applied for DSLs, this PR temporarily -- for the next few release cycles -- adds keyword parameter usesSpecialCaseHighlighting to constructor parsing of ADT LanguageServer.

  • Present release cycle: The default is true. Existing grammars that rely on the special case will have the expected highlighting. They could be updated (encouraged), and usesSpecialCaseHighlighting could be set to true (encouraged).
  • Future release cycle (short-term): The default is false. Existing grammars that rely on the special case may not have the expected highlighting. Either they should be updated (encouraged), or usesSpecialCaseHighlighting should be set to true (discouraged).
  • Future release cycle (mid-term): usesSpecialCaseHighlighting will be removed (i.e., the special case ceases to exist). Existing grammars that rely on the special case may not have the expected highlighting. They must be updated.

A few details

The updated constructor:

= parsing(Tree (str _input, loc _origin) parsingService
, bool usesSpecialCaseHighlighting = true)

The updated docs:

* Currently, `@category` tags are ignored in the following special case:
* if a parse tree has a `syntax` non-terminal node `n` with a category
(either declared as part of `n`, or inherited from an ancestors),
* and if `n` has a `syntax` non-terminal node `m` as a child,
* then the category of `n` is ignored in the subtree rooted at `m`
(regardless of whether a category is declared as part of `m`).
This special case is deprecated and will be removed in a future release. In
anticipation of the removal, users that rely on this special case for
syntax highlighting can update their grammars and explicitly opt-out of the
special case by passing `usesSpecialCaseHighlighting = false` when
registering the ((parsing)) service.

The updated Pico language server as an example:

set[LanguageService] picoLanguageServer() = {
parsing(parser(#start[Program]), usesSpecialCaseHighlighting = false),
documentSymbol(picoDocumentSymbolService),

@sungshik sungshik changed the title Semantic tokenizer fall2024 legacy highlighting Add legacy highlighting opt-out to parsing service Nov 5, 2024
Copy link
Contributor Author

@sungshik sungshik left a comment

Choose a reason for hiding this comment

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

Additional comments

@sungshik sungshik changed the title Add legacy highlighting opt-out to parsing service Add special-case-highlighting parameter to parsing service Nov 6, 2024
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

LGTM, except that we're missing the typescript side of the dedicated parser configuration.

@sungshik sungshik marked this pull request as ready for review November 6, 2024 15:09
Copy link

sonarcloud bot commented Nov 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
13.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@sungshik sungshik merged commit cb5a1a3 into semantic-tokenizer-fall2024 Nov 6, 2024
12 of 13 checks passed
@sungshik sungshik deleted the semantic-tokenizer-fall2024-legacy-highlighting branch November 6, 2024 15:17
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