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

Disallow phase inconsistent inline parameters #8061

Conversation

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jan 22, 2020

In preparation for #8060.

Originally we used inline parameters as the way to recover statically known values from the arguments. This worked on some predefined types such as literal primitives, case objects, case classes, enums among others. The latter ones where ad-hoc addons without a clear specification.

ValueOfExpr also recovers statically known values but it is more general. It can be implemented for any arbitrary type that represents a value. In this PR we introduce some extra ways to use this such as Expr.value, matching.Value and matching.ValueSeq.

ValueOfExpr makes the current use of inline parameters completely redundant hence this PR replaces all uses of inline parameters by equivalent code using ValueOfExpr.

It also simplifies the PCP as it removes a special case from the rules.

@nicolasstucki nicolasstucki self-assigned this Jan 22, 2020
@nicolasstucki nicolasstucki force-pushed the dotty-staging:disallow-phase-inconsistent-inline-params branch 10 times, most recently from 2ecdf91 to b937f9f Jan 22, 2020
@nicolasstucki nicolasstucki requested a review from liufengyun Jan 23, 2020
@nicolasstucki nicolasstucki force-pushed the dotty-staging:disallow-phase-inconsistent-inline-params branch 5 times, most recently from 334930e to 07f104e Jan 23, 2020
@nicolasstucki nicolasstucki marked this pull request as ready for review Jan 23, 2020
@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

nicolasstucki commented Jan 23, 2020

Will need to be rebased on #8077 (the second commit will disapear)

Copy link
Contributor

liufengyun left a comment

Otherwise, LGTM

docs/docs/reference/metaprogramming/macros.md Outdated Show resolved Hide resolved
inline def power(x: Double, inline n: Int) = ${ powerCode('x, 'n) }
private def powerCode(x: Expr[Double], n: Expr[Int]): Expr[Double] =
powerCode(x, n.value) // Transform `n` into an Int or emit an error if it is not possible

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 23, 2020

Contributor

Maybe do it safely by n.getValue

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 23, 2020

Author Contributor

This has the same semantics as before. If the inline parameter is not a constant, an error is emitted.

def unapply[T](expr: Expr[T])(given valueOf: ValueOfExpr[T], qxtc: QuoteContext): Option[T] =
valueOf(expr)

}

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 23, 2020

Contributor

We already have extractor for Const, a duplicate? The name Value is a little misleading, if we in fact only want to get the literal const out.

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 23, 2020

Author Contributor

Yes, Value is more general. I plan to replace and remove Const in another PR

* Otherwise returns the value.
*/
final def value[U >: T](given qctx: QuoteContext, valueOf: ValueOfExpr[U]): U =
valueOf(this).getOrElse(qctx.throwError(s"Expected a known value. \n\nThe value of: $show\ncould not be recovered using $valueOf", this))

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 23, 2020

Contributor

It seems this method only invites errors in meta-programming, maybe we should just remove it.

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 23, 2020

Author Contributor

Without it we would end up writing expr.getValue.get all over the place which would crash the macro expansion. This alternative emits an error saying that the call expected a constant value. Pointing to the argument that is not a value.

@@ -18,6 +18,14 @@ class Expr[+T] private[scala] {
*/
final def getValue[U >: T](given qctx: QuoteContext, valueOf: ValueOfExpr[U]): Option[U] = valueOf(this)

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 23, 2020

Contributor

I find getValue is a little misleading, if what it does is to get the literal out. Maybe getLiteralValue?

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 23, 2020

Author Contributor

It is not necessarily a literal value.

@nicolasstucki nicolasstucki force-pushed the dotty-staging:disallow-phase-inconsistent-inline-params branch 3 times, most recently from 4e68df9 to e90f44d Jan 23, 2020
@nicolasstucki nicolasstucki force-pushed the dotty-staging:disallow-phase-inconsistent-inline-params branch from e90f44d to e56574f Jan 23, 2020
@nicolasstucki nicolasstucki merged commit 9982f0d into lampepfl:master Jan 23, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@nicolasstucki nicolasstucki deleted the dotty-staging:disallow-phase-inconsistent-inline-params branch Jan 23, 2020
@nicolasstucki nicolasstucki mentioned this pull request Feb 3, 2020
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.