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

locals are not retained for the whole document #1151

Open
dead10ck opened this issue Nov 24, 2021 · 6 comments
Open

locals are not retained for the whole document #1151

dead10ck opened this issue Nov 24, 2021 · 6 comments
Labels
A-tree-sitter Area: Tree-sitter C-bug Category: This is a bug

Comments

@dead10ck
Copy link
Member

I reported this in the Matrix chat room, but filing an issue to track it.

I tried to improve the C highlighting to highlight macro invocations differently than regular function calls. After reading through the tree-sitter docs, it seems like local variables should be able to enable this.

So far, I think I've followed the docs pretty closely and came up with this. However, it doesn't seem to be working. Macro invocations are still being colored like any other function call.

I figured out that locals do seem to work, but only if the definition is on screen.

asciicast

As soon as you scroll past the definition, it loses it. I can see with added debug logging around here that the scope.local_defs is at first populated with the macro definition, but when it goes out of sight, scope.local_defs becomes empty.

@archseer mentioned that it could be because highlights are done only for the range on the screen, but that locals should be kept for the whole document.

https://github.com/helix-editor/helix/blob/master/helix-term/src/ui/editor.rs#L136-L142

Reproduction steps

Define a language locals query that finds variable definitions, and color them differently. If the definition and the reference are both on screen, it works, but as soon as the definition is off screen, the highlight reverts back to its own tree-sitter match instead of deferring to the definition's highlighter.

Environment

  • Platform: Linux
  • Helix version: v0.5.0-143-g8ae6d96
@dead10ck dead10ck added the C-bug Category: This is a bug label Nov 24, 2021
@sudormrfbin sudormrfbin added the A-tree-sitter Area: Tree-sitter label Nov 24, 2021
@dead10ck
Copy link
Member Author

@archseer unfortunately it doesn't look like the recent incremental changes mentioned in #1378 have fixed this issue. This is the branch I am using to test locals used in highlighting.

@archseer
Copy link
Member

Yeah I haven't addressed this yet, it requires some more thought. It's possible to resolve locals for the whole document when we deal with injections (but that's slightly inefficient if we don't need the whole set), but that doesn't solve the whole problem: we'd also need to know what highlighting query matches for that local.definition so we can use it for the reference.

A workaround would be resolving all the highlights ahead of time (so no querying at all happens during render), but that can use a lot of memory on large documents.

We could include more styling information on the locals.scm queries themselves: @local.definition.variable would use @variable for the @local.reference

@dead10ck
Copy link
Member Author

If we're worried about pathologically large documents, maybe we could do something kind of dumb: remember all definitions and highlight matches up to a certain size limit. If we reach the limit, bail out and don't remember anything (current behavior) so that we don't get erratic highlighting. This will enable local highlighting for 95% of cases, and fall back in extreme cases to save hx from blowing up.

@dead10ck
Copy link
Member Author

Another thought: do we have to remember all references? Could we get away with querying all of the references that are contained in the visible scopes that lie in the view? What I mean by that is e.g.:

int global = 1;

void foo() {
    char b;
}

// We only see from here down
void bar() {
    float f;
}
...

If our view starts at bar, then the scopes that lie within the view are bar and the top level scope. So the definitions we query for should be bar, f, and global.

@d-hain
Copy link

d-hain commented Mar 6, 2023

I just got the same thing.

Environment

  • Platform: Windows 10
  • Helix version: Latest (22.12) built from source

Function is in the view:

image

Function out of the view:

image

@archseer
Copy link
Member

archseer commented Mar 7, 2023

Yes, it hasn't been addressed yet which is why this issue is still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tree-sitter Area: Tree-sitter C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

4 participants