DanielGavin / ols

Language server for Odin
MIT License
375 stars 56 forks source link

Fix variable names in value declarations not having symbols #385

Closed thetarnav closed 1 month ago

thetarnav commented 1 month ago

I was trying to fix a bug where local variables did not get any symbols in their own declarations. e.g. in foo := 123, foo would get a symbol if it was declared globally, but not when declared in a proc scope.

The reason was that the offset of the foo ident in the decl was too low. The get_local proc only matched idents with locals if they were after the declaration. So I added another hacky condition to match offsets with the lhs expression directly. Kinda like matching pointers I guess. But I couldn't use a pointer because ast.Ident was getting passed around as a value instead.

This solution doesn't feel correct because it is too indirect. I was thinking I could just use a simple is_in_declaration flag, to simplify it, but it turns out the walker in analysis doesn't even know if the ident is in a declaration or not?

This also partially fixes #374 But only the case where both the shadowed and shadowing variables are mutable.

Also get_local had some weird conditions in it i < 0 couldn't ever happened and this one:

ret := local_stack[i].rhs
if ident, ok := ret.derived.(^ast.Ident);
    ok && ident.name == name {
    if i - 1 < 0 {
        return {}, false
    }
}

I'm guessing it has to do with doing foo := foo? But I couldn't ever hit it. Especially when it required i to be 0

DanielGavin commented 1 month ago

You could potentially also look at: get_locals_value_decl.

Change what the offset is of the local when it's being stored.

The whole walker was designed for completion, it is being used for something it wasn't really planned for - semantic tokens. Now it's being asked what the symbol is of something being declared, which is a meaningless question for completion.

So might have to refractor it. In fact it's probably not even the walker that's the issue, since you can already tell the identifier is being declared before going into the resolve walker.

DanielGavin commented 1 month ago

image

Semes to work in vscode without any code changes.

thetarnav commented 1 month ago

Simply changing the offset won't work here, right? The third foo is referencing the first local, not second.

foo := 123
{
    foo := foo
}
DanielGavin commented 1 month ago

Most likely not. I thought you had issues with foo := 2, but that works for me.

DanielGavin commented 1 month ago

Might just allow this hack. It might also be possible to check the identifier is in ast_context.value_decl, it's only initialized in the completion. But could also be set in hover and goto. Then just check if the identifier is in. This case can only happen in the beginning, since the first check will be the declaration or never again, while jumping through the resolve walker.

DanielGavin commented 1 month ago

Merging it for now. We have some tests now for shadowing, so could be helpful if deciding to change it.