-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #2960: Only allow one inserted apply per tree #2962
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
Conversation
This fix prevents infinite chains of inserted apply's. Fixes scala#2960.
The parameter is never queried in current code, just passed along.
@@ -1818,9 +1819,17 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit | |||
*/ | |||
def tryInsertApplyOrImplicit(tree: Tree, pt: ProtoType)(fallBack: => Tree)(implicit ctx: Context): Tree = { | |||
|
|||
def isSyntheticApply(tree: Tree): Boolean = tree match { | |||
case tree: Select => tree.getAttachment(InsertedApply).isDefined |
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.
Why is an attachment needed instead of checking if the tree selects nme.apply
?
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.
Good question. It depends how we want to specity it:
1st choice: An apply
is inserted on any expression in function position unless that expression is already an inserted apply.
2nd choice: An apply
is inserted on any expression in function position unless that expression is itself an apply
selection or call.
Which way should we go? Here's where it makes a difference. Consider the case where we want to expand to
f.apply(...).apply(...)
If we give only one apply
, which one should be the inserted one? According to the PR, it must be the second apply
. If we followed 2nd choice, this would be rejected instead.
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.
One reason to keep the current PR is that the first apply
could be macro generated where the macro returns a map, say. Then it would seem sensical to insert the 2nd apply automatically, as for any other map.
Can this go in now? |
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.
LGTM, but adding a need-spec tag so we remember to specify this at some point.
This fix prevents infinite chains of inserted apply's. Fixes #2960.