Skip to content

Fix i19315: avoid calling addOuterRefs when the actual type is box-adapted #19323

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

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

Linyxus
Copy link
Contributor

@Linyxus Linyxus commented Dec 21, 2023

Fixes #19315.

The unsound code:

import language.experimental.captureChecking
trait Logger
case class Boxed[T](unbox: T)

// a horrible function, but it typechecks
def magic(l: Logger^): Logger =
  class Foo:
    def foo: Boxed[Logger^{this}] =
      // for the following line to typecheck
      //   the capture checker assumes {l} <: {this}
      Boxed[Logger^{this}](l)
  val x = new Foo
  val y = x.foo.unbox  // y: Logger^{x}
  val z: Logger = y  // now the capability becomes pure
  z

The magic function typechecks before this fix. It casts a capability to a pure value, which is clearly unsound. The crux of the problem is Boxed[Logger^{this}](l), which relies on the subcapturing relation {l} <: {this} enabled by the trick implemented addOuterRefs.

addOuterRefs augment a capture set that contains a self reference (like {this}) with all outer references reachable from this. In this case, the augmented set is {this, l}. The reasoning is that, any captured outer references in the class can be rewritten into a field, thus being a subcapture of {this}:

class Foo:
  val this_l: Logger^{l} = l
  def foo: Boxed[Logger^{this}] = Boxed(this.this_l)

But the problem with this unsound example is that, the reference to l is boxed, so l is not captured by the class. Therefore, the above rewriting mismatches with the actual situation.

This PR proposes a simple fix, which simply disable addOuterRefs when box adaptation happens. This is of course imprecise, but all tests pass with this fix.

@Linyxus Linyxus requested a review from odersky December 21, 2023 14:10
@Linyxus Linyxus assigned odersky and Linyxus and unassigned odersky Dec 21, 2023
@Linyxus
Copy link
Contributor Author

Linyxus commented Dec 21, 2023

I'll look into the failed test first.

@Linyxus Linyxus assigned odersky and unassigned Linyxus Jan 7, 2024
@Linyxus
Copy link
Contributor Author

Linyxus commented Jan 7, 2024

The previously-failing test seems to be a random incident. It passes after re-running. This PR is ready for review.

@odersky odersky merged commit b2ba6dc into scala:main Jan 11, 2024
@odersky odersky deleted the fix-19315 branch January 11, 2024 22:16
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
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.

How subcapturing works for this is unsound
3 participants