+ added nimsuggest support for exception inlay hints #23202
Conversation
…port number, echo the actual data we got from the nimsuggest process for diagnostic reasons
…c purposes in case of parseInt error
…kind = nkIdent' [FieldDefect]
compiler/modulegraphs.nim
Outdated
| @@ -55,10 +55,12 @@ type | |||
| concreteTypes*: seq[FullId] | |||
| inst*: PInstantiation | |||
|
|
|||
| SymInfoPair* = object | |||
| SymInfoPair* = ref object | |||
There was a problem hiding this comment.
Does it really need to be ref? The compiler suffers from a massive overuse of ref making things harder to understand than necessary.
There was a problem hiding this comment.
How do we set caughtExceptions then? This is done in a different pass, after the SymInfoPair objects have been collected. Then, we need to update them, by setting caughtExceptions. If it's not a ref object, we should copy the entire object, change a single field, then copy it again? Isn't that worse than using a 'ref object'? Or, is there an easier way?
There was a problem hiding this comment.
Why not update it in-place or move the object?
nimsuggest/tester.nim
Outdated
| except ValueError: | ||
| echo "Error parsing int: " & a | ||
| echo outp.readAll() | ||
| raise |
There was a problem hiding this comment.
What does that do? Improve the error message but does not handle the error (it uses re-raising)? But who had problems with the previous error message?
There was a problem hiding this comment.
During the development of this feature, I had a nimsuggest crash that only happened on the GitHub CI machines, which I could not reproduce on my computer. Therefore, I added this, so that the stack trace, produced by nimsuggest is printed on the screen, and becomes visible in the GitHub CI log. The bug, that caused the crash has been fixed, but I left this here, because it's useful for diagnostic purposes, even though it's unrelated to the inlay hints for exceptions. I can extract it to a separate PR if you want?
There was a problem hiding this comment.
Ideally you simply remove it as I don't like it. If you want a better/different error mesage handle the error completely aka replace the raise by quit.
compiler/modulegraphs.nim
Outdated
| proc add*(s: var SuggestSymbolDatabase; v: SymInfoPair) = | ||
| s.mgetOrPut(v.info.fileIndex, newSuggestFileSymbolDatabase()).add(v) | ||
|
|
||
| proc findSymInfo*(s: var SuggestFileSymbolDatabase; li: TLineInfo): ref SymInfoPair = |
There was a problem hiding this comment.
I saw that other functions in nimsuggest.nim return ref SymInfoPair - findSymData, findByTLineInfo, so I used ref for consistency. Is using ptr the preferred way? Should the other functions that return ref SymInfoPair also be changed to ptr SymInfoPair?
There was a problem hiding this comment.
If possible, yes. We don't wrap types with ref just so that we can get nil as a possible value. We use ptr for that. Of course, you need to watch out things are still memory safe but it's usually the case.
There was a problem hiding this comment.
Changed the result to ptr in findSymInfo: 6a22c4a
Should I change the others here, or should that be in a separate PR?
compiler/modulegraphs.nim
Outdated
| var idx = binarySearch(s.items, q, cmp) | ||
| if idx != -1: | ||
| new(result) | ||
| result[] = s.items[idx] |
There was a problem hiding this comment.
Why not use result = addr s.items[idx]?
|
How does this affect the memory consumption of nimsuggest? |
I hope the memory use won't increase much, but this needs to be measured. I'll try to do some measurements and report what I found. |
… the error completely and quit, instead of reraising the exception
Ok, I measured the memory use of nimsuggest on two files - nimsuggest.nim (an example of a large-sized project, as it includes the Nim compiler) and nimlangserver.nim (a medium-sized project). I used nimsuggest.nim: That's about 7.4% increase. nimlangserver.nim: That's about 26% increase. I'm a little surprised that we get a larger increase (percentage-wise) on smaller projects, compared to large projects. I would expect it to be the opposite. Maybe it's an artifact of the measurement method or the way the heap is managed (how it grows)? |
|
Can be this made opt-in somehow? Not everybody enjoys a nimsuggest that uses more memory just so that it reports exception tracking details which many don't care about. |
I'm thinking about this. Meanwhile I've commited a small optimization that makes the memory increase slightly less. Now it's: for nimlangserver.nim: It's still not opt-in. I'm still thinking about how to optimize this SymInfoPair structure. Since it's so critical, there are a lot of optimizations possible if it's redesigned, however these changes would probably be unrelated to the exception hints. |
|
One way to make this opt-in is to move the caught exceptions data to a parallel array (in other words, to partially move from an array of structs to a structure of arrays). This way, if exception hints are turned off, we can avoid allocating the extra memory. If there are no better suggestions, and if I don't come with a better solution, I'll try to implement this approach. |
|
Ok, let's see how this outline solution of yours looks like. |
…ains caughtExceptions as a separate array. This will allow for the memory for caughtExceptions not to be allocated, when the exception inlay hints are not enabled (which is not done, yet). Many things are refactored to allow converting SuggestFileSymbolDatabase from an array of structs, to a struct of arrays. Extracting further fields as separate arrays will result in further reduction of used memory, even when inlay hints for exceptions are turned off, since the SymInfoPair structure wastes too much memory, due to alightment padding.
…ce wasted memory, due to padding (this will actually happen in the next commit, when isDecl is moved as well). * fixed the commented out setting of caughtExceptionsSet that was temporarily disabled in the previous commit
…n (versus the previous column) inside getTokenLenFromSource
… that, instead of seq[bool] in SuggestFileSymbolDatabase.caughtExceptionsSet and SuggestFileSymbolDatabase.isDecl, in order to save memory.
|
Move |
Done: 00f6d55 |
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
|
This seems to have some issues inferring exceptions of generic procs. proc foo[T](x: T) =
discard
foo(1)A bell for the call foo(1) appears, supposedly raising "Exception". Removing the Issue is that the symbol table holds the generic variant instead of an instance, so propagation still works as it happens in sempass2, but the immediate ide hint is complete nonsense. |
This adds nimsuggest support for displaying inlay hints for exceptions. An inlay hint is displayed around function calls, that can raise an exception, which isn't handled in the current subroutine (in other words, exceptions that can propagate back to the caller). On mouse hover on top of the hint, a list of exceptions that could propagate is shown. The changes, required to support this are already commited to nimlangserver and the VS code extension. The extension and the server allow configuration for whether these new exception hints are enabled (they can be enabled or disabled independently from the type hints), as well as the inlay strings that are inserted before and after the name of the function, around the function call. Potentially, one of these strings can be empty, for example, the user can choose to add an inlay hint only before the name of the function, or only after the name of the function. (cherry picked from commit 899ba01)
This adds nimsuggest support for displaying inlay hints for exceptions. An inlay hint is displayed around function calls, that can raise an exception, which isn't handled in the current subroutine (in other words, exceptions that can propagate back to the caller). On mouse hover on top of the hint, a list of exceptions that could propagate is shown. The changes, required to support this are already commited to nimlangserver and the VS code extension. The extension and the server allow configuration for whether these new exception hints are enabled (they can be enabled or disabled independently from the type hints), as well as the inlay strings that are inserted before and after the name of the function, around the function call. Potentially, one of these strings can be empty, for example, the user can choose to add an inlay hint only before the name of the function, or only after the name of the function. (cherry picked from commit 899ba01)
I guess that's the conservative choice - you can't know which exceptions will be raised until you instantiate it - proc foo[T](x: T) =
when T is int: raise ValueError
else: raise OSError
foo(1) |
|
It propagates without issues in the sempass2 step of compiler, nimsuggest does not get the instance symbol and that is what fails. I tried hacking together a fix to demonstrate this in #23414 |
This adds nimsuggest support for displaying inlay hints for exceptions. An inlay hint is displayed around function calls, that can raise an exception, which isn't handled in the current subroutine (in other words, exceptions that can propagate back to the caller). On mouse hover on top of the hint, a list of exceptions that could propagate is shown. The changes, required to support this are already commited to nimlangserver and the VS code extension. The extension and the server allow configuration for whether these new exception hints are enabled (they can be enabled or disabled independently from the type hints), as well as the inlay strings that are inserted before and after the name of the function, around the function call. Potentially, one of these strings can be empty, for example, the user can choose to add an inlay hint only before the name of the function, or only after the name of the function. (cherry picked from commit 899ba01)
This adds nimsuggest support for displaying inlay hints for exceptions. An inlay hint is displayed around function calls, that can raise an exception, which isn't handled in the current subroutine (in other words, exceptions that can propagate back to the caller). On mouse hover on top of the hint, a list of exceptions that could propagate is shown. The changes, required to support this are already commited to nimlangserver and the VS code extension. The extension and the server allow configuration for whether these new exception hints are enabled (they can be enabled or disabled independently from the type hints), as well as the inlay strings that are inserted before and after the name of the function, around the function call. Potentially, one of these strings can be empty, for example, the user can choose to add an inlay hint only before the name of the function, or only after the name of the function. (cherry picked from commit 899ba01)
This adds nimsuggest support for displaying inlay hints for exceptions. An inlay hint is displayed around function calls, that can raise an exception, which isn't handled in the current subroutine (in other words, exceptions that can propagate back to the caller). On mouse hover on top of the hint, a list of exceptions that could propagate is shown. The changes, required to support this are already commited to nimlangserver and the VS code extension. The extension and the server allow configuration for whether these new exception hints are enabled (they can be enabled or disabled independently from the type hints), as well as the inlay strings that are inserted before and after the name of the function, around the function call. Potentially, one of these strings can be empty, for example, the user can choose to add an inlay hint only before the name of the function, or only after the name of the function. (cherry picked from commit 899ba01)
This adds nimsuggest support for displaying inlay hints for exceptions. An inlay hint is displayed around function calls, that can raise an exception, which isn't handled in the current subroutine (in other words, exceptions that can propagate back to the caller). On mouse hover on top of the hint, a list of exceptions that could propagate is shown.
The changes, required to support this are already commited to nimlangserver and the VS code extension. The extension and the server allow configuration for whether these new exception hints are enabled (they can be enabled or disabled independently from the type hints), as well as the inlay strings that are inserted before and after the name of the function, around the function call. Potentially, one of these strings can be empty, for example, the user can choose to add an inlay hint only before the name of the function, or only after the name of the function.