-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix ClassCastException
on call to trait method with call-by-name argument, if implemented as SAM
#10830
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
Open
som-snytt
wants to merge
1
commit into
scala:2.13.x
Choose a base branch
from
som-snytt:issue/11237-CCE-value-class-SAM
base: 2.13.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix ClassCastException
on call to trait method with call-by-name argument, if implemented as SAM
#10830
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
|
||
trait Semigroup[F] { self => | ||
def append(f1: F, f2: => F): F | ||
val z = 10 | ||
} | ||
|
||
case class Box(i: Int) | ||
|
||
class R extends Runnable { | ||
def run() = { | ||
val boxSemigroup: Semigroup[Box] = (x1, x2) => Box(x1.i + x2.i) | ||
// disallowed, by-name must be inferred | ||
//val boxSemigroup: Semigroup[Box] = (x1: Box, x2: Box) => Box(Math.max(x1.i, x2.i)) | ||
assert(boxSemigroup.append(Box(1), Box(2)) == Box(3)) | ||
} | ||
} | ||
|
||
object Test extends App { | ||
new R().run() | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Spent too much time on understanding this thing... comparing with
here it's not
noApply
that is in the works, but thex
tree is added tobyNameArgs
.I was wondering why it doesn't work here.
The compiler first creates
((x1: Box, x2: () => Box) => $anonfun$run(x1, x2))
, runssuper.transform
on it, and then translates that into the anonymous class withdef append(f1: Box, f2: () => Box): Box = $anonfun$run(f1, f2.apply())
.The
.apply()
is the bug. The issue is thatbyNameArgs
no longer works on the second transformation.https://github.com/scala/scala/blob/v2.13.14/src/compiler/scala/tools/nsc/transform/UnCurry.scala#L499-L501
because at the second pass,
fn.tpe
already has the post-uncurry type, I guess from herehttps://github.com/scala/scala/blob/v2.13.14/src/compiler/scala/tools/nsc/transform/UnCurry.scala#L557.
so
fnParams
no longer has by name=> Box
, but() => Box
. Then thef2
argument tree is not added tobyNameArgs
, and theapply
is added.Your fix looks fine, I was just not 100% it would only ever trigger in that case we are looking at, or if it might cause other changes. So I went into the rabbit hole.
Maybe you have more thoughts..
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.
Sorry, I ought to have written it up. I spent a day to understand LL220, and wasn't confident in the desired behavior until I saw that dotty does the same thing (that is, the same result as this PR).
I think I had a different approach but finally did LL250 when I ran out of patience. This looks too brute-force or ad-hoc to me now. Maybe it seemed natural after looking at the trees for hours.
I remember wondering why L223 doesn't use
attachments.contains
, and assumed I would make another pass here, but didn't want to spend a half-day on subtleties about attachments. (contains
does not use retronym's hand-rolled set function.)At a minimum, I'll respool and document.
Uh oh!
There was an error while loading. Please reload this page.
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.
This would be the right fix I think https://github.com/scala/scala/compare/2.13.x...lrytz:scala:pr10830?expand=1
But it needs to be fixed for multiple param lists,
Apply(Apply(f), args0), args1)
we need to get the second params.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'll make some time now for the more thoughts you asked for.
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 spent too much time" etc. I think my previous idea was to pass a flag to say "this is just a forwarder", but today I noticed that you can detect it because the forwarder has
(x: () => T) => R
instead of(x: => T) => R
. The method param and the function param happen to be mismatched. More precisely, thevparam.tpt
is a function type, while thesymbol.info
is cbn. I exploit that to tag references withPreserveArg
attachment. (That could be improved to do only cbn args, but I think for a forwarder that is always the case.)Today I was looking at the change in
TreeGen
, which I previously avoided. I looked again at howbyNameArgs
is used and it's still not clear to me. Maybe if I take a quick power nap first.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 think the current logic in UnCurry is fine and we don't need anything specific for this bug.
The underlying issue is that the check
isByNameParamType(param.info)
tests aparam
of aMethodType
that went through theuncurry
type map, which replaces by-name params (DesugaredParameterType
).If we get the
param
symbol from the method symbol (and not from the AST), then it has a full type history and we see (during uncurry) that it's by-name. And it cleans up the thing explained in the comment ("Read the param symbols beforetransform(fn)
...").