nim CI fails with chronos

[timotheecour]

https://github.com/nim-lang/Nim/pull/17442/checks?check_run_id=2186758767

PASS: [3/42] chroma c                                                     ( 3.80 sec)
  FAIL: chronicles c
  Test "chronicles" in category "nimble-packages"
  Failure: reBuildFailed
  nim c -o:chr -r chronicles.nim
  Hint: used config file '/home/runner/work/Nim/Nim/config/nim.cfg' [Conf]
  Hint: used config file '/home/runner/work/Nim/Nim/config/config.nims' [Conf]
  Hint: used config file '/home/runner/work/Nim/Nim/pkgstemp/chronicles/nim.cfg' [Conf]
  ......................................................
  /home/runner/.nimble/pkgs/chronos-3.0.0/chronos/asyncmacro2.nim(185, 1) template/generic instantiation from here
  /home/runner/.nimble/pkgs/chronos-3.0.0/chronos/asyncmacro2.nim(187, 55) Error: can raise an unlisted exception: ref ValueError
  
  PASS: [5/42] cligen c                                                     ( 3.43 sec)

[arnetheduck]

The compiler error here doesn't make a lot of sense - it's not chronos breaking, but rather specific usages of chronos - ie the chronos test suite passes, and it does stress this code path.

verifyReturnType doesn't have a raises annotations, so it shouldn't trigger the error:

nim-chronos/chronos/asyncmacro2.nim

Line 185 in 4abd7a5

proc verifyReturnType(typeName: string) {.compileTime.} =

(there's also no push raises active here)

[timotheecour]

i suspect raises: [Defect] in var identName: proc(udata: pointer) {.gcsafe, raises: [Defect].} somehow interacts with this:

proc verifyReturnType(typeName: string) {.compileTime.} =
  if typeName.isInvalidReturnType:
    error("Expected return type of 'Future' got '$1'" %
          typeName)

which calls % => addf => invalidFormatString => ValueError

yes, compiler needs to generate a better error msg here linking a path where an exception can be raised to the place where a raises pragma is used.

please try to minimize to find root cause (package may need to be disabled to avoid showing PR's as red)

[arnetheduck]

It is a fact that verifyReturnType raises a ValueError, but that shouldn't matter - this proc doesn't have raises annotations - it shouldn't be part of any effect constraint checking.

identName doesn't logically interact with verifyReturnType - they're evaluated in different contexts - if this is indeed the case, then something is wrong.

please try to minimize to find root cause

you have a perfectly reproducible test case to work with already if you actually are interested in addressing the root cause - reducing it is something you can do when you understand what the bug is, which we don't, in this case. We know how to work around it however, since it appears quite often, but that will leave the bug unsolved and us with more technical debt.

package may need to be disabled to avoid showing PR's as red

Sure, I mean, this certainly will make the bug go away ;)

[arnetheduck]

verifyReturnType is used here:

nim-chronos/chronos/asyncmacro2.nim

Line 216 in 4abd7a5

verifyReturnType(repr(returnType))

the snippet that generates identName is here:

nim-chronos/chronos/asyncmacro2.nim

Line 320 in 4abd7a5

var procCb = getAst createCb(retFutureSym, iteratorNameSym,

What makes you think the two interact?

[arnetheduck]

nim-lang/Nim#17382 would be a starting point as well, it's a simple case with somewhat similar symptoms

[sinkingsugar]

The title is misleading..
I think chronos is showing something is broken in nim...

[mratsim]

We have an application securing about $6 billions dollars running on Chronos. We are currently stuck v1.2.6 because of regressions in future Nim versions. I sure hope the engine that drives that is kept as one of the important package.

This seems to be yet another regression. I expect regressions notified from Nim CI are at least accompanied by a minimal context on what was changed in the PR where the CI failed.

[ringabout]

This seems to be yet another regression. I expect regressions notified from Nim CI are at least accompanied by a minimal context on what was changed in the PR where the CI failed.

The failure happened after 4abd7a5, the regression of Nim maybe have been existing for a long time.

First Nim CI failure began with a minor docs change
nim-lang/Nim#17495

https://github.com/nim-lang/Nim/runs/2182689512

[timotheecour]

I don't really understand the backlash here; I might as well not have reported anything and let this problem slide. I don't know where the bug lies (nim, chronos, nim-chronicles or something else), all i know is that nim CI currently fails and the only important_packages that fails is chronicles, while running nim c -o:chr -r chronicles.nim

Next time I won't bother notifying this repo.

[arnetheduck]

@xflywind indeed, this smells of a latent bug in Nim, given the many similar bugs reported while creating this branch for which we were able to create an easy repros (and subsequently worked around - we try to document the workaround when possible with an upstream bug report).

Consequently, if we understood the issue well enough, we would have done a repro by now as well - but this could also be an opportunity for a Nim dev to uh.. improve the quality of the Nim compiler since there's a trivial repro in Nim's own CI - I'm pretty sure it's surmountable if we work together.

Now, as suggested by @timotheecour, in the interest maintaining its CI green, Nim is also free to sweep it under the rug and remove the package and not test it any more - if you do, I'd recommend you at least look deep enough to remove the right package (chronicles) - the backlash is mostly about the flippant approach to quality presented in the bug report where the first suggested solution is to lower the quality bar in Nim - this is something we generally would prefer not to see happen.

Like mentioned above as well, it's fairly easy to add a workaround in this case, the ValueError is bogus, it's a compile-time function so there's nothing meaningful about the compiler error being reported. Happy to do that, seems like a lose-lose solution though.

[timotheecour]

Now, as suggested by @timotheecour, in the interest maintaining its CI green, Nim is also free to sweep it under the rug and remove the package and not test it any more

see nim-lang/Nim#17513 for a better approach

[ringabout]

Since nim-lang/Nim#17513 is merged, this issue can be closed.

[timotheecour]

ok, closing, but there still needs to be a minimized issue filed in nim so that the chronos failure can be investigated, but at least now chronos keeps running in CI (although with error), please file an issue after minimizing.

[arnetheduck]

please file an issue after minimizing

You cannot keep assuming or requiring that we'll be able to minimize every issue and bug in the compiler - that's possible when the issue is well understood and one can work backwards from that understanding, but it's not possible when the issue is more complex and requires debugging the compiler first. This is what the backlash is about and you keep being surprised by it, which is getting odd.

The situation we're in is that the code looks good and the compiler error does not - that's usually grounds enough to open an issue.