Spent too much time on understanding this thing... comparing with

  def f(x: => Int) = 0  def g(x: => Int) = f(x)

here it's notnoApply 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 here

https://github.com/scala/scala/blob/v2.13.14/src/compiler/scala/tools/nsc/transform/UnCurry.scala#L557.

sofnParams 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..

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 useattachments.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.

This would be the right fix I thinkhttps://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.

I'll make some time now for the more thoughts you asked for.

"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 inTreeGen, 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.

I think the current logic in UnCurry is fine and we don't need anything specific for this bug.

The underlying issue is that thecheckisByNameParamType(param.info) tests aparam of aMethodType that went through theuncurry type map, which replaces by-name params (DesugaredParameterType).

If we get theparam 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)...").

could we put thenoApply tag directly when the trees are created ingen.expandFunction, by makingnoApply a tree attachment?

I think that was the previous version? but I'll look again with so-called fresh eyes.