Fixes a nimsuggest crash when using chronos

[jmgomez]

The following would crash nimsuggest on init:

import chronos
type
  HistoryQuery = object
    start: int
    limit: int

  HistoryResult = object
    messages: string

type HistoryQueryHandler* = proc(req: HistoryQuery): Future[HistoryResult] {.async, gcsafe.}

[arnetheduck]

I'm not sure I understand the fix - it's a perfectly valid snippet of code, why should introducing a type error fix it?

[jmgomez]

it's a perfectly valid snippet of code

Do you happen to know if this project should compile with nim devel? Chronos doesnt like the snippet:

Error: Cannot transform this node kind into an async proc. proc/method definition or lambda node expected.

So it's hard to tell if it's as valid as it seems.

It's WIP, as there is another segfault in the project that I want to identify.

[arnetheduck]

Do you happen to know if this project should compile with nim devel?

generally yes - devel should not break chronos - otherwise, devel has a significant regression that needs to be addressed cc @ringabout

[ringabout]

I cannot reproduce the error message on the Linux with the devel compiler (Compiled at 2024-02-07) or the stable compiler when compiling the mentioned snippet

[jmgomez]

@arnetheduck

generally yes - devel should not break chronos - otherwise, devel has a significant regression that needs to be addressed

I was referring to waku, the snippet above wont pass the test in asyncSingleProc.

 if prc.kind notin {nnkProcDef, nnkLambda, nnkMethodDef, nnkDo}:

So it still may be a localError as semProcTypeWithScope should be able to return the PType if well formed.

[jmgomez]

@ringabout

I cannot reproduce the error message on the Linux with the devel compiler (Compiled at 2024-02-07) or the stable compiler when compiling the mentioned snippet

What error message are you referring to? Did you mean the NimSuggest crash?

[arnetheduck]

I was referring to waku,

Waku currently isn't tested with nim devel since this would probably be too disruptive (both orc and devel have too many regressions compared to 1.6/refc) - at some point, it would make sense to make a pass over waku and see what bugs still affect it (in general, we lock the nim version that application projects like waku use).

It's interesting though if it's hitting this condition in chronos - it would mean that devel is somehow generating a different AST for something which is needs investigation.

[jmgomez]

@arnetheduck I see, things makes more sense now. The idea is to do a backport afterwards anyways so the tooling can be used in the project. But whatever the case, no valid Nim code should ever crash nimsuggest on init

[ringabout]

What error message are you referring to? Did you mean the NimSuggest crash?

I was referring to waku, the snippet above wont pass the test in asyncSingleProc.

No, I meant I didn't know the steps to reproduce waku tests to check whether it was a regression or not

[jmgomez]

No, I meant I didn't know the steps to reproduce waku tests to check whether it was a regression or not

Gotcha. NimSuggest does crash with the snippet above in 1.6 and 2.0 though

[arnetheduck]

NimSuggest

nimsuggest crashes on a lot of things it shouldn't crash on :stuck_out_tongue: but it would be nice if it also didn't give false error positives.

[jmgomez]

@arnetheduck

nimsuggest crashes on a lot of things it shouldn't crash on

So let's hunt those down :)

but it would be nice if it also didn't give false error positives.

But what would be a better response from the compiler there? The macro lib (chronos) is doing something that it shouldnt be doing (i.e. asyncdispatch works fine) and the user is hinted with the async pragma in the error type.

chk returns both: "Cannot transform this node kind into an async proc. proc/method definition or lambda node expected." and type expected, but got: proc (req: HistoryQuery): Future[HistoryResult] {.async, gcsafe.}"

[arnetheduck]

But what would be a better response from the compiler there?

Hold on - I can compile your snippet with the version of nim and chronos that nwaku uses - so something is fishy here - if nim compiles it, nimsuggest should give no error, right?

[jmgomez]

ah, yes, probably I have a deps issue. Im just running nimsuggest waku.nim --v4 --stdin on it. But regardless the case (i.e. current version of chronos dont taking into account the current conditions) NimSuggest shouldnt crash, no? If so, that's the case after the fixes which means tooling can work on it

From @arnetheduck

right - yeah, I think that's where the confusion is coming from - the code in nwaku is legit for its versions of nim and chronos, but if nimsuggest sees different versions it'll complain -> indeed, it should complain and not crash, so you might be looking at a legit bug..

this highlights an important point: the tooling must always use the exact same version of things as the compilation process. "more or less" what we're looking for as guiding principle is what python venv achieves, ie perfect isolation for each project that you have "open".. so a project like nwaku has an env.sh that sets up this environment (with import paths and so on) - in the future, if/when we get a package manager, it would be the work of the PM in concert with nim to ensure that tooling (and by extension vscode) looks only in the right places / versions of deps notably, the code in your snippet is also legit for nim devel and the versions of chronos that nwaku uses, so probably, you're picking up some out-of-date chronos from some random location - this is another significant problem / bug in the nim ecosystem: it'll look for code randomly in the nimble folder which causes a lot of grief and difficult-to-debug scenarios - the fact that nimble paths exist as default is a big impediment to a good tooling experience and something that should be fixed once the PM works in a satsifactory way

[github-actions[bot]]

Thanks for your hard work on this PR! The lines below are statistics of the Nim compiler built from 7bf8cd3f8654c152b87052e28c515e525b43b38c

Hint: mm: orc; opt: speed; options: -d:release 178003 lines; 7.506s; 769.082MiB peakmem