-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Cleanup QuoteContext #8915
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package scala.quoted | ||
|
||
object Reporting { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, can we make trait Reporting { thisCtx: Context =>
// ...
} For end-users, they don't need to know the concept There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thakns for explanation, I can see it is a good design principle. The little question still in my mind is why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
} |
This file was deleted.
There was a problem hiding this comment.
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 introduceReporting
?There was a problem hiding this comment.
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
QuoteContext
s.There was a problem hiding this comment.
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) = ...
?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.