Skip to content

Cleanup QuoteContext #8915

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

Merged
merged 1 commit into from
May 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/Splicer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ object Splicer {
catch {
case ex: CompilationUnit.SuspendException =>
throw ex
case ex: scala.quoted.StopQuotedContext if ctx.reporter.hasErrors =>
case ex: scala.quoted.Reporting.StopQuotedContext if ctx.reporter.hasErrors =>
// errors have been emitted
EmptyTree
case ex: StopInterpretation =>
Expand Down Expand Up @@ -389,7 +389,7 @@ object Splicer {
throw new StopInterpretation(sw.toString, pos)
case ex: InvocationTargetException =>
ex.getTargetException match {
case ex: scala.quoted.StopQuotedContext =>
case ex: scala.quoted.Reporting.StopQuotedContext =>
throw ex
case MissingClassDefinedInCurrentRun(sym) =>
if (ctx.settings.XprintSuspension.value)
Expand Down
4 changes: 2 additions & 2 deletions docs/docs/reference/metaprogramming/macros.md
Original file line number Diff line number Diff line change
Expand Up @@ -727,14 +727,14 @@ private def showMeExpr(sc: Expr[StringContext], argsExpr: Expr[Seq[Any]])(using
val showTp = '[Show[$tp]]
Expr.summon(using showTp) match {
case Some(showExpr) => '{ $showExpr.show($arg) }
case None => qctx.error(s"could not find implicit for ${showTp.show}", arg); '{???}
case None => Reporting.error(s"could not find implicit for ${showTp.show}", arg); '{???}
}
}
val newArgsExpr = Varargs(argShowedExprs)
'{ $sc.s($newArgsExpr: _*) }
case _ =>
// `new StringContext(...).showMeExpr(args: _*)` not an explicit `showMeExpr"..."`
qctx.error(s"Args must be explicit", argsExpr)
Reporting.error(s"Args must be explicit", argsExpr)
'{???}
}
}
Expand Down
4 changes: 2 additions & 2 deletions docs/docs/reference/metaprogramming/tasty-reflect.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ def natConstImpl(x: Expr[Int])(using qctx: QuoteContext): Expr[Int] = {
xTree match {
case Inlined(_, _, Literal(Constant(n: Int))) =>
if (n <= 0) {
qctx.error("Parameter must be natural number")
Reporting.error("Parameter must be natural number")
'{0}
} else {
xTree.seal.cast[Int]
}
case _ =>
qctx.error("Parameter must be a known constant")
Reporting.error("Parameter must be a known constant")
'{0}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ object CompileTimeMacros:
case (Expr.StringContext(Consts(parts)), Varargs(args2)) =>
Expr(StringContext(parts: _*).s(args2.map(_.show): _*))
case _ =>
qctx.throwError("compiletime.code must be used as a string interpolator `code\"...\"`")
Reporting.throwError("compiletime.code must be used as a string interpolator `code\"...\"`")
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,12 @@ object StringContextMacro {

val (partsExpr, parts) = strCtxExpr match {
case Expr.StringContext(p1 @ Consts(p2)) => (p1.toList, p2.toList)
case _ => qctx.throwError("Expected statically known String Context", strCtxExpr)
case _ => Reporting.throwError("Expected statically known String Context", strCtxExpr)
}

val args = argsExpr match {
case Varargs(args) => args
case _ => qctx.throwError("Expected statically known argument list", argsExpr)
case _ => Reporting.throwError("Expected statically known argument list", argsExpr)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me the usability is better if methods can be called directly on qctx, as they don't need to remember another top-level name. What's the consideration to introduce Reporting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be imported if needed. The previous location was unintuitive and a few missed it. Additionally, this seems to be the only reason to not use anonymous QuoteContexts.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about using the same trick as Dotty to introduce a method def qctx(using QuoteContext) = ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would also help. But, still errors and warnings do not belong there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you, logically it is better to separate things.

The little worry I have is that Scala 3 macro authors may also be compiler hackers, compiler plugin writers, and some experience with Scala 2 macros. In all other cases, the error reporting is the same by calling ctx.error. So it would be nice to follow the convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As shown, that can be optionally supported with an implicit conversion.

It also seems that the users of the API did not find the reporting utilities or ended up finding the ones in the reflection API. The second is fine but the overhead could have been avoided by making it more explicit.

}

val reporter = new Reporter{
Expand Down
2 changes: 1 addition & 1 deletion library/src-bootstrapped/scala/quoted/Expr.scala
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class Expr[+T] private[scala] {
* Otherwise returns the value.
*/
final def unliftOrError[U >: T](using qctx: QuoteContext, unlift: Unliftable[U]): U =
unlift(this).getOrElse(qctx.throwError(s"Expected a known value. \n\nThe value of: $show\ncould not be unlifted using $unlift", this))
unlift(this).getOrElse(Reporting.throwError(s"Expected a known value. \n\nThe value of: $show\ncould not be unlifted using $unlift", this))

/** Pattern matches `this` against `that`. Effectively performing a deep equality check.
* It does the equivalent of
Expand Down
31 changes: 5 additions & 26 deletions library/src-bootstrapped/scala/quoted/QuoteContext.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,10 @@ trait QuoteContext { self =>
val tasty: self.tasty.type
}

/** Report an error at the position of the macro expansion */
def error(msg: => String): Unit =
tasty.error(msg, tasty.rootPosition)

/** Report an error at the on the position of `expr` */
def error(msg: => String, expr: Expr[Any]): Unit =
tasty.error(msg, expr.unseal(using this).pos)

/** Report an error at the position of the macro expansion and throws a StopQuotedContext */
def throwError(msg: => String): Nothing = {
error(msg)
throw new StopQuotedContext
}
/** Report an error at the on the position of `expr` and throws a StopQuotedContext */
def throwError(msg: => String, expr: Expr[Any]): Nothing = {
error(msg, expr)
throw new StopQuotedContext
}

/** Report a warning */
def warning(msg: => String): Unit =
tasty.warning(msg, tasty.rootPosition)

/** Report a warning at the on the position of `expr` */
def warning(msg: => String, expr: Expr[Any]): Unit =
tasty.warning(msg, expr.unseal(using this).pos)
}

object QuoteContext {
// TODO remove in 0.26.0
@deprecated("Errors and warnings have been moved to scala.quoted.Reporting", "0.25.0")
given error_and_warining_on_QuoteContext as Conversion[QuoteContext, Reporting.type] = _ => Reporting
}
35 changes: 35 additions & 0 deletions library/src/scala/quoted/Reporting.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package scala.quoted

object Reporting {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or, can we make Reporting a trait, and make QuoteContext extend Reporting, the same as it is done in Dotty?

trait Reporting { thisCtx: Context =>
  // ...
}

For end-users, they don't need to know the concept Reporting, which improves usability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would not help to decouple the concepts. The idea is not to use the QuoteContext directly at all and only let it implicitly track the current scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is not to use the QuoteContext directly at all and only let it implicitly track the current scope.

Thakns for explanation, I can see it is a good design principle. The little question still in my mind is why not use qctx directly if it can improve usability, like it's done in Dotty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that based on the users, the usability of this feature was not great. It may look simple to us because that is the way it is done internally in dotty. I prefer to make it clear for normal users than for users that know the compiler internals.


/** Report an error at the position of the macro expansion */
def error(msg: => String)(using qctx: QuoteContext): Unit =
qctx.tasty.error(msg, qctx.tasty.rootPosition)

/** Report an error at the on the position of `expr` */
def error(msg: => String, expr: Expr[Any])(using qctx: QuoteContext): Unit =
qctx.tasty.error(msg, expr.unseal.pos)

/** Report an error at the position of the macro expansion and throws a StopQuotedContext */
def throwError(msg: => String)(using qctx: QuoteContext): Nothing = {
error(msg)
throw new StopQuotedContext
}
/** Report an error at the on the position of `expr` and throws a StopQuotedContext */
def throwError(msg: => String, expr: Expr[Any])(using qctx: QuoteContext): Nothing = {
error(msg, expr)
throw new StopQuotedContext
}

/** Report a warning */
def warning(msg: => String)(using qctx: QuoteContext): Unit =
qctx.tasty.warning(msg, qctx.tasty.rootPosition)

/** Report a warning at the on the position of `expr` */
def warning(msg: => String, expr: Expr[_])(using qctx: QuoteContext): Unit =
qctx.tasty.warning(msg, expr.unseal.pos)

/** Throwable used to stop the expansion of a macro after an error was reported */
class StopQuotedContext extends Throwable

}
4 changes: 0 additions & 4 deletions library/src/scala/quoted/StopQuotedContext.scala

This file was deleted.

2 changes: 1 addition & 1 deletion tests/neg-macros/quote-error-2/Macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ object Macro_1 {

def msg(b: Boolean)(using qctx: QuoteContext): Expr[String] =
if (b) '{"foo(true)"}
else { qctx.error("foo cannot be called with false"); '{ ??? } }
else { Reporting.error("foo cannot be called with false"); '{ ??? } }

}
2 changes: 1 addition & 1 deletion tests/neg-macros/quote-error/Macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ object Macro_1 {
inline def foo(inline b: Boolean): Unit = ${fooImpl('b)}
def fooImpl(b: Expr[Boolean])(using qctx: QuoteContext) : Expr[Unit] =
if (b.unliftOrError) '{println("foo(true)")}
else { qctx.error("foo cannot be called with false"); '{ ??? } }
else { Reporting.error("foo cannot be called with false"); '{ ??? } }
}
2 changes: 1 addition & 1 deletion tests/neg-staging/i5941/macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ object Lens {
apply($getter)(setter)
}
case _ =>
qctx.error("Unsupported syntax. Example: `GenLens[Address](_.streetNumber)`")
Reporting.error("Unsupported syntax. Example: `GenLens[Address](_.streetNumber)`")
'{???}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/run-macros/f-interpolation-1/FQuote_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ object FQuote {
values.forall(isStringConstant) =>
values.collect { case Literal(Constant(value: String)) => value }
case tree =>
qctx.error(s"String literal expected, but ${tree.showExtractors} found")
Reporting.error(s"String literal expected, but ${tree.showExtractors} found")
return '{???}
}

Expand Down
12 changes: 6 additions & 6 deletions tests/run-macros/i5941/macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ object Lens {
apply($getter)(setter)
}
case _ =>
qctx.error("Unsupported syntax. Example: `GenLens[Address](_.streetNumber)`")
Reporting.error("Unsupported syntax. Example: `GenLens[Address](_.streetNumber)`")
'{???}
}
}
Expand Down Expand Up @@ -95,7 +95,7 @@ object Iso {
// 2. A must be a tuple
// 3. The parameters of S must match A
if (tpS.classSymbol.flatMap(cls => if (cls.flags.is(Flags.Case)) Some(true) else None).isEmpty) {
qctx.error("Only support generation for case classes")
Reporting.error("Only support generation for case classes")
return '{???}
}

Expand All @@ -106,13 +106,13 @@ object Iso {
}

if (cls.caseFields.size != 1) {
qctx.error("Use GenIso.fields for case classes more than one parameter")
Reporting.error("Use GenIso.fields for case classes more than one parameter")
return '{???}
}

val fieldTp = tpS.memberType(cls.caseFields.head)
if (!(fieldTp =:= tpA)) {
qctx.error(s"The type of case class field $fieldTp does not match $tpA")
Reporting.error(s"The type of case class field $fieldTp does not match $tpA")
'{???}
} else '{
// (p: S) => p._1
Expand All @@ -139,7 +139,7 @@ object Iso {
val cls = tpS.classSymbol.get

if (cls.caseFields.size != 0) {
qctx.error("Use GenIso.fields for case classes more than one parameter")
Reporting.error("Use GenIso.fields for case classes more than one parameter")
return '{???}
}

Expand All @@ -154,7 +154,7 @@ object Iso {
}
}
else {
qctx.error("Only support generation for case classes or singleton types")
Reporting.error("Only support generation for case classes or singleton types")
'{???}
}
}
Expand Down
4 changes: 2 additions & 2 deletions tests/run-macros/i8671/Macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ object FileName {
case Right(fn) =>
'{FileName.unsafe(${Expr(fn.name)})} // Or `Expr(fn)` if there is a `Liftable[FileName]`
case Left(_) =>
qctx.throwError(s"$s is not a valid file name! It must not contain a /", fileName)
Reporting.throwError(s"$s is not a valid file name! It must not contain a /", fileName)
}

case _ =>
qctx.throwError(s"$fileName is not a valid file name. It must be a literal string", fileName)
Reporting.throwError(s"$fileName is not a valid file name. It must be a literal string", fileName)
}
}

8 changes: 4 additions & 4 deletions tests/run-macros/refined-selectable-macro/Macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ object Macro {
case _: TypeBounds =>
rec(parent)
case _: MethodType | _: PolyType | _: TypeBounds | _: ByNameType =>
qctx.warning(s"Ignored `$name` as a field of the record", s)
Reporting.warning(s"Ignored `$name` as a field of the record", s)
rec(parent)
case info: Type =>
(name, info) :: rec(parent)
Expand Down Expand Up @@ -57,10 +57,10 @@ object Macro {
// Tuple2(S, T) where S must be a constant string type
case AppliedType(parent, ConstantType(Constant(name: String)) :: (info: Type) :: Nil) if (parent.typeSymbol == defn.TupleClass(2)) =>
if seen(name) then
qctx.error(s"Repeated record name: $name", s)
Reporting.error(s"Repeated record name: $name", s)
(seen + name, (name, info))
case _ =>
qctx.error("Tuple type was not explicit expected `(S, T)` where S is a singleton string", s)
Reporting.error("Tuple type was not explicit expected `(S, T)` where S is a singleton string", s)
(seen, ("<error>", defn.AnyType))
}
}
Expand All @@ -79,7 +79,7 @@ object Macro {
})._2
// Tuple
case _ =>
qctx.error("Tuple type must be of known size", s)
Reporting.error("Tuple type must be of known size", s)
Nil
}
}
Expand Down
4 changes: 2 additions & 2 deletions tests/run-macros/string-context-implicits/Macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ private def showMeExpr(sc: Expr[StringContext], argsExpr: Expr[Seq[Any]])(using
val showTp = '[Show[$tp]]
Expr.summon(using showTp) match {
case Some(showExpr) => '{ $showExpr.show($arg) }
case None => qctx.error(s"could not find implicit for ${showTp.show}", arg); '{???}
case None => Reporting.error(s"could not find implicit for ${showTp.show}", arg); '{???}
}
}
val newArgsExpr = Varargs(argShowedExprs)
'{ $sc.s($newArgsExpr: _*) }
case _ =>
// `new StringContext(...).showMeExpr(args: _*)` not an explicit `showMeExpr"..."`
qctx.error(s"Args must be explicit", argsExpr)
Reporting.error(s"Args must be explicit", argsExpr)
'{???}
}
}
Expand Down
7 changes: 4 additions & 3 deletions tests/run-macros/tasty-interpolation-1/Macro.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import scala.quoted._
import scala.language.implicitConversions
import scala.quoted.autolift
import scala.quoted.Reporting.error

object Macro {

Expand Down Expand Up @@ -36,15 +37,15 @@ abstract class MacroStringInterpolator[T] {
catch {
case ex: NotStaticlyKnownError =>
// TODO use ex.expr to recover the position
qctx.error(ex.getMessage)
error(ex.getMessage)
'{???}
case ex: StringContextError =>
// TODO use ex.idx to recover the position
qctx.error(ex.getMessage)
error(ex.getMessage)
'{???}
case ex: ArgumentError =>
// TODO use ex.idx to recover the position
qctx.error(ex.getMessage)
error(ex.getMessage)
'{???}
}
}
Expand Down
4 changes: 2 additions & 2 deletions tests/run-macros/tasty-macro-const/quoted_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ object Macros {
xTree match {
case Inlined(_, _, Literal(Constant(n: Int))) =>
if (n <= 0) {
qctx.error("Parameter must be natural number")
Reporting.error("Parameter must be natural number")
'{0}
} else {
xTree.seal.cast[Int]
}
case _ =>
qctx.error("Parameter must be a known constant")
Reporting.error("Parameter must be a known constant")
'{0}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/run-macros/xml-interpolation-1/XmlQuote_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ object XmlQuote {
values.forall(isStringConstant) =>
values.collect { case Literal(Constant(value: String)) => value }
case tree =>
qctx.error(s"String literal expected, but ${tree.showExtractors} found")
Reporting.error(s"String literal expected, but ${tree.showExtractors} found")
return '{ ??? }
}

Expand Down
6 changes: 3 additions & 3 deletions tests/run-macros/xml-interpolation-2/XmlQuote_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,15 @@ object XmlQuote {
values.iterator.map {
case Literal(Constant(value: String)) => value
case _ =>
qctx.error("Expected statically known String")
Reporting.error("Expected statically known String")
return '{???}
}.toList
case _ =>
qctx.error("Expected statically known StringContext")
Reporting.error("Expected statically known StringContext")
return '{???}
}
case _ =>
qctx.error("Expected statically known SCOps")
Reporting.error("Expected statically known SCOps")
return '{???}
}

Expand Down
2 changes: 1 addition & 1 deletion tests/run-staging/staged-tuples/StagedTuple.scala
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ object StagedTuple {
else {
def fallbackApply(): Expr[Elem[Tup, N]] = nValue match {
case Some(n) =>
qctx.error("index out of bounds: " + n, tup)
Reporting.error("index out of bounds: " + n, tup)
'{ throw new IndexOutOfBoundsException(${Expr(n.toString)}) }
case None => '{dynamicApply($tup, $n)}
}
Expand Down