fix(suggest): file level cmds don't leak internal syms

[bung87]

Summary

Suggest commands related to the whole file (i.e. outline), and not
cursor positions, no longer include symbols from inside template and
generic routines.

Details

semIdeForTemplateOrGenericCheck is used to detect whether the cursor
is within a generic routine or template body, before this change it
didn't validate which suggest command was invoked ( ideCmd ). This
resulted in safeSemExpr being run and potentially reporting symbols
from inside such routines. This led to commands such as outline
containing symbols from the routine bodies polluting module level
results.

semIdeForTemplateOrGenericCheck has now been updated to check the
command against the newly added ideLocCmds constant, which includes
commands that require cursor position tracking ( ideSug , ideCon ,
ideDef , ideUse , ideDus ), preventing the unexpected analysis
and pollution of module level results.

A regression test was added to ensure that accidentally providing cursor
tracking information to module level commands doesn't change the
results.

[zerbina]

The more elaborate PR message helps with understanding your reasoning better, which thus makes review easier, nice.

[bung87]

I aware of that, just it take times, as it's much different from making PR in nim repo.

[zerbina]

Testing whether the cursor column is -1 relies too much on details decided far away from here, which introduces hidden dependencies making the code harder to reason about and change.

It's more robust to check whether a cursor is attached at all (whole project or whole file operations don't attach a cursor).

[saem]

@bung87 the PR message is really helpful, thank you!

[zerbina]

The changes are good, but the PR message needs to be adjusted a bit:

• the title and summary should focus less on the implementation and more on the change in behaviour (generic/template bodies no longer being sometimes outlined)
• the details section should mention under what circumstances the problem appeared (you can use the test to base this on)

I can help with phrasing and grammar, but the basic structure and information needs to be there.

[bung87]

updated, thank you!

[zerbina]

While updating the PR message, I thought about whether this would a regression for use, def, and highlight in templates/generics (these commands also need the semExpr call), and I think it would.

However, the workings of the three commands are a bit shaky with regards to templates/generics, so maybe the regression is okay. What are your thought's on this, @saem and @bung87?

[bung87]

I still have no knowledge why this need a seperate sem step semExpr, while use may works most of time as we place suggestSym in many slots, def will not work as said before. highlight never used in nimlsp. maybe need a test trigger the issue.

[saem]

However, the workings of the three commands are a bit shaky with regards to templates/generics, so maybe the regression is okay. What are your thought's on this, @saem and @bung87?

Honestly, I'm a bit confused, and it could be that the other changes are elsewhere.

My mental model:

• early within suggest if we don't have a valid cursor then cursor requiring commands are never run
  • error handling for this is upto nimsuggest or nimskulllsp if/as required
• that semIdeForTemplateOrGenericCheck should:
  • for cursor requiring commands, assume a valid cursor and set requiredCheck appropriately
  • for 'all usage'/global commands set requiredCheck to true
  • for commands that are neither set requiredCheck to false

Example commands (non-exhaustive):

• require cursors: suggest, consider param, usage, definition, definition + usage
• global: highlight and module modified

Thoughts/Questions:

• Right now it seems the set of commands we're guarding on is too narrow in semIdeForTemplateOrGenericCheck
• Is the early avoidance of running cursor requiring commands outside the scope of this PR?
• Usage is so very broken for me that there really isn't much to regress; while definition partially works and I use it often

[bung87]

well, if this intent to work for two kinds of commands then can't guard on cursor outside, the suggest tool run single command one time.

[zerbina]

My mental model:

Thank you for the overview. My mental model matches yours.

• Right now it seems the set of commands we're guarding on is too narrow in semIdeForTemplateOrGenericCheck

Yep, I think so too, and that's what I was trying to raise with my initial question.

• Is the early avoidance of running cursor requiring commands outside the scope of this PR?

Personally, I'd say so, yeah.

[saem]

well, if this intent to work for two kinds of commands then can't guard on cursor outside, the suggest tool run single command one time.

Just to make sure I understand, the reason we can't have early avoidance of running the command is not so much the avoidance part not working, but suggest runs once and unless something is marked dirty it won't run again, it'll simply use cached results. The changing of the cursor position doesn't invalidate cache.

So if we had the following module:

template a(thing: untyped) =
  thing.#[1]#

proc b[T](thing: T) =
  thing.#[2]#

And we ran two sug in a row, without any edits in between, at positions #[1]# and #[2]#, the semIdeForTemplateOrGenericCheck would only come into play for #[1]# and we'd only get cached results for #[2]#?

[bung87]

no, the dirty file thing you described only apply to #946 that ideUse, ideDus require pre processing allUsages, guard on outside will like ideCmd in IdeLocCmds then validate the col, so the col either valid or invalid when valid invoke semIdeForTemplateOrGeneric, on the other hand will not.

[saem]

Couple of minor phrasing items.

Once you've updated the PR body to the latest ping me, thanks.

[bung87]

@saem updated.

[saem]

It's no where close to done, but I made a single change of moving the second bullet point from the Summary to the Details. The Summary should be short, focusing on the user facing impact. Also it seems like you wrote the Summary first, instead write the Details first, then summarize it.

[saem]

/merge

[github-actions]

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:

---

Notes for Reviewers

• another problem: the initial test case will cause unhandled exception: vmtypegen.nim(337, 16) unreachable: tyGenericParam [AssertionDefect] when invoke safeSemExpr, stack trace here: https://pastebin.com/5WvZivqq

[saem]

/cancel

[saem]

/merge