-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add path support for capture checking #21445
Conversation
@@ -122,10 +122,6 @@ object CheckCaptures: | |||
* This check is performed at Typer. | |||
*/ | |||
def checkWellformed(parent: Tree, ann: Tree)(using Context): Unit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this check removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When implementing the path, I considered the rule for a singleton type with a capture set. I don't think this check is necessary, as we can already achieve the same result by creating a type alias with a capture set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are saying the rule is incomplete? But then maybe we should dealias before we check? What is the argument for removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is it is okey to let users add a capture set to a singleton type, if they just want to explicitly indicate what the underlying type capturing (for example, in the member function signature def f: this.file^{this.io}
).
@@ -99,25 +100,56 @@ trait CaptureRef extends TypeProxy, ValueType: | |||
* x: x1.type /\ x1 subsumes y ==> x subsumes y | |||
*/ | |||
final def subsumes(y: CaptureRef)(using Context): Boolean = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to get better reassurance that this does the right thing. It looks too complicated to be able to argue it's obviously correct. What would help:
- A formal set of rules for subcapturing.
- A rendition of these rules as a doc comment for this method. It's a shame that the old doc comment was not updated in any way.
- More extensive tests that try to test all combinations. I saw we have neg tests but no new pos tests.
I tried this version of the old final def subsumesOld(y: CaptureRef)(using Context): Boolean =
(this eq y)
|| this.isRootCapability
|| y.match
case y: TermRef =>
y.prefix.match
case ypre: CaptureRef =>
this.subsumes(ypre)
|| this.match
case x @ TermRef(xpre: CaptureRef, _) =>
x.symbol == y.symbol && xpre =:= ypre
case _ =>
false
case _ => false
|| y.info.match
case y1: SingletonCaptureRef => this.subsumes(y1)
case _ => false
case MaybeCapability(y1) => this.stripMaybe.subsumes(y1)
case _ => false
|| this.match
case ReachCapability(x1) => x1.subsumes(y.stripReach)
case x: TermRef =>
x.info match
case x1: SingletonCaptureRef => x1.subsumes(y)
case _ => false
case x: TermParamRef => subsumesExistentially(x, y)
case x: TypeRef => assumedContainsOf(x).contains(y)
case _ => false There was no difference in outcomes, and the version here is shorter and clearer. So we should probably use this, unless we have tests that show there is a difference and the version here is wrong. |
I would have expected more changes in the recheckSelect rule. Indeed, recheckSelect seems to not make use of path types. Here is a simple test program: class IO
class C(val f: IO^)
def test(io: IO^) =
val c = C(io)
val g = () => println(c.f) This types EDIT: We probably also need to change markFree, so that it stores path types instead of their prefixes. The problem seems to be that we already marlk |
Also, I tink in light of path types, there's scope for a simplification of |
Generally, what's missing is a suite of tests that shows how path-dependent types give a more expressive language. Show code that did not typecheck before, but now does. |
There's a problem with pattern matching shown in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
This is done for comparing old with new
Add the path cases without changing the whole logic
If we refer to a path `a.b`, we should mark `a.b` as used, which is better than marking `a`.
Needed to make stdlib2-cc go through. There were two errors. One in LayListIterable required a type annotation and a tweak to markFree. The other in Vieew.scala required a cast, but this could be fixed with better handling of pattern matching. path-patmat-should-be-pos.scala is a minimization.
Rebased to main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I think we can merge this now.
No description provided.