-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve assertion error message for Apply
and TypeApply
#18700
Improve assertion error message for Apply
and TypeApply
#18700
Conversation
@nicolasstucki thank you 🙏 |
@nicolasstucki I fixed the root cause in #18719, and then I'm tweaking that in #18727. |
7c7d299
to
d2b97e7
Compare
Ok, I changed this PR to only improve the assertion failure message. |
Apply
and TypeApply
assert(ctx.reporter.errorsReported) | ||
assert( | ||
fn.isInstanceOf[RefTree | GenericApply] || ctx.reporter.errorsReported, | ||
s"Illegal TypeApply function prefix: $fn" |
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.
We should use the i
interpolator, so it's pretty-printed.
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.
No. We need to see the exact AST. Otherwise we would not see the difference between a TypeApply
and Inlined
; or some similar cases where show
is equivalent.
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.
Another way to see this is as a match error. Maybe I could encode it in a simpler way and make it fail with an equivalently useful output. I will try that.
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.
The alternative is
def TypeApply(fn: Tree, args: List[Tree])(using Context): TypeApply = fn match
case Block(Nil, expr) =>
TypeApply(expr, args)
case _: RefTree | _: GenericApply =>
ta.assignType(untpd.TypeApply(fn, args), fn, args)
case _ if ctx.reporter.errorsReported => // We expect the error. Otherwise, we want to throw a match error.
ta.assignType(untpd.TypeApply(fn, args), fn, args)
But this one does duplicate ta.assignType(untpd.TypeApply(fn, args), fn, args)
. I find the current one clearer.
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.
How about i"Illegal TypeApply function prefix: $fn (${fn.className})"
then? Don't need the verbose nested trees, I would think.
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.
Given the history of the failure, I would argue otherwise. Knowing which tree caused the issue in one of those large projects can be helpful in pinpointing the likely source of the problem.
These are valid prefixes for a TypeApply in the same way they are for an Apply.See #16861 (comment)