Have compiler warnings for "bug prone use after move" on copyable types

[kunitoki]

Motivation

There are situations in which one have a copyable type, but an instance of the type should not be copied implicitly when consumed in a procedure with a sink argument when the instance is also used after the sink. It's possible to accidentally write incorrect code (or less performant code) without having the compiler warning about possible mistakes.

Much like what x-value references argument in C++ are doing: the compiler will force a creation of a x-value to be able to call into the function, without the added benefit of being able to be notified of bugprone use after moves (but tools like clang-tidy could spot instances of these usages, see https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html).

Description

Consider this snippet:

type
    X = object
      s: string

proc `=copy`(x: var X, y: X) =
    x.s = "copied " & y.s

proc `=sink`(x: var X, y: X) =
    `=destroy`(x)
    wasMoved(x)
    x.s = "moved " & y.s

proc consume(x: sink X) =
    echo "Consumed: ", x.s

Now consider these 3 cases.

 Case 1

proc main =
    var x = X(s: "abcdefg")
    consume(x)

when isMainModule:
    main()

Output will be:

Consumed: abcdefg

In this case everything is fine, value seems to be moved implicitly (without even reaching a call to =sink, which is even better).

 Case 2

proc main =
    var x = X(s: "abcdefg")
    consume(x)
    echo "Unconsumed: ", x.s # Let's add this, maybe it was done by mistake

when isMainModule:
    main()

Output will be:

Consumed: copied abcdefg
Unconsumed: abcdefg

This will copy the value x when calling consume because x is accessed after the call as well, so the sink parameter is just an optimization, but it's not an enforced specification of how the parameter should be transferred into the procedure. It might be incorrect code, compiler could warn this.

Case 3

proc main =
    var x = X(s: "abcdefg")
    consume(move(x))
    echo "Unconsumed: ", x.s

when isMainModule:
    main()

Output will be:

Consumed: abcdefg
Unconsumed: 

This is also suspicious, it might be fine or it might be incorrect code.

So in cases like 2 and 3 it would be great if the compiler could warn about use after move.

Ideas

Pragma at procedure definition when sink parameters are defined

proc consume(x: sink X) {.forcedsink: x.} =
    echo "Consumed: ", x.s

Will produce with Cases 2 and 3:

Warning: procedure 'consume' requires last read of passed argument 'x' but it seems it used after at line 45; routine: main

Backward incompatibility

None that I can think of in the API.

[Araq]

Tried --hint[Performance]:on? It then warns about implicit copies, and don't use move when you are not certain. The problem with snippets like let x = move y; use(y) is that they don't scale to let x = move y[i]; use y[j] -- when I had move-related bugs, the compiler could not help me anyway as aliasing is really hard. So ... in complex cases I need the explicit move but then I'm on my own already and the compiler cannot warn and in simple cases the move is not required anyway.

[kunitoki]

Will definately try the hint suggestion, was not aware of it could be used to identify implicit copies.

[Araq]

Alternatively we could think of a assertMove operation. It doesn't do anything but enforces that the compiler turned the read into a move. If the compiler could not, an error would be produced. This seems quite useful.

[beef331]

type
  X = object
    s: string

proc `=copy`(x: var X, y: X) =
  x.s = "copied " & y.s

proc `=sink`(x: var X, y: X) =
  `=destroy`(x)
  wasMoved(x)
  x.s = "moved " & y.s

proc consume(x: sink X) =
  echo "Consumed: ", x.s

template ensureSink(body: typed): untyped =
  {.push hint[Performance]: on hintAsError[Performance]: on.}
  body
  {.pop.}

proc main =
  ensureSink:
    var x = X(s: "abcdefg")
    consume(x)
    echo "Unconsumed: ", x.s

when isMainModule:
  main()

I figured would be a solution but it seems the move semantic/destructor pass does not respond to pragmas so as long as {.pop.} is there it does not work. Is this a bug or intentional for that pass?

[Araq]

push and pop are always broken.

[kunitoki]

Yes, something like assertMove would be useful. What about renaming it to ensureMove? It seems to me a more explanatory name (to not mistake it's a debug only feature).

[gemath]

@beef331's pragma example works if it is specified for the entire module, which is good enough for me. Maybe the name of the Performance hint is too general and ImplicitCopy would be better. If I read the compiler source correctly, it is only used for sink-related hints anyway (for failing to imply a sink parameter something like FailedImplicit could be used).

If per-parameter control is deemed absolutely necessary, we should try to define something less redundant than proc consume(x: sink X) {.forcedsink: x.}.

[beef331]

Yea I should've mentioned it worked if applied globally, but when the destructors insertion is ran it doesnt seem to revaluate pragamas so they toggle on/off dont work, or it inserts the destructor after the {.pop.} I have not looked into it just guessing.

[Araq]

What about renaming it to ensureMove?

ensureMove sounds good too, yes.

[disruptek]

How about ensureSink since, y'know, sink is an optimization; it cannot make your code more correct.

[gemath]

With a modifier taken, the signature could be

proc consume(x: taken X)

same as sink, but use-after-consume is an error. The name doesn't require users to understand the move mechanics behind the scenes. It could as well be owned AFAIK, just don't make me write the param name a second time in a pragma.

[Araq]

How about ensureSink since, y'know, sink is an optimization; it cannot make your code more correct.

This is unfortunately not true anymore for types that prevent =copy via .error. I know it's my fault for having spread this piece of misinformation but I didn't know better back then. Or maybe back then .error for =copy wasn't a thing.

[Araq]

With a modifier taken

The idea is good but adding type modifiers adds much more complexity than yet-another builtin like ensureMove/Sink.