nolanderc / glsl_analyzer

Language server for GLSL (autocomplete, goto-definition, formatter, and more)
GNU General Public License v3.0
156 stars 3 forks source link

Known hover/completion bugs #32

Closed automaticp closed 8 months ago

automaticp commented 9 months ago

I'll post newly found bugs for hover and completion in this issue to keep track of them until a testing suite that covers them is implemented with #26.

@nolanderc, please don't close the issue even if you fix the bugs mentioned here, as more can be discovered in the process.

Feel free to post too if you find any!

automaticp commented 9 months ago

BUG 1 (Hover, v1.2.2):

Hover confusion in local subscopes (no shadowing):

image

Is okay for global-to-local scopes:

image

automaticp commented 9 months ago

BUG 2 (Completion, v1.2.2):

Completion confusion when field access is followed by a semicolon:

image

When no semicolon:

image

nolanderc commented 9 months ago

BUG 1: Right, to accomodate overloading in global scope we return all completions in the first scope a symbol occurs. However, overloading should not happen in local scopes. (Again, type-based overload resolution is WIP).

BUG 2: seems like completions are triggering on the ; instead of the ., most likely a off-by-one error somewhere… what happens if there’s a single space between the dot and semicolon?

automaticp commented 9 months ago

BUG 2: seems like completions are triggering on the ; instead of the ., most likely a off-by-one error somewhere… what happens if there’s a single space between the dot and semicolon?

Back to expected behavior:

image

Identifier completions seem to be fine next to a semicolon:

image

Something about the .?

automaticp commented 9 months ago

BUG 3 (Hover, v1.2.3):

Wrong type on shadowing initialization when hovered in rhs:

image

nolanderc commented 9 months ago

I see, the hover should really be const S, right?

In v1.2.3 I added this condition so that hovering foo in would show float in the following:

float foo;

However, this also means that we declare the name before checking the initializer (the tree is traversed from left to right). Essentially, we need an exception such that if the identifier is in the right-hand-side of an initializer we skip the left-hand-side.

We should also support things such as:

float foo = 1, bar = foo;

which makes things a bit more complicated, but I'll see what I can do :)

automaticp commented 9 months ago

I see, the hover should really be const S, right?

Yes, exactly!

In v1.2.3 I added this condition so that hovering foo in would show float in the following...

Is there any utility to provide hover information for a declared name in a declaration itself? I mean, the type is the previous token, why bother showing it? Maybe I'm not thinking of some edge case?

automaticp commented 9 months ago

BUG 4 (Completion, v1.2.3):

Completion is trigger-able within the comments:

image

nolanderc commented 9 months ago

Is there any utility to provide hover information for a declared name in a declaration itself? I mean, the type is the previous token, why bother showing it? Maybe I'm not thinking of some edge case?

If we don't register the local foo, it would be matched to the global:

float foo;
void main() {
    int foo /* <- this would show `float` */;
}

Completion is trigger-able within the comments:

True, just haven't gotten around to it :smile:

automaticp commented 9 months ago

BUG 5 (Hover, v1.2.3):

Nested struct fields do not resolve correctly. Seems to confuse with the field set of the outermost struct. If a name matches, outputs the type from the enclosing struct, if it doesn't, then no hover info is produced at all.

image

automaticp commented 9 months ago

If we don't register the local foo, it would be matched to the global:

Oh, yeah, best to keep it then.

nolanderc commented 9 months ago

Nested struct fields do not resolve correctly. Seems to confuse with the field set of the outermost struct. If a name matches, outputs the type from the enclosing struct, if it doesn't, then no hover info is produced at all.

I know :frowning_face: This is dependent on better type-checking, which is still WIP.

EDIT: Actually, I have an idea which might make this possible for most cases... Give me a couple minutes :cat:

automaticp commented 9 months ago

This is dependent on better type-checking, which is still WIP.

Oh, okay, I'll keep track of it still. Maybe will dump it in "expected fails" for now.

nolanderc commented 9 months ago

Good news: I fixed nested field completions in #41!

Bad news: I accidentally closed this issue :sweat_smile: But this is getting quite long and all bugs should now be resolved, so I think it's better to start a new issue so that we can save some scrolling :)

If you disagree, we can reopen this.

automaticp commented 9 months ago

But this is getting quite long and all bugs should now be resolved, so I think it's better to start a new issue so that we can save some scrolling :) If you disagree, we can reopen this.

I'd rather have this reopened since this is related to #26 and would like to close it once that is closed instead. I'm discovering these as I write tests, but I'm not even close to being done, so I'm pretty sure I'll find some more. Otherwise I'll just have to open another one called "Known hover/completion bugs 2" ;)

Good news: I fixed nested field completions in #41

You do this amazingly fast!

automaticp commented 9 months ago

BUG 4 (Completion in comments) is still trigger-able when the number of characters is 2 or less (v1.3.2):

image

nolanderc commented 9 months ago

This looks like the textual autocomplete form vscode (it looks at words which occur anywhere in the open file and suggests those). Note the abc prefix before the suggestion. This is not something we can influence.

automaticp commented 9 months ago

You're right, sorry, the tests disagreed too and saw the server correctly return nothing. It doesn't trigger as you type, only when you explicitly press completion button, so it's not intrusive.

automaticp commented 8 months ago

BUG 6 (Hover and Completion, v1.4.1):

Function declarations show up on hover and in completion if they haven't yet been introduced in that scope, but have been declared later in the same file.

image

See related test 1 and test 2

automaticp commented 8 months ago

BUG 7 (Completion, v1.4.1):

I want to consider the behavior described in #44 (duplicate completion entries) as a bug.

Related: test 1, test 2

automaticp commented 8 months ago

BUG 8 (Completion v1.4.1):

Field completion is trigger-able on arrays as if the array is the contained element itself.

image

where Elem is just

struct Elem { int field1; };

Line 38 is correct usage, and completion works there correctly. Line 36 should not, however, give any completions.

This is less critical than other bugs, but I stumbled upon it by mindlessly putting . behind every identifier and seeing if anything comes up, which in some sense is also a "workflow".