Initial implementation of nimsuggest v3

[yyoncho]

Rework nimsuggest to use caching to make usage of ide commands more efficient.
Previously, all commands no matter what the state of the process is were causing
clean build. In the context of Language Server Protocol(LSP) and lsp clients
this was causing perf issues and overall instability. Overall, the goal of v3 is
to fit to LSP Server needs

• added two new commands:

  • recompile to do clean compilation
  • changed which can be used by the IDEs to notify that a particular file has been changed.
    The later can be utilized when using LSP file watches.
  • globalSymbols - searching global references

• added segfaults dependency to allow fallback to clean build when incremental
  fails. I wish the error to be propagated to the client so we can work on fixing
  the incremental build failures (typically hitting pointer)

• more efficient rebuild flow. ATM incremental rebuild is triggered when the
  command needs that(i. e. it is global) while the commands that work on the
  current source rebuild only it

Things missing in this PR:

• Documentation
• Extensive unit testing.

Although functional I still see this more as a POC that this approach can work.

Next steps:

• Implement sug request.
• Rework/extend the protocol to allow better client/server communication.
  Ideally we will need push events, diagnostics should be restructured to allow
  per file notifications, etc.
• implement v3 test suite.
• better logging

[ghost]

Is there a need for this to be a regex? Can't it be done with contains (in) or something similar?

[ghost]

Code below seems to do find(cstring(sym.name.s), reg, 0, sym.name.s.len) , you can instead just use https://nim-lang.org/docs/strutils.html#find%2Cstring%2Cstring%2CNatural%2Cint to get the position of the first match

[yyoncho]

This is my personal preference. It will be pretty much up to us to decide whether to run with regex or with startsWith or with in. If we talk about other language servers some of them are using glob, and some of them use startsWith.

[ghost]

@yyoncho the problem is that it affects more than just personal preference - Nim's re library depends on PCRE, so if you make nimsuggest depend on re, users will have to make sure they have pcre installed. In a simple case like this I really think you should just use strutils.find.

[yyoncho]

@yyoncho the problem is that it affects more than just personal preference

Of course - I am just saying why I picked this implementation. Using find should be good enough for the initial implementation.

[yyoncho]

@Yardanico changed to contains. I was wondering whether to downcase before matching but I guess it might lead to too many matches.

[yyoncho]

Friendly ping.

[Araq]

Why is import segfaults required? Remove this, don't continue after segfaults, fix segfaults properly as they arise.

[yyoncho]

The idea here is to catch segfaults as part of incremental recompilation and then do a clean compilation. In some of the future commits I will change to code to send the notification to the client about that failure to make sure that these crashes are not lost and after that properly fixed. Note that in any other case the uncaught exception will trigger that same behavior - the application will exit.

[yyoncho]

To elaborate a bit more - I am trying to make nimsuggest more crash resilient to eliminate the need for having an external process wrapper(i. e. nimlangserver).

[yyoncho]

I am also planning to replace quit calls in the compiler with throw exception (in case nimsuggest) to make crashes less likely. Please share your thoughts.

[Araq]

The compiler keeps a tremendous amount of state around. This is pretty bad already and not easily fixed. If you continue after a bug the state becomes "corrupt", not necessarily in the sense that it causes a crash. A crash can easily be detected by nimlangserver or the like and the service can be restarted. If you instead make crashes disappear and at the same time nimsuggest produces wrong output, missing output etc then it becomes less trustworthy. It's not a good idea, Erlang's "let it crash" philosophy is the right idea and it relies on process isolation.

Process isolation is crucial for reliability. What we can do is to make nimsuggest a supervisor process like nimlangserver so that editor support does not have to reimplement the supervisor logic. Nimsuggest would then call something like a nimsuggest_worker process.

[yyoncho]

A crash can easily be detected by nimlangserver or the like and the service can be restarted.

I had that same conversation with @zah - the problem here is that there is no easy way to detect where the crash comes from - if the crash comes from something else and nimlangserver restarts nimsuggest then the result might be a loop.

The process isolation is crucial for reliability. What we can do is to make nimsuggest a supervisor process like nimlangserver so that editor support does not have to reimplement the supervisor logic. Nimsuggest would then call something like a nimsuggest_worker process.

Isn't throwing away ModuleGraph equivalent to starting a clean process? My understanding is that there is no state outside of ModuleGraph. Or you have other concerns? FWIW I have tried to emulate creating "fresh" module graph in initModuleGraphFields which will be called after recompile crashes. If we remove segfaults then we should remove the catch block here: https://github.com/yyoncho/Nim/blob/nimsuggest-version-3/nimsuggest/nimsuggest.nim#L707 because it no longer makes sense if the strategy is let it crash. The java language server has a similar strategy in a slightly different context - it performs incremental builds as you type but in addition it has a command Clean build which is for the cases when the server context breaks up.

If you don't buy these arguments what do you think about having this behavior behind a build flag?

PS: I see this code evolving and this "patch" is something that will go away once we address more and more recompilaton-related bugs. I have identified a few but I wanted to come up with something as usable as possible and then focus on fixing them.

[arnetheduck]

throw exception

generally, in a compiler api built for tooling, exceptions are not the way to go in many / most cases - instead you want to collect errors along the way without aborting the process, meaning that you pass in some collector - if we're removing quit and the like, this is likely a better way to go

[yyoncho]

@arnetheduck afaics that is what is happening in practice, it is just that when the collected error kind is in fatalMsgs it quits (https://github.com/yyoncho/Nim/blob/nimsuggest-version-3/compiler/lineinfos.nim#L214 ). So it is either throw an exception or let it continue. I guess for nimsuggest both should be good enough as long as it does not crash the process.

[Araq]

Apart from the import segfaults it's fine.

[yyoncho]

So I removed the import segfaults line to unblock the PR. I guess we can sort out that later.