Fix back-quoted constructor params with identical prefixes

[martijnhoekstra]

Match names on decoded names.

Fixes scala/bug#8831

[martijnhoekstra]

Fixes scala/bug#8831

[som-snytt]

There's also an interaction with specialization, so I'd suggest test coverage. I didn't try to guess what would be broken before/after this change. It looks fixed now.

scala> case class wrong(`a b`: Int, a: Int)
class wrong

scala> wrong(1, 2)
val res0: wrong = wrong(1,1)

scala> import Specializable.Args
import Specializable.Args

scala> case class wrong[@specialized(Args) A](`a b`: A, a: A)
class wrong

scala> wrong(1, 2)
val res1: wrong[Int] = wrong(1,2)

scala> case class wrong[@specialized(Args) A](`a b`: A, a: Int)
class wrong

scala> wrong(1, 2)
val res2: wrong[Int] = wrong(1,0)

[martijnhoekstra]

Sleeping on this, I'll take a second look. If name1 != name2 but name1.decoded == name2.decoded it sounds plausible one of the two is double decoded, implying a bug when the decoded name itself is a valid encoded name.

Then I can also test specialized names and the intersection of those, if that's a thing.

[retronym]

Thanks for taking this on.

The things that need testing:

• case and regular classes.
• with case classes with public and private parameters. (Such parameters have a mangled name for the publicly available accessor method. Example case class C(private val foo : Int) gets foo$access$1 as the accessor method.)
• references to the parameter within the class body (class C(private val foo: Int) { val foo1 = foo }) -- the constructors phase rewrites this.foo$access1() to foo in this transform.
  • with/without specialization
• extracting the elements via a pattern match under joint and separate compilation. In partest, you can separately compile files this file name conventions: Classes_1.scala / Test_2.scala.
• Using synthetic case class unapply / productIterator / productElement extract the correct values.

[martijnhoekstra]

Thanks for that overview @retronym, that's very helpful for getting this into shape!

[martijnhoekstra]

I made a generator to test all those cases. It turns out to be a lot of cases, and becomes one of the slowest running tests. Is that OK, or do you have any suggestions to cut it down?

[daytonwilliams-okta]

Originator of SI-8831 here (even if I'm not using the same account, yes, it's me). I saw one bug scala/bug#10625 closed as a dupe of 8831. I don't actually see it covered by the test here, though. I'd hate for that ticket to be closed, only the fix for 8831 doesn't actually fix it.

[som-snytt]

+1 I hope the test case class is named ButWhy after hrhino's comment on that ticket.

[martijnhoekstra]

fixing scala/bug#10625 has much more impact than the ambitions of this PR: unexpandedName uses the heuristic of finding the last $$ and deciding the unexpanded name is the part after that. ^$# is encoded as $up$$hash and we incorrectly find hash as the unexpanded name.

Figuring out whether $$ is because of a literal $ followed by an encoded hash or because of an outer prefix is not possible: the name $outer$$inner just can't be disambiguated from the actual encoded version.

I suspect I could check whether the name $outer in $outer$$inner refers to an actual outer scope, and resolve the ambiguity that way, but that's too ambitious for this PR -- and I don't want to speak for anyone else, but maybe also too ambitious for scala 2.13 and with that for scala 2. Hopefully, this will nerd-snipe someone into proving me wrong, but I'm not counting on it.

Maybe that means that if this gets accepted, 10625 should be re-opened.

[som-snytt]

I almost re-opened the other ticket on that basis, but then I thought "don't crash in param name lookup in constructors" is the unifying theme. I don't think it's necessary to handle the maliciously crafted name in the sense of looking it up. My idea over there to support $dollar expansion for backquotes doesn't work (I assume) because we don't distinguish backquoted names. (I haven't tried doing that expansion explicitly in parser when scanning.)

[martijnhoekstra]

Another idea: if lookup fails on the unexpanded name, try again on the expanded name.

I'll see if it works. As the maxim goes, if it stupid and it works, it's still stupid but at least it works.

[daytonwilliams-okta]

Thanks for the detailed explanation of scala/bug#10625 Martijn (I hope I picked the right spot to break that username apart; apologies if not).

That does raise an interesting question: What happens if in addition to declaring ^$# as a member the case class also declares a member named hash? (I'm way too unfamiliar with the internals of the compiler to know and I haven't got access to one right this second to try it and see.)

[som-snytt]

Talk of names and hash made me also wonder if hoekstra is english huckster, actually. Also TIL the current US ambassador.

In case this isn't tested:

scala> case class C(#### : Int, ### : Int)
class C

scala> C(1,2)
val res5: C = C(1,1)

[martijnhoekstra]

Hoekstra is a Frisian name which, where the -stra postfix is somewhat common, thought to be akin to inhabitant of. Whether the Hoek prefix is the modern dutch hoek which means corner, derives from the archaic hoek, which in modern Dutch is haak and in English hook, or may have an entirely different Frisian etymology, I wouldn't know.

In the current PR:

scala> case class C(#### : Int, ### : Int)
case class C(#### : Int, ### : Int)class C

scala> C(1, 2)
C(1, 2)val res0: C = C(1,2)

so that's good.

Unfortunately, the horrible hack with first trying unexpanded and then expanded doesn't survive the assault of case class(^$#: Int, hash: Int)

[retronym]

@martijnhoekstra To reduce the number of cases, I'd suggest to give all the test classes the same parameter list, with enough parameters to cover the variants of parameters. This ought to reduce the permutations of classes to a reasonable amount.

Thanks for being so thorough!

[martijnhoekstra]

The tricky thing with just giving it more parameters is that the original bug is order-dependent i.e.

case class Foo(`a b`: Int, a: Int)

failed, where

case class Foo(a: Int, `a b`: Int)

was fine. But maybe I can trim down some more

[martijnhoekstra]

Reduce number of testcases by not testing the unaccessed variety.

[martijnhoekstra]

The hack around the unexpandedName works here, but the same edge-case will still fail for other uses of unexpandedName if it over-un-expands literal $$ or $# or something.

That remains festering. also see outerSource in Symbols.scala, scala/bug#2806. I haven't closely looked into the dotty situation, but scala/scala3#7601 may be related.

[SethTisue]

@retronym Let's try and land this for 2.13.4 if we can, as a surprising number of people have hit the bug it fixes.

[dwijnand]

Thanks, @martijnhoekstra!