Move unsafe operations from Expr to UnsafeExpr

[nicolasstucki]

No description provided.

[liufengyun]

From my experience with ScalaTest, usage of underlyingArgument is pretty common. I don't see how moving it to another package can help meta-programmers.

[nicolasstucki]

It is common to use the reflect API variant of this method, we have it all over. But this is the Expr based basEd version which has never been used in any code so far. It is important to make explicit the fact that this is the only method in the Expr API that can change normal call semantics. This implies that macro that only uses Expr guarantees that call semantics are preserved the same way inline preserves them.

[liufengyun]

If it's not used, maybe we can remove them? Meta-programming is inherently dangerous and difficult, singling out this API and putting it in an unsafe package doesn't sound like a good idea. The safety warning can be reflected in the doc of the method. For example, Option.get and List.head are unsafe, we don't move them to another place.

[nicolasstucki]

These operations may be phased out into a library later. For now it is good to keep them around to see what legitimate use cases they support. But they should not be visible out of the box to any new user.

[liufengyun]

As discussed with @nicolasstucki , the eventual goal is to remove them, this is just an intermediate step.

[deusaquilus]

@nicolasstucki It looks like Expr.underlyingArgument has been moved to UnsafeExpr. I use Expr.underlyingArgument a lot for parsing quoted sections. Ever since it was removed, I started using Expr.unseal.underlyingArgument.seal everywhere. What's the rationale for removing Expr.underlyingArgument?

[nicolasstucki]

There where several issues with the version of Expr.underlyingArgument that was added in the previous release. First, it gave a (simple) way to break call semantics only using Expr, which is something that they are supposed to preserve. For example, a by-value argument could be evaluated more than once

inline def f(i: Int): Int = ${impl}
private def impl(i: Expr[Int]): Expr[Int] = '{
  ${Expr. underlyingArgument(i)}
  ${Expr. underlyingArgument(i)}
}

var x = 0
def next: Int = {
  x += 1
  x
}
f(next) // would return 2
f(next) // would return 4

In general it is unsafe to use the result underlyingArgument, it is only safe to gather static information about it such as its possible value at runtime.

Furthermore, breaking this guaranties has also shown to cause compiler crashes.

Originally underlyingArgument was added to overcome the limitations of missing inline parameters that would actually get inlined. This is not the case anymore and most of the code that used to call underlyingArgument now only define thier parameters as inline.

Nevertheless, if it is for simple inspection there are some ways to provide a sound version of underlyingArgument but we still need to improve some infrastructure to get there. Otherwise, you could define your own version of Expr.underlyingArgument that factors out all the Expr.unseal.underlyingArgument.seal or use the one in UnsafeExpr (though that one might be removed).

[deusaquilus]

@nicolasstucki Interesting! My use-case for Quill is to parse the inlined tree and use it to generate SQL, not use it directly. There are situations where we splice parts of the tree back in but they are clearly delimited with a lift(runtimeCode) and we're also fairly careful about what we do with the runtimeCode splice, typically it'll go into a preparedStatement.set(runtimeCode) block. Directly using the inlined parameter also works since we can just recurse into Inline(_, _, v) => v but it makes the parser's call-stack much deeper (performance impact?) and less comprehensible which makes debugging the parser harder. Using Expr.unseal.underlyingArgument.seal is also fine so long as it doesn't do anything too crazy with the term-tree (and I don't have to worry about it being removed :).

What approach would you recommend? Is using Expr.unseal.underlyingArgument.seal safe so long as we're careful about calling things multiple times? Are there any other safety issues to be aware of?

[nicolasstucki]

It is ok to use it and it will not disappear. Be careful if you extract any expression that defines a symbol.