(Edited) better error messages when Future[T] is not used

[alehander92]

Summary

The docs say that we shouldn't discard futures, but Nim should really catch that on compile time if possible.

Description

The easiest way should be by adding an {.undiscardable.} pragma for types, using it for Future and making nim generate a compile time error discarding such types on sem checking: I can open a PR, if this is accepted

Alternatives

Just forbidding discarding Future, but not sure how this will work without somehow hardcoding that (the user might have different Future type)

[metagn]

This is currently possible with term rewriting macros.

import async

template discardedFuture{discard fut}(fut: Future) {.
  error: "cannot discard future, use asyncCheck instead".} = discard

proc foo {.async.} = discard

discard foo()

[dom96]

holy wow, that's amazing if it works. @hlaaftana you wanna make a PR?

[metagn]

On it

[Araq]

discard x is a dangerous construct, you clearly throw away information, you clearly should know what you're doing. Comparable to Nim's cast.

It's a race to the bottom otherwise, async is complex and you need to read its documentation. We cannot produce a system where you never have to read anything and even if we did, people then complain about other things.

[dom96]

Huh? Why reject this if it works? Your comment gives no reasoning as far as I can see.

[dom96]

I see what happened (#14176). Once again I would have appreciated a ping on a clearly async-related PR.

Now can we get the code that @hlaaftana suggested above? Is the only reason you rejected it because a test (which is incorrect as far as I can see) used discard? From my point of view, @hlaaftana's PR already found a bug. Imagine how much more impact this change will have on making code which leaks exceptions better.

[alehander92]

template discardedFuture{discard call(a)}(call: proc, a: varargs[untyped]) =
  when call(a) is Future:
    static:
      error "cannot discard an async call, use asyncCheck instead"
  else:
    discard call(a) 

hm!

EDIT: add else!

[alehander92]

after some discussions with @disruptek I agreed that async calls should be separated from futures: I want to see the first one not discardable , so it seems this variation of the original author's template maybe does it? i tested it a bit with

import asyncdispatch, macros

template discardedFuture{discard call(a)}(call: proc, a: varargs[untyped]) =
  when call(a) is Future:
    static:
      error "cannot discard an async call, use asyncCheck instead"
  else:
    discard call(a)

proc sync =
  raise newException(ValueError, "e")

proc aSync0(b: int): int =
  echo b

proc a2(b: int, b2: int) {.async.} =
  echo 0

proc a0 {.async.} =
  echo 0
proc a(b: int) {.async.} =
  if b == 0:
    sync() 
  else:
    echo 0

proc run {.async.} =
  var fut = newFuture[int]()
  discard fut
  asyncCheck a0()
  discard a2(0, 0)
  discard a(0)
  discard aSync0(0)

waitFor run()

thanks again to the original author!

[alehander92]

ok, it seems some we get into some kind of recursion which is limited tho with discard in else : i am not sure how to match just proc returning Future, sorry, but we can generate discard in a different way like let ignore ?

[disruptek]

Still 👎 because it's still a solution to a problem that doesn't exist. discard works correctly. Let us not contrive to break it to penalize those who had the wherewithal to actually ask for its expected behavior.

[metagn]

template discardedFuture{discard call(a)}(call: proc, a: varargs[untyped]) =
  when call(a) is Future:
    static:
      error "cannot discard an async call, use asyncCheck instead"
  else:
    discard call(a)

This recurses the template a good 50 times (in the else branch). If I remember there is some pragma that lets you not recurse but I can't find what it is.

If async calls are bad, and the option that isn't async "calls" is let foo = asyncCall(), which gives an unused warning, and the alternative is let _ = asyncCall(), which my PR allowed, what's the problem here?

[metagn]

There is another, simpler solution to all of this.

import async

template discardedFutureError{discard fut}(fut: Future) {.
  error: "cannot discard future, use asyncCheck instead".} = discard

type ProperlyDiscardedFuture*[T] = distinct Future[T]

template properlyDiscard[T](fut: Future[T]) =
  discard ProperlyDiscardedFuture[T](fut)

proc foo() {.async.} = discard

discard foo() # error
properlyDiscard foo() # fine

This, coupled with every other alternative presented here and in #14176, should solve all your problems. This language is too powerful lol

[alehander92]

ah smart! but no , i agree with disruptek, discard for futures should stay, but i still think discard for async calls only should be detected

the rewriting thing works easily: there should be a way to match that, and even if not, one can define a concept for proc returning a future (with a macro for returnType) and probably easier ways are possible

[alehander92]

And i can't fully agree with @disruptek, there is nothing inherently correct about hiding errors with discard : discard for sync calls is not try except

it's just seems as a bad surprise for users waiting to happen and a recipe for bugs imho, sorry if i am wrong: silent runtime errors are not something we want, they seem worse than compile errors and than normal runtime errors: so let's use try except or "ignoreErrors" for such

Again, i am talking only about async calls not futures

[metagn]

discard for futures should stay

They would stay. With distinct types or let variable = with unused warning/{.used.}/let _ =/let () = if that ever gets added. Futures being discarded isn't banned, the language is made so it can't be, it's just harder to access, which is the point here. I really think some kind of asyncDontCheck/asyncIgnoreErrors is a good solution.

The only inconsistency here, is what @Araq said, term rewriting is a bad solution. They're not meant for this purpose. The problem is we should let users know at the code level that discarding futures is wrong, warning or error (documentation is cool but the code should be self-sufficient for information), and we're doing it by rewriting AST.

[alehander92]

I don't agree with that:

• discarding future values seems kinda ok
• discarding async calls seems to be where usually unexpected silent errors can happen

[metagn]

I've given up, sorry. I don't think there's a solution here that will appease everyone. Your call solution is more like a sink check but you can't use sink here. I resent the amount of effort and time I spent to argue about this just close the issue and let beginners waste time trying to figure out why their introductory async code doesn't work not to sound passive aggressive

[alehander92]

Well it's a valid discussion from all sides

Forgive me, @disruptek for my yesterday's irc/gitter behavior/attacks: I was indeed mistaken in my try/except argument, as i didnt think about how async does insert an invisible try/except by design, as well as other hypothetical macros: so this wasnt a good argument and i was way too aggressive about all that.

I still believe some kind of detection of discard asyncCall() is a good thing, as otherwise we still just confuse users and let silent bugs happen : i cant prove it with "perfect" language design, but the compiler's role should be to help, but this is your decision to make guys

[Araq]

So then we have a system where discard f() produces a warning/error and the uglier let _ = f() doesn't, yet, when I don't read documentation why wouldn't I have used let _ = f()? I've seen beginners write this too when they haven't yet learned about discard. It's silly.

[Araq]

Is the only reason you rejected it because a test (which is incorrect as far as I can see) used discard?

No, I rejected it because it used a TR macro for detection and TR macros are reserved for custom optimizations by design. Misusing language features in order to prevent misusing other language features isn't good. And that doesn't have to do much with async at all, so you weren't involved.

[dom96]

Okay, after discussing with Araq on IRC I agree that we shouldn't pick and choose for which types discard can be used: https://irclogs.nim-lang.org/02-05-2020.html#12:25:48.

One important thing is that the compiler leads new users in a wrong path:

import asyncdispatch

proc foo() {.async.} = echo(42)

foo()

Gives: Error: expression 'foo()' is of type 'Future[system.void]' and has to be discarded

Immediate fix for this is to s/discarded/used/, but in the long term the best thing would be to suggest asyncCheck in the error message for this case. I will change this issue to that.

Regarding some of the other responses here:

discarding future values seems kinda ok

How did you come to that conclusion @alehander92? Any future can be failed. It doesn't matter where you get it from. But maybe I'm missing something?

[metagn]

How did you come to that conclusion alehander92? Any future can be failed. It doesn't matter where you get it from. But maybe I'm missing something?

Must have thought about something like this:

let fut = foo()
discard fut
discard fut
asyncCheck fut

Which also points out how general of a feature discard is to rewrite. The intended way to rewrite things like this is =destroy but there's no workaround for that (distinct type?) and people would get more mad

That error message should be changed, but not specifically for asyncCheck. Something like expressions with a value not allowed here, pass to a routine or use discard.

I think we should also rename asyncCheck, the "async" is redundant and confusing since the type signature has a future already. The documentation for asyncCheck is also bad:

This should be used instead of discard to discard void futures, or use waitFor if you need to wait for the future's completion.

Void futures aren't the only thing that have to be discarded and waitFor shouldn't be used inside async procs