-
Notifications
You must be signed in to change notification settings - Fork 3.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
Fix back-quoted constructor params with identical prefixes #9008
Conversation
Fixes scala/bug#8831 |
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.
|
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. |
Thanks for taking this on. The things that need testing:
|
Thanks for that overview @retronym, that's very helpful for getting this into shape! |
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? |
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. |
+1 I hope the test case class is named |
fixing scala/bug#10625 has much more impact than the ambitions of this PR: unexpandedName uses the heuristic of finding the last Figuring out whether I suspect I could check whether the name Maybe that means that if this gets accepted, 10625 should be re-opened. |
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 |
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. |
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 |
Talk of names and hash made me also wonder if In case this isn't tested:
|
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:
so that's good. Unfortunately, the horrible hack with first trying unexpanded and then expanded doesn't survive the assault of |
@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! |
The tricky thing with just giving it more parameters is that the original bug is order-dependent i.e.
failed, where
was fine. But maybe I can trim down some more |
don't test unaccessed
Reduce number of testcases by not testing the unaccessed variety. |
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 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. |
@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. |
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.
Thanks, @martijnhoekstra!
Match names on decoded names.
Fixes scala/bug#8831