eclipse-langium / langium

Next-gen language engineering / DSL framework
https://langium.org/
MIT License
754 stars 68 forks source link

Abstract semantic highlight provider dependent on AST ordering #420

Closed msujew closed 2 years ago

msujew commented 2 years ago

Langium version: next Package name: langium

Steps To Reproduce

  1. Create a parser rule which contains two different arrays, which can be mixed
  2. Create a semantic highlighting rule for both of the elements contained in the array

The current behavior

The semantic highlight provider breaks in the case outlined above. This is because the streamAllContents function does not return AST nodes in their order of appearance. However, the SemanticTokenBuilder expects them this way.

The expected behavior

The semantic highlight provider should not care about the order in which tokens where added and order them itself.

msujew commented 2 years ago

@spoenemann I would like to have your opinion on this. Fixing this could be accomplished using two approaches:

  1. Store all token results in the AbstractSemanticTokenBuilder and sort them just before building the LSP result.
  2. Let streamAllContents return a sorted stream of elements. (Perhaps with an additional argument?)

I believe having a stable return order of AST nodes based on order of appearance makes sense as the default behavior for streamAllContents, and shouldn't even be that expensive from a computational perspective. Node uses quick-sort, so it should almost always pick the perfect pivot right from the start, given that the AST node streamContents method returns an almost sorted stream.

spoenemann commented 2 years ago

Sorted by what?

The majority of use cases of streamAllContents doesn't care about order. To me that seems like a special case that should be handled in the respective service.