Skip to content

SI-7936 access protected members in package-protected class#5792

Merged
retronym merged 3 commits intoscala:2.12.xfrom
lrytz:t7936
Jul 7, 2017
Merged

SI-7936 access protected members in package-protected class#5792
retronym merged 3 commits intoscala:2.12.xfrom
lrytz:t7936

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Mar 21, 2017

A super access to a protected member in a package-protected class is allowed through
an intermediate public class. The fix for SI-5162 actually introduced an error for this
case, because the scala compiler would generate an INVOKESPECIAL with the package-protected
class as receiver, which is illegal. However, we can use the public intermediate class in
the invocation signature.

This is very similar to SI-4283.

@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Mar 21, 2017
NoSymbol
}
if (phase.erasedTypes && qual.isInstanceOf[Super] && tree.symbol != NoSymbol)
qual setType tree.symbol.owner.tpe
Copy link
Member Author

@lrytz lrytz Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is almost 12 years old.. 2a5f62338c#diff-b0ec9c916463796fd7ff1d2ed73d46e2L810

@lrytz lrytz force-pushed the t7936 branch 3 times, most recently from b94ba47 to e316b65 Compare March 22, 2017 15:17
@@ -1,4 +0,0 @@

object Test extends App {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test was removed because a more exhaustive version of it exists in https://github.com/scala/scala/tree/2.12.x/test/files/run/t4283

@lrytz
Copy link
Member Author

lrytz commented Mar 22, 2017

this took longer than expected, i hope i covered all cases now.. 🤞

@lrytz
Copy link
Member Author

lrytz commented Mar 22, 2017

OK, while I think it's going in the right direction, the kind of errors that are showing while sorting things out ask for not rushing this in 2.12.2 :) currently VerifyError: Bad <init> method call

@lrytz lrytz modified the milestones: 2.12.3, 2.12.2 Mar 22, 2017
@milessabin
Copy link
Contributor

@lrytz is it possible you're hitting the same problem I ran in to here?

@lrytz lrytz force-pushed the t7936 branch 6 times, most recently from ea76528 to 0495f53 Compare March 24, 2017 13:14
@lrytz
Copy link
Member Author

lrytz commented Mar 24, 2017

I've got some greens now. A nice side-effect of this fix is that it reduces the cases where one has to manually provide a Java interface as direct parent to allow emitting a super call. For example

// Java
interface T { default int f() { return 1; } }

// Scala
trait U extends T
class C extends U { def t = super.f }

now compiles using invokespecial U.f.

There are still situations where a parent needs to be provided, for example

// Java
interface T { default int f() { return 1; } }

// Scala
trait U extends T { override def f = super.f + 1 }
class C extends U

doesn't compile: the super accessor is implemented as invokespecial T.f, and U.f would not work, so T needs to be added as a direct parent.

@retronym
Copy link
Member

retronym commented Jul 3, 2017

This LGTM on a first pass review. Needs a rebase and I also need to try it out in anger.

lrytz added 3 commits July 4, 2017 09:57
A super access to a protected member in a package-protected class is allowed through
an intermediate public class. The fix for scala/bug#5162 actually introduced an error
for this case, because the scala compiler would generate an INVOKESPECIAL with the
package-protected class as receiver, which is illegal. However, we can use the public
intermediate class in the invocation signature.

Fixes scala/bug#7936

This is very similar to scala/bug#4283
Also removes the special-case in minimizeParents for Java interfaces,
they can now be removed since we know which ones are used for super
calls
@lrytz
Copy link
Member Author

lrytz commented Jul 4, 2017

Rebased

@lrytz
Copy link
Member Author

lrytz commented Jul 5, 2017

Should run this through a community build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants