Skip to content

Fix #4811: Widen private[this]/protected[this] just before Flatten #4814

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 24, 2018
Merged

Fix #4811: Widen private[this]/protected[this] just before Flatten #4814

merged 1 commit into from
Jul 24, 2018

Conversation

allanrenucci
Copy link
Contributor

@allanrenucci allanrenucci commented Jul 19, 2018 •
edited
Loading

This fix the crash because we don't need to generate a getter or setter
for private[this] field anymore.

The original documentation for ElimLocals says:

This phase drops the Local flag from all private[this] and protected[this] members.
This allows subsequent code motions where code is moved from a class to its
companion object. It invalidates variance checking, which is consequently disabled
when retyping.

I believe this is legacy code and not true anymore (when code is moved to the companion object, modifiers are widened as needed). This phase is still required to Ycheck Flatten. Not sure why...

@allanrenucci
Copy link
Contributor Author

#159 is the original PR that introduced the phase

@allanrenucci allanrenucci requested a review from odersky July 19, 2018 16:25
@allanrenucci
Copy link
Contributor Author

test performance please

@allanrenucci allanrenucci assigned allanrenucci and unassigned odersky Jul 19, 2018
odersky
odersky reviewed Jul 19, 2018
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

There's something weird going on: Extension methods does move code to companion objects, which causes strictly speaking illegal code.

First, private[this] methods are moved as private[this] to the companion, which means the forwarder to the method is incorrect. This should be corrected by removing Local from extension methods.

Second, private[this] fields in the instance are then accessed from the extension methods in the companion object. So far, this is not a problem because Ycheck is a ReTyper that does not do ensureAccessible except for New nodes. But since value classes cannot contain inner classes it's impossible to have a New of a private[this] class accessed from an extension method.

All of this is a bit subtle and arbitrary. On the other hand, it only affects testing, not the actual code, which does not care about Local. So I would just do the change for extension methods and document the rest in the ExtensionMethods phase.

@allanrenucci
Copy link
Contributor Author

I took a different approach: I exclude fields when removing Local flags. Fields are not moved to the companion object so their modifier does not need to be widened

@allanrenucci allanrenucci assigned odersky and unassigned allanrenucci Jul 19, 2018
smarter
smarter reviewed Jul 19, 2018
View reviewed changes
compiler/src/dotty/tools/dotc/transform/ElimLocals.scala Outdated
*
* This allows subsequent code motions where code is moved from a class to its
* companion object.
* We maintain the Local flag for field since then cannot move to the companion
Copy link
Member

Choose a reason for hiding this comment

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

typo: then -> they

@odersky
Copy link
Contributor

odersky commented Jul 20, 2018

I took a different approach: I exclude fields when removing Local flags. Fields are not moved to the companion object so their modifier does not need to be widened.

Would be good to check that this works for this example:

class FooOps(s: String) extends AnyVal {
  def foo = s
}

The extension method accesses the private[this] s. Is it classified as a field? In this case the access would be illegal.

@allanrenucci
Copy link
Contributor Author

allanrenucci commented Jul 20, 2018 •
edited
Loading

This is what I get after ExtensionMethods. So it doesn't seem to be classified as a field

final class FooOps(s: String) extends AnyVal() {
  private[this] val s: String
  def foo: String = FooOps.foo$extension(this)
  override def hashCode(): Int = FooOps.hashCode$extension(this)()
  override def equals(x$0: Any): Boolean = FooOps.equals$extension(this)(x$0)
}

final lazy module val FooOps: FooOps$ = new FooOps$()
final module class FooOps$() extends Object() {
  final def foo$extension($this: FooOps): String = $this.s
  final def hashCode$extension($this: FooOps)(): Int = $this.s.hashCode()
  final def equals$extension($this: FooOps)(x$0: Any): Boolean =
    x$0 match
      {
        case x$0 @ _: FooOps @unchecked => $this.s.==(x$0.s)
        case _ => false
      }
}

@allanrenucci
Copy link
Contributor Author

I think it would be nice to have ReTyper do ensureAccessible. Maybe in a separate PR?

@allanrenucci
Copy link
Contributor Author

allanrenucci commented Jul 20, 2018 •
edited
Loading

The extension method accesses the private[this] s. Is it classified as a field? In this case the access would be illegal.

Ah yes indeed the extension method makes an illegal access (i.e. $this.s)

@allanrenucci allanrenucci assigned allanrenucci and unassigned odersky Jul 20, 2018
@odersky
Copy link
Contributor

odersky commented Jul 20, 2018 via email

@odersky
Copy link
Contributor

odersky commented Jul 20, 2018 •
edited
Loading

Edits: In fact, if you look at Getters, here's the relevant condition for creating a getter: not creating a getter:

  d.initial.asInstanceOf[SymDenotation].is(PrivateLocal) && !d.owner.is(Trait) && !isDerivedValueClass(d.owner) && !d.is(Flags.Lazy)

So, derived value class parameters do not get a getter, even if they are not Local. This means we can not also retract Local for fields if their owner is a DerivedValueClass. It's subtle, for sure, and needs to be documented in detail.

Summary: What I proposed originally here will not work.

@allanrenucci allanrenucci force-pushed the fix-4811 branch 3 times, most recently from a843f61 to 724c377 Compare July 24, 2018 15:23
@allanrenucci allanrenucci assigned smarter and unassigned allanrenucci Jul 24, 2018
smarter
smarter approved these changes Jul 24, 2018
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Update the docs of Getters and ExtensionMethods to indicate their new behavior, also would be nice if TreeChecker started checking accessibility everywhere.

@allanrenucci
Fix scala#4811: widen Local modifier in ExtensionMethods and Getter
b156b83 
This fixes the crash because in Getter we don't look for a setter that was not
generated.

The original documentation for ElimLocals says:

This phase drops the Local flag from all private[this] and protected[this] members.
This allows subsequent code motions where code is moved from a class to its
companion object. It invalidates variance checking, which is consequently disabled
when retyping.

We instead let ExtensionMethods remove the flag when needed. And then
remove all Local flags in Getters once there are not needed anymore.
@allanrenucci allanrenucci merged commit d79b57e into scala:master Jul 24, 2018
@allanrenucci allanrenucci deleted the fix-4811 branch July 24, 2018 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky left review comments

@smarter smarter smarter approved these changes

Assignees

@smarter smarter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@allanrenucci @odersky @smarter