protected members should not allow access from superclass

[scabug]

I'm finding all kinds of interesting bugs working on the tree and icode checkers. The following should not compile, but does.

class Base {
  def mkNew() = {
    val s = new Sub2
    s.foo = true
    s
  }
}

class Sub2 extends Base {
  protected var foo = false
}

If this is in doubt, I offer two pieces of evidence. One, the spec:

Protected members of a class can be accessed from within

– the template of the defining class
- all templates that have the defining class as a base class
– the companion module of any of those classes.

The superclass is none of the above. Secondly, the java behavior prohibits it. Have to put them in separate packages because java protected allows package access, but note that the scala version (unsurprisingly) has the same behavior either way.

// J.java
package bong;

import bing.S;

public class J {
  public S bippy() {
    S s = new S();
    s.bip = true;
    return s;
  }
}
// S.java
package bing;

public class S extends bong.J {
  protected boolean bip;
}

% javac -d out */*.java
bong/J.java:8: bip has protected access in bing.S
    s.bip = true;
     ^
1 error

Coming back around to the first sentence, the reason I know about this is that after babying the icode checker back to life, it started telling me about all kinds of stuff which looks wrong to it. And it seems generally to be correct. I'm guessing that somewhere in the late stages the compiler starts ignoring everyone and brute forcing its way to the access it wants, trusting itself to do the right thing even after access has been relaxed in some places because of the JVM. And obvious brokenness arises from being too strict, but being too relaxed can lay hidden indefinitely, and so it does.

Here one place at least where the compiler actually depends upon the incorrect behavior - it seems likely there are more. Symbols.scala, in class Symbol:

final def newOuterAccessor(pos: Position) = {
  val sym = newMethod(pos, nme.OUTER) 
  sym setFlag (STABLE | SYNTHETIC)
  if (isTrait) sym setFlag DEFERRED
  sym.expandName(this)
  sym.referenced = this
  sym
}

And class TermSymbol:

  class TermSymbol(initOwner: Symbol, initPos: Position, initName: Name)
  extends Symbol(initOwner, initPos, initName) {
    protected var referenced: Symbol = NoSymbol

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-3871?orig=1 Reporter: @paulp

[scabug]

@paulp said: I fixed this, but I don't really want to touch that area so here is a patch and test case.

http://github.com/scala/scala/commit/a9c28720f64c8d0cc5b35538c7eb4e2f659e21c5

[scabug]

@odersky said: The original code is without a doubt confused. But I did not see how the proposed change corresponds to the spec either (?)

[scabug]

@paulp said: There's a test case in the commit (the same one in this ticket) which compiles with current trunk and doesn't after my patch, with all other tests passing. So while I've zero doubt there's a better way to express the condition, you'll have to give me a clue what you mean if you want me to explain it better.

[scabug]

@odersky said: It's not a matter of explanation. This one test case passes, but I see no relationship of the code to the spec. So how do we know it's correct? In fact I tend to believe it isn't (but I did not have the time to figure out a counterexample either, sorry).

To explain: Protected is very subtle, and we can not hope to even approximate to have enough tests for it now. The best course of action is to do what's in the spec.

[scabug]

@paulp said: I'll have to take your word for it. If the implementation was a direct translation of the spec I'm sure I could point to where it doesn't follow it and say "here." To recap, here is what I have done:

1) Precisely identify a bug, cite the specification to show that it is a bug according to the specification, and include a minimal reproduction and an example of where the compiler relies on the bug.

2) Modify the implementation to disallow the bug as reported without causing any other observable change (other than the spot where it was being relied upon.) Submitted patch.

You're holding me to a far higher standard here than anyone else ever is. I can accept that: I would be happy to try to write the method according to the spec if I thought it might be used. But it'd be a much bigger patch and it seems unlikely it'd be any better received, so I think it's more practical for me to go on to the next ticket.

[scabug]

@odersky said: (In r23298) Closes #3871. I think this implements the right set of rules, corresponding to the spec. Bonus: better error messages if protected access fails. Review by extempore.