-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Represent read-only with a classifier #24295
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
Conversation
`x.rd` is now a shorthand for `x.only[caps.Read]`.
|
It is surprising that this PR also fixes #24334. |
|
@natsukagami ping for review |
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.
I was slightly concerned with readonly not being a .only[Read] if we wanted to drop the cannot-capture-different-classifier restriction, but since we will take another route I think it's fine.
| */ | ||
| object ReadOnly: | ||
| def apply(underlying: CoreCapability | RootCapability | Reach | Restricted)(using Context): Restricted = | ||
| Restricted(underlying.stripRestricted.asInstanceOf, defn.Caps_Read) |
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.
Is it valid here to strip the existing restricted? e.g. if the classifiers was extended with a sub-classifier of Read
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 don't support restrictions with more than one classifierr, so the only issue here is that we might want to keep the existing sub-classifier of Read (if such things exist). But I think that should be handled by the caller.
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.
But we said Mutable should be a special "classifier" that allows its instance to capture anything. So we might well have a matrix m: Matrix^{fresh1, async} that captures a control capability Async (here the fresh1 is the Mutable fresh for this matrix). Then, getting readonly on m.only[Control].rd should, by principle, be empty. But with the current logic it will be m.rd or equivalently fresh1.rd.
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.
After a second thought I think it should be fine. This expressiveness issue could be solved by first widening m.only[Control] to just async and then take readonly on 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.
After a second thought I think it should be fine. This expressiveness issue could be solved by first widening m.only[Control] to just async and then take readonly on it, which will be empty.
x.rdis now a shorthand forx.only[caps.Read].