suggest: limit Psym.allUsages processing per ideCmd

[bung87]

Summary

• Inside suggestSym limit Psym.allUsages processing to ideNone,ideUse, ideDus, ideSug, ideCon
• Avoiding unnecessarily processing on other unrelated ideCmd

Details

• ideNone is for initial compilation.
• ideDus and ideUse require allUsages through their use of listUsages.
• ideSug, ideCon use Psym.allUsages for sorting results.
• Remove "no recompiling" logic as can not determine by dirty file with IdeCmd,
  a file that changed isn't same with dirty file runs with IdeCmd.

---

Notes for Reviewers

[zerbina]

Thank you for your work towards improving the suggestion engine!

For the PR message, I think mentioning that ideDus and ideUse require allUsages through their use of listUsages would help.

I'm not sure I understand the inclusion of ideNone just yet. Could you elaborate on why it's needed?

[bung87]

IDE tools wrap around suggest api will first compileProject with project entry file, while ideCmd is ideNone, as you can see the code in compiler/tools/suggest.nim, "no need to recompile anything" under if conf.ideCmd in {ideUse, ideDus} and dirtyfile.isEmpty, when not provide dirtyfile it doesn't recompile the project, so it should at least execute once when initial project.

[zerbina]

Hmm, but in what situations are use or dus ran without a dirty file (which, if I understand correctly, means that no cursor is provided)?

[bung87]

it means no editing file as far as I know. user just open exists project.

[zerbina]

Okay, I looked into the implementation. Without a cursor position, use and dus can only a report a rsemSugNoSymbolAtPosition report (since findUsageSym wont find anything). So far, I don't see what allUsages need to be populated for during the initial compilation, given that all commands that do need the contents cause a partial recompilation already.

Do you have a reproducible example where ideNone not being included causes a difference in behaviour?

[bung87]

don't know why you mentioned "cursor position", the code avoiding re compilation when use, dus is because this operation is heavy, if there's no file changed the allUsages no need processing again, allUsages remain the same, actually there's another important reason process these symbols when initialization, the usages should analyze on project file, as the symbol usages across whole project, it can't be determined specific module.

BTW you can remove the ideNone then test files for these commands will fail.

[zerbina]

Ah, got it, I confused dirtyFile being empty with no cursor position being provided, which is where my misunderstanding stemmed from.

Okay, that makes things clearer, but I think that there's a problem now. Consider the following:

1. nimsuggest is opened for a project consisting of a single module.
2. the initial full compilation is run and the allUsages seqs are populated (because ideNone is included in the "allowed" set).
3. the file is modified on disk
4. a command like highlight is executed, which causes recompilation. However, allUsages is not populated
5. use or dus is run without the file having changed since running highlight

If I understand correctly, the result would be that one now gets nothing as the result. Is that correct?

[bung87]

maybe we can just remove no need to recompile anything thing, as {ideUse, ideDus} no much differences from other commands, they all need process whole nodes, and make the mod command work as modification notification, not used in two different ways.

well, this is another problem, here the thing is doing right I think.

[zerbina]

well, this is another problem, here the thing is doing right I think.

It's possible that I misunderstand what you're saying, but does that mean that the PR, as is, is incorrect?

[bung87]

I mean the PR is for limit the Psym.allUsages processing per ideCmd, as is, now the comments going to dirtyFile topic.

[zerbina]

I detailed a possible issue with this PR here. Is that something that now fails or is it not?

[bung87]

the point 5 assuming running these commands without dirty file, that will fails.

[zerbina]

Okay, that's what I wanted to know, thanks. In that case, how do you plan to continue with this PR?

[bung87]

remove no need to recompile anything thing

[zerbina]

I think the implementation should stay how it was (until a rodfile-based approach is used).

The way it is now is not correct, as no usages outside of the selected module and its client modules are reported when the module the symbol is defined in was recompiled since the last use, dus, con, or sug command (or initial compilation).

[bung87]

isn't now you conflict with your previous comment? I explain this in last point in details section, also this apply to recent changes that handle isolated module

[zerbina]

isn't now you conflict with your previous comment?

Which one do you mean? I'm not sure I follow.

---

As for the incorrect behaviour I've mentioned, consider the following setup:

# module_a.nim
var global* = 1
global = 0 # use 1

# module_b.nim
import module_a
global = 1 # use 2

Now run the following commands:

nimsuggest --tester module_b.nim
# all usages are collected during the initial compilation
> use module_a.nim:2:1
# both usage 1 and 2 are reported
> def module_b.nim:2:1
# module_b and module_a are recompiled, but no usages are collected
> use module_a.nim:2:1
# only module_a is recompiled and thus the only reported usage is usage 2 

Prior to this PR, the last use command properly reports all usages.

[bung87]

I just test on local, both this patch and devel branch passed as below, though it's not my expectation.

discard """
$nimsuggest --tester nimsuggest/tests/fixtures/module_b.nim
>use $path/fixtures/module_a.nim:2:1
def;;skVar;;module_a.global;;int;;*;;1;;4;;"";;100
>def $path/fixtures/module_b.nim:2:1
def;;skVar;;module_a.global;;int;;*;;1;;4;;"";;100
>use $path/fixtures/module_a.nim:2:1
def;;skVar;;module_a.global;;int;;*;;1;;4;;"";;100
use;;skVar;;module_a.global;;int;;*;;2;;0;;"";;100
"""

[zerbina]

I should have provided a runnable test right away, my bad. There is a small but severe mistake in the setup I posted previously: the files for the def and last use command are swapped.

Here are the exact files I've tested with the locally:

# nimsuggest/tests/module_a.nim
var global* = 1
global = 2

# nimsuggest/tests/tmodule_b.nim
import module_a
global = 1

discard """
$nimsuggest --tester $file
>use $path/module_a.nim:3:1
def;;skVar;;module_a.global;;int;;*;;2;;4;;"";;100
use;;skVar;;module_a.global;;int;;*;;3;;0;;"";;100
use;;skVar;;module_a.global;;int;;$file;;3;;0;;"";;100
>def $path/module_a.nim:3:1
def;;skVar;;module_a.global;;int;;*;;2;;4;;"";;100
>use $path/tmodule_b.nim:3:1
def;;skVar;;module_a.global;;int;;*;;2;;4;;"";;100
use;;skVar;;module_a.global;;int;;*;;3;;0;;"";;100
use;;skVar;;module_a.global;;int;;$file;;3;;0;;"";;100
"""

A nimsuggest built from 0.1.0-dev.21054 successfully runs the test, while the nimsuggest from this PR fails, for the reasons I outlined earlier.