Skip to content
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

A more flexible scheme for handling the universal capability #18699

Merged
merged 85 commits into from
Oct 23, 2023

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 15, 2023

  • Start with sealed type parameters, extended so that class parameters can also be sealed.
  • Tighten the rules so that type parameter arguments for sealed type parameters must themselves be sealed, except if the argument is from some outer scope.
  • Introduce a coercion that maps a universal capture set to a local root that is enclosed by the owners of all free variables of the coerced expression.

Supersedes #18566

This is needed if we want to do setup transforms as part of denotation transformers.
 1. Update symbols owner before recursing to children, so that new owners
    taking account of try block owners are installed.
 2. Query owner chaines at phase checkCpatures when computing enclosing level owners.
I tried several other strategies, but nothing worked.

 - update after Setup: This means we cannot to levelOwner during setup.
 - running levelOwner after Setup causes CyclicErrors when transforming symbols
   in Setup
 - having a seperate notion of ccOwner maintained in a CCState table does
   not work since there are too many hidden uses of owner everywhere that
   would ignore that table.
(except if they are updated). This is the first step towards converting
to capturing types in SymTransformer, where we have the context.
Need to copy the denotation, since denotations come with next pointers
which would get scrambled otherwise.
Makes things more regular and allows for a
non-identity mapping between declared types and infos of vals.
The new check is that a publicly visible inferred type after capture checking
must conform to the type before capture checking (which is also the type seen
by other separately compiled units).
Widen singleton types when instantiating local roots in checkConforms
Don't force info when checking whether something is a root capability while printing
Refactor everything so that now vals as well as defs can be level owners
Innstead of traversing old types at postCheck, note what needs to be done
when these types are first transformed at Setup.
With the new way to construct capturing types at Setup, we can add the correct
boxing annotations there, so the previous unmodular hack can be dropped.
Break out operation to add a single element. This makes widenCaptures
redundant.
Avoid the mutable state in CapturSet.Var
# Conflicts:
#	compiler/src/dotty/tools/dotc/cc/Setup.scala
Also: better parameter names for synthesized methods
Showing leak detection with simulated stdlib classes
@odersky
Copy link
Contributor Author

odersky commented Oct 23, 2023

@Linyxus I am going to merge now, so that we have a stable state for IWACO. When you have time, it would still be good to do a post-merge review.

@odersky odersky merged commit a940541 into scala:main Oct 23, 2023
18 checks passed
@odersky odersky deleted the cc-deep-cs branch October 23, 2023 17:28
@Linyxus
Copy link
Contributor

Linyxus commented Oct 27, 2023

I'm testing the new scheme and found the following won't compile:

import language.experimental.captureChecking
trait Cap:
  def use: Int = 42

def test2(cs: List[Cap^]): Unit =
  val t0: Cap^{cap[test2]} = cs.head  // error
  var t1: Cap^{cap[test2]} = cs.head  // error

@odersky
Copy link
Contributor Author

odersky commented Oct 27, 2023

Would be good to try some variants. Does it infer the right type if the type is not given?

@Linyxus
Copy link
Contributor

Linyxus commented Oct 27, 2023

Without a type being given, the inferred type is Cap^.

@odersky
Copy link
Contributor Author

odersky commented Oct 27, 2023

For the var as well?

@Linyxus
Copy link
Contributor

Linyxus commented Oct 27, 2023

It behaves differently but fails either: it creates a capture set variable on LHS, but since RHS is Cap^, capture set inference complains we are pushing a cap into the capture set of a variable.

@Linyxus
Copy link
Contributor

Linyxus commented Oct 27, 2023

I investigated into what happened. It turns out that when the input to adaptUniversal is a TermRef, for instance, cs.head : -> box Cap^, it get adapted into something like box (cs.head : -> box Cap^)^{cap[test2]}.

@odersky
Copy link
Contributor Author

odersky commented Oct 27, 2023

And why does that lead to a problem? It looks like we do adapt to capture set {cap[test2]}?

@Linyxus
Copy link
Contributor

Linyxus commented Oct 27, 2023

It seems that tp.widenDealias turns box (cs.head : -> box Cap^)^{cap[test2]} to box Cap^. .widenDealiased is called by adaptBoxed.

Besides, nested capture set annotations accumulate. So the effective boxed capture set of box (cs.head : -> box Cap^)^{cap[test2]} is {cap, cap[test2]}, which is equivalent to {cap} modulo subcapturing.

@odersky
Copy link
Contributor Author

odersky commented Oct 27, 2023

Yes, I guess here they should not accumulate. In fact I am not even sure that accumulation is correct in general. I think adaptBoxed should be changed to avoid widenDealias.

@odersky
Copy link
Contributor Author

odersky commented Oct 27, 2023

Or else do the widenDealias eagerly in adaptUniversal where we then strip the universal capture set .

odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 28, 2023
Linyxus added a commit that referenced this pull request Oct 30, 2023
@Linyxus
Copy link
Contributor

Linyxus commented Oct 30, 2023

I constructed an unsound code snippet with recursion:

import language.experimental.captureChecking
trait Cap:
  def use: Int = 42

def usingCap[sealed T](op: Cap^ => T): T = ???

def badTest(): Unit =
  def bad(b: Boolean)(c: Cap^): Cap^{cap[bad]} =
    if b then c
    else
      val leaked = usingCap[Cap^{cap[bad]}](bad(true))
      leaked.use  // boom
      c

  usingCap[Unit]: c0 =>
    bad(false)(c0)

The setup is classic. We have Cap, a capability, and usingCap, which creates a temporary local capability that has to be used within a scope.

The function bad manifests the problem. It takes a boolean and a capability. Depending on the boolean value it has two cases:

  • When b is true bad is just an identity. Note that the identity coerces the input capability onto the level of bad, as seen in the return type.
  • Otherwise, it calls usingCap with itself, setting b to true. Note that bad(true) is just an identity. It returns the capability on the level of bad, which is perfectly valid and usable in the current scope. However, it is actually a leaked capability and using it causes undefined behaviors.

Remarks:

  • I think the essence of the problem is that the meaning of scoping enforced by levels becomes vague when recursion comes into play.
  • This is a soundness hole, but seems to be a weird and rare case that is merely possible to be written by an actual user.
  • Does that rings a bell on the necessity of taking recursion into account in the formalism?

@odersky
Copy link
Contributor Author

odersky commented Oct 30, 2023

Can you translate the program to explicit levels? Does it typecheck then? Where would it go wrong?

@Linyxus
Copy link
Contributor

Linyxus commented Oct 30, 2023

At first glance with explicit levels that will not be a issue thanks to the polymorphism trick, but I'll verify.

@odersky
Copy link
Contributor Author

odersky commented Oct 30, 2023

I think one solution would be that we don't let the local root of a method appear in the external type of that method. So the following line would be in error:

def bad(b: Boolean)(c: Cap^): Cap^{cap[bad]} =

@odersky
Copy link
Contributor Author

odersky commented Oct 30, 2023

See 1065bd1 which fixes the problem and still allows the standard library to compile. It would be good if you could refine that further. (see commit message)

@Linyxus
Copy link
Contributor

Linyxus commented Oct 30, 2023

cool! Thanks. I'll look into it.

@Linyxus
Copy link
Contributor

Linyxus commented Nov 1, 2023

I looked into the commit and find it working in most cases I could come up with. Then I compared it with checkNoPrivateLeaks in Checking.scala which I believe checks the leaking of private types in class interfaces, but I could not figure out what is missing in the current commit. I wonder what specifically should be refined?

@odersky
Copy link
Contributor Author

odersky commented Nov 2, 2023

I think this should be rejected:

class C:
  object inner:
    val x: T^{cap[C]}  // error

But this should be OK:

class C:
  private object inner:
    val x: T^{cap[C]}  // ok

AFAICR that seems similar to what checkNoPrivate leaks does.

@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
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.

3 participants