Let nimsuggest refer to generic instance symbols

[SirOlaf]

Ref #23202 (comment)

Makes nimsuggest show generic instances for symbols instead of the plain non-instantiated version, fixes displayed (inferred) exceptions on hover and makes the exception inlay hints added by #23202 accurate for generics.

I could see this breaking ide tooling in regards to finding usages of a symbol (seems to work as before though) and it now displays the substituted parameters instead of generic T ones upon hover of a function call, so might need a better solution or a followup by someone with more knowledge.

[nickysn]

Can't you just store both the non-instantiated and the instantiated version in the nimsuggest database and use the appropriate one for each nimsuggest feature? Also, when the exception hints are off, don't store the extra data, to avoid wasting memory for users who don't use this feature?

[SirOlaf]

Well as mentioned I don't know exactly how to go about fixing this, what I got working is displaying the correct symbol but that broke some highlighting according to tests it seems, probably because of the onUse I removed.

Not blaming you for this issue, nimsuggest is a huge mess from what I've seen, but I do not think the new exception tracking in the extension is ready in this state.

[nickysn]

Well as mentioned I don't know exactly how to go about fixing this, what I got working is displaying the correct symbol but that broke some highlighting according to tests it seems, probably because of the onUse I removed.

Just add some extra field to SuggestFileSymbolDatabase, e.g. IsInstantiatedGeneric: PackedBoolArray and just collect both the instantiated and non-instantied versions, then use the appropriate one for the exception hints and the other for the onhover highlighting as well as the other existing features.

Not blaming you for this issue, nimsuggest is a huge mess from what I've seen, but I do not think the new exception tracking in the extension is ready in this state.

Thanks for reporting this issue and for working on a fix. I think we can improve your fix the way I suggested above and it'll avoid breaking existing features.

[SirOlaf]

Will try. For reference, this is what it currently looks like for the typehint upon hover. Might be desirable in a modified state that shows that the function is a generic instance, but that one really is beyond me.

[SirOlaf]

Are the test failures even a bug? Having trouble understanding what the mismatched output refers to.

[nickysn]

Are the test failures even a bug? Having trouble understanding what the mismatched output refers to.

Yes, they are failing nimsuggest tests. E.g.

26/62 test: nimsuggest/tests/tgenerics.nim
==== STDIN ======================================

Test failed: /home/vsts/work/1/s/nimsuggest/tests/tgenerics.nim
  Expected:
def	skProc	tgenerics.printHelloValue	proc (hello: Hello[printHelloValue.T])	/home/vsts/work/1/s/nimsuggest/tests/tgenerics.nim	5	5	""	100

  But got:
def	skProc	tgenerics.printHelloValue.printHelloValue	proc (hello: Hello[system.float]){.gcsafe, raises: <inferred> [].}	/home/vsts/work/1/s/nimsuggest/tests/tgenerics.nim	5	5	""	100

When working on nimsuggest, it is helpful to run the tests locally. You can do this by running in the nim directory:

nim r nimsuggest/tester.nim

Note that this tests the binary bin/nimsuggest_testing, so you should probably copy bin/numsuggest or create a symlink.

[SirOlaf]

Apologies, meant the ones that have missing entries in the version that is tested for. Is there any significance to them being there?

Test failed: /home/vsts/work/1/s/nimsuggest/tests/tsi_highlight.nim
  Expected:  highlight	skProc	1	5	1
highlight	skType	1	8	3
highlight	skResult	1	0	0
highlight	skProc	2	0	7

  But got:   highlight	skProc	1	5	1
highlight	skType	1	8	3
highlight	skResult	1	0	0

[nickysn]

Apologies, meant the ones that have missing entries in the version that is tested for. Is there any significance to them being there?

Test failed: /home/vsts/work/1/s/nimsuggest/tests/tsi_highlight.nim
  Expected:  highlight	skProc	1	5	1
highlight	skType	1	8	3
highlight	skResult	1	0	0
highlight	skProc	2	0	7

  But got:   highlight	skProc	1	5	1
highlight	skType	1	8	3
highlight	skResult	1	0	0

I did not create these tests, but they're probably significant. We should ensure existing tests don't fail, unless we can prove that these particular tests are actually broken, which is probably not the case here. There's a missing skProc entry in this case, after this change.

[SirOlaf]

Yeah I'm aware it was missing, just wish there was documentation on all of this somewhere. Hard to figure out what highlight even means in this context, so could have been gibberish for all I know.

[SirOlaf]

Way it is now works I think, still unsure about the field in SuggestFileSymbolDatabase you suggested but looking into that.
Sadly there's still a lot of false positives/worst case assumptions because sempass2 assumes worst case as soon as cast is involved, but that probably can't be solved.

Example: raises: <inferred> [ref ValueError, Exception] where Exception comes from getEbase. Nasty because it inserts the marker all over the place.

[nickysn]

Apologies, meant the ones that have missing entries in the version that is tested for. Is there any significance to them being there?

Test failed: /home/vsts/work/1/s/nimsuggest/tests/tsi_highlight.nim
  Expected:  highlight	skProc	1	5	1
highlight	skType	1	8	3
highlight	skResult	1	0	0
highlight	skProc	2	0	7

  But got:   highlight	skProc	1	5	1
highlight	skType	1	8	3
highlight	skResult	1	0	0

I did not create these tests, but they're probably significant. We should ensure existing tests don't fail, unless we can prove that these particular tests are actually broken, which is probably not the case here. There's a missing skProc entry in this case, after this change.

Way it is now works I think, still unsure about the field in SuggestFileSymbolDatabase you suggested but looking into that. Sadly there's still a lot of false positives/worst case assumptions because sempass2 assumes worst case as soon as cast is involved, but that probably can't be solved.

Example: raises: <inferred> [ref ValueError, Exception] where Exception comes from getEbase. Nasty because it inserts the marker all over the place.

The goal of the hints is to expose the exception info, inferred by the compiler. The fact that the actual exception inference in the compiler can be improved in certain cases is a separate issue. However, automatic exception inference can never be ideal, there will always be needed to annotate certain procvar types with a raises pragma, etc. For people who don't like that, we offer the option of turning the exception hints off, if they find them annoying. It even reduces the memory use of nimsuggest.

[nickysn]

Hi, I've started working on finishing this in my own branch:
https://github.com/nickysn/Nim/tree/nickysn-SirOlaf-generic-sym-suggest
I'm going to open a separate pull request as soon as I get it properly fixed, cleaned up and I get all the tests to pass (without having to modify any of the tests).

[SirOlaf]

Thanks for picking it up, looking forward to seeing this fantastic feature working as intended