Use decoded names when checking if a classfile is reachable from client code #49
Conversation
…nt code Operators are encoded into `$<plain-name>` during compilation to please the JVM. Hence, the former implementation of `PackageInfo.isAccessible` was broken as it relied on the encoded name of a class, instead of using the decoded one. This commit fixes the issue. Fixes lightbend-labs#46
| def bytecodeName: String | ||
|
|
||
| /** The name as found in the original Scala source. */ | ||
| final def decodedName: String = NameTransformer.decode(bytecodeName) |
There was a problem hiding this comment.
Can you get here with a name that doesn't originate from Scala source? Technically you should try to avoid decoding them. We had a bug recently where a Java class named R$attr broke the compiler. Maybe it's not the most important thing for MiMa.
There was a problem hiding this comment.
Can you get here with a name that doesn't originate from Scala source?
Yes
Technically you should try to avoid decoding them. We had a bug recently where a Java class named R$attr broke the compiler. Maybe it's not the most important thing for MiMa.
Bugs are just part of software development, so I'm not really worried about that. The alternative would be to duplicate the logic, as I still want to use the decoded name for error reporting purposes. But the actual main reason for using decoded names, is that PackageInfo.isAccessible relies on it.
There was a problem hiding this comment.
Can you add a method to this trait def isScala: Boolean, and predicate the behaviour of decodedName on that?
I think the correct way to find out is to find the enclosing top level class and check if it has the 'ScalaSignature' or ScalaLongSignature annotation.
There was a problem hiding this comment.
It's a good idea, and I think we could do it while parsing a classfile. However, I can't dedicate more time to this at the moment and, overall, I believe the current implementation is not worst than what we had before, since we were already relying on NameTransformer.decode here.
We should definitely file a ticket, and maybe someone else can take it. Sounds ok?
There was a problem hiding this comment.
And here is the ticket #50
|
Should we merge it? Would be great if you guys could give it a try before I cut a new release. |
|
LGTM |
|
LGTM. I trust the enclosed test. |
…ng-accessibility-of-class-46 Use decoded names when checking if a classfile is reachable from client code
dotta
deleted the
issue/use-decoded-names-when-checking-accessibility-of-class-46
branch
Operators are encoded into
$<plain-name>during compilation to please theJVM. Hence, the former implementation of
PackageInfo.isAccessiblewas brokenas it relied on the encoded name of a class, instead of using the decoded one.
This commit fixes the issue.
Fixes #46
review by @gkossakowski, @retronym