Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(transport): libp2p compilation in Nim version-2-0 #1186

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Sep 1, 2024

For example:

/Users/runner/work/nim-libp2p/nim-libp2p/nimbledeps/pkgs2/websock-0.1.0-94f836ae589056b2deb04bdfdcd614fff80adaf5/websock/http/client.nim(173, 5) template/generic instantiation of `async` from here
/Users/runner/work/nim-libp2p/nim-libp2p/nimbledeps/pkgs2/websock-0.1.0-94f836ae589056b2deb04bdfdcd614fff80adaf5/websock/http/client.nim(165, 1) Warning: The raises pragma doesn't work on async procedures - use `async: (raises: [...]) instead. [User]
/Users/runner/work/nim-libp2p/nim-libp2p/nimbledeps/pkgs2/websock-0.1.0-94f836ae589056b2deb04bdfdcd614fff80adaf5/websock/websock.nim(257, 5) template/generic instantiation of `async` from here
/Users/runner/work/nim-libp2p/nim-libp2p/nimbledeps/pkgs2/websock-0.1.0-94f836ae589056b2deb04bdfdcd614fff80adaf5/websock/websock.nim(251, 1) Warning: The raises pragma doesn't work on async procedures - use `async: (raises: [...]) instead. [User]
/Users/runner/work/nim-libp2p/nim-libp2p/libp2p/transports/wstransport.nim(77, 18) template/generic instantiation of `async` from here
/Users/runner/work/nim-libp2p/nim-libp2p/libp2p/transports/wstransport.nim(83, 10) template/generic instantiation of `setResult` from here
/Users/runner/work/nim-libp2p/nim-libp2p/libp2p/transports/wstransport.nim(78, 26) template/generic instantiation of `mapExceptions` from here
/Users/runner/work/nim-libp2p/nim-libp2p/nimbledeps/pkgs2/chronos-4.0.2-c5e9517b9189713210e2abab8b77a68da71ded12/chronos/internal/asyncmacro.nim(542, 60) Error: expression 'value(cast[type(recv(s.session, pbytes, nbytes))](chronosInternalRetFuture.internalChild))' is of type 'int' and has to be used (or discarded); start of expression here: /Users/runner/work/nim-libp2p/nim-libp2p/libp2p/transports/wstransport.nim(78, 26)
stack trace: (most recent call last)

from https://github.com/vacp2p/nim-libp2p/actions/runs/10655841970/job/29533846606?pr=1145

For minimal example of this:

template g(body: untyped) =
  try:
    body
  except CatchableError:
    raise newException(CatchableError, "")

discard g(0)

Also, even in 2.0.8, a variation doesn't work:

template g(body: untyped) = body
discard g(0)

@tersec tersec changed the title fix libp2p compilation in Nim version-2-0 and devel fix(transport): libp2p compilation in Nim version-2-0 and devel Sep 1, 2024
Copy link
Contributor

@etan-status etan-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that should fix the recent issue in #1145 and #1185 CI.

@diegomrsantos
Copy link
Collaborator

Thanks for the fix. What is the cause of the issue? Was it implemented in a wrong, but supported way? This code hasn't changed for more than 2 years.

@diegomrsantos diegomrsantos enabled auto-merge (squash) September 1, 2024 21:10
@tersec
Copy link
Contributor Author

tersec commented Sep 1, 2024

Thanks for the fix. What is the cause of the issue? Was it implemented in a wrong, but supported way? This code hasn't changed for more than 2 years.

Some change from post-Nim 2.0.8 version-2-0 did trigger stricter checking in these instances of the return type of the template. The template, arguably, was always incorrect: it didn't indicate that it returned anything, and see examples, it only works because Nim got confused by the exception construction until recently.

For another, close example of Nim 2.0.8 already disallowing/being confused by this incorrect return type declaration:

template g(body: untyped) =
  try:
    body
  except CatchableError:
    raiseAssert ""

discard g(0)

yields

a.nim(7, 11) Error: expression '0' is of type 'int literal(0)' and has to be used (or discarded)

i.e. it was specifically that raise newException(...) bypassed this check somehow.

@tersec tersec changed the title fix(transport): libp2p compilation in Nim version-2-0 and devel fix(transport): libp2p compilation in Nim version-2-0 Sep 1, 2024
@diegomrsantos
Copy link
Collaborator

The error message seems a bit confusing. In the original error, it gets used, and your example is discarded.

@tersec
Copy link
Contributor Author

tersec commented Sep 1, 2024

The error message seems a bit confusing. In the original error, it gets used, and your example is discarded.

Not in a relevant way. Same error message:

template g(body: untyped) =
  try:
    body
  except CatchableError:
    raiseAssert ""

let t = g(0)

discard outside of the template is a red herring. The "discarding" it's complaining about it in the template g/mapExceptions itself, not in the code calling/invoking it.

@diegomrsantos
Copy link
Collaborator

The error message mentions the line where it is used.

@tersec
Copy link
Contributor Author

tersec commented Sep 1, 2024

The error message mentions the line where it is used.

Yes, it's a template. It doesn't really exist until it's instantiated.

@diegomrsantos diegomrsantos merged commit a1811e7 into vacp2p:master Sep 1, 2024
14 checks passed
@tersec tersec deleted the CjV branch September 1, 2024 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

3 participants