Fix #4754: Don't generate inline accessor for constants #4760
Conversation
allanrenucci
changed the title
|
Does this hold for protected accessors too? |
| def needsAccessor(sym: Symbol)(implicit ctx: Context) = | ||
| sym.isTerm && | ||
| (sym.is(AccessFlags) || sym.privateWithin.exists) && | ||
| !sym.isContainedIn(inlineSym) | ||
| !sym.isContainedIn(inlineSym) && | ||
| !sym.info.widenTermRefExpr.isInstanceOf[ConstantType] |
There was a problem hiding this comment.
You also need to check isIdempotentExpr like we do in TreeInfo#constToLiteral which is called from FirstTransform. Alternatively you could just check is(Final) I think.
There was a problem hiding this comment.
I don't think so. Here sym refer to a definition not an expression like in TreeInfo#constToLiteral.
I should add a test for private methods though making sure they get an accessor
There was a problem hiding this comment.
Can you elaborate why is(Final)?
private final val x: Int = 1 // won't be inlined by FirstTransform, so need accessor
private final def y: Int = 1 // won't be inlined either, need accessor
There was a problem hiding this comment.
I think the right test is `
!(sym.isStable && sym.info.widenTermRefExpr.isInstanceOf[ConstantType])That excludes defs with constant result. These have to be called anyway for their side effects.
tests/pos/i4754.scala
Outdated
| } | ||
|
|
||
| class Test { | ||
| (new Foo).foo(1) |
There was a problem hiding this comment.
This should be a run test that checks that no accessor method is present in the class using java reflection.
| def needsAccessor(sym: Symbol)(implicit ctx: Context) = | ||
| sym.isTerm && | ||
| (sym.is(AccessFlags) || sym.privateWithin.exists) && | ||
| !sym.isContainedIn(inlineSym) | ||
| !sym.isContainedIn(inlineSym) && | ||
| !sym.info.widenTermRefExpr.isInstanceOf[ConstantType] |
There was a problem hiding this comment.
I think the right test is `
!(sym.isStable && sym.info.widenTermRefExpr.isInstanceOf[ConstantType])That excludes defs with constant result. These have to be called anyway for their side effects.
|
This PR doesn't solve the underlying issue: inline accessor are not generated for private definition in companion objects. However once the issue is fixed, it could be added as an optimisation since accessors should not be needed for constant vals |
Need to tweak ensureAccessible to use the constant type instead of the reference.
No description provided.