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 #776: do not narrow access bounds of overriden methods in ParamForwarding #784

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
1 participant
@DarkDimius
Member

DarkDimius commented Sep 7, 2015

No description provided.

@DarkDimius

This comment has been minimized.

Show comment
Hide comment
@DarkDimius

DarkDimius Sep 7, 2015

Member

/rebuild

Member

DarkDimius commented Sep 7, 2015

/rebuild

@DarkDimius

This comment has been minimized.

Show comment
Hide comment
@DarkDimius

DarkDimius Sep 7, 2015

Member

paramForwarding.scala used to pass as we generated under-specified bytecode(private method overriding public one) that happened to work. Now we need to have a correct fix.

Member

DarkDimius commented Sep 7, 2015

paramForwarding.scala used to pass as we generated under-specified bytecode(private method overriding public one) that happened to work. Now we need to have a correct fix.

@DarkDimius

This comment has been minimized.

Show comment
Hide comment
@DarkDimius

DarkDimius Sep 7, 2015

Member

for class

class A(val theValue: Int) {
}

class NonVal(theValue: Int) extends A(theValue) {
  def getTheValueInNonVal = theValue // use the constructor parameter theValue
}

before change we had:

public class NonVal
extends A {
    public NonVal(int theValue) {
        super(theValue);
    }

    @Override
    private int theValue() {
        return super.theValue();
    }

    public int getTheValueInNonVal() {
        return this.theValue();
    }
}

note @Override private. This lead to this.theValue being statically resolved to private method.
Additionally, it ment that in some callsites calling theValue on nw13 would lead to IllegalAccessError.

After change we have

public class NonVal
extends A {
    public NonVal(int theValue) {
        super(theValue);
    }

    @Override
    public int theValue() {
        return super.theValue();
    }

    public int getTheValueInNonVal() {
        return this.theValue();
    }
}

Note that method is now public, making it vulnerable to overriding in sublasses:

class X(override val theValue: Int) extends NonVal(0)

What scalac does in this case is this:

public class NonVal
extends A {
    public NonVal(int theValue) {
        super(theValue);
    }

    public int getTheValueInNonVal() {
        return super.theValue();
    }
}

As you see, it does not generate the method. I believe that this also supports my claim that method name is irrelevant.

Unfortunately, scalac approach cannot be implemented in a miniphase. What I propose instead is to create private method with a mangled name. I'll experiment with this.

Member

DarkDimius commented Sep 7, 2015

for class

class A(val theValue: Int) {
}

class NonVal(theValue: Int) extends A(theValue) {
  def getTheValueInNonVal = theValue // use the constructor parameter theValue
}

before change we had:

public class NonVal
extends A {
    public NonVal(int theValue) {
        super(theValue);
    }

    @Override
    private int theValue() {
        return super.theValue();
    }

    public int getTheValueInNonVal() {
        return this.theValue();
    }
}

note @Override private. This lead to this.theValue being statically resolved to private method.
Additionally, it ment that in some callsites calling theValue on nw13 would lead to IllegalAccessError.

After change we have

public class NonVal
extends A {
    public NonVal(int theValue) {
        super(theValue);
    }

    @Override
    public int theValue() {
        return super.theValue();
    }

    public int getTheValueInNonVal() {
        return this.theValue();
    }
}

Note that method is now public, making it vulnerable to overriding in sublasses:

class X(override val theValue: Int) extends NonVal(0)

What scalac does in this case is this:

public class NonVal
extends A {
    public NonVal(int theValue) {
        super(theValue);
    }

    public int getTheValueInNonVal() {
        return super.theValue();
    }
}

As you see, it does not generate the method. I believe that this also supports my claim that method name is irrelevant.

Unfortunately, scalac approach cannot be implemented in a miniphase. What I propose instead is to create private method with a mangled name. I'll experiment with this.

@DarkDimius

This comment has been minimized.

Show comment
Hide comment
@DarkDimius

DarkDimius Sep 9, 2015

Member

Closing in favor of #786.

Member

DarkDimius commented Sep 9, 2015

Closing in favor of #786.

@DarkDimius DarkDimius closed this Sep 9, 2015

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