-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Simplify Level Checking and Fix Errors #23934
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
base: main
Are you sure you want to change the base?
Conversation
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.
Overall LGTM!
There are two tests that look like an expressiveness problem. I investigated a bit, and I think it may be due to Capability.visibility
returns an owner that too narrow for local bindings. For instance for an object foo
in a function main
, its visibility will just be foo
itself. But by principle, it should be visible in the entire main
.
I tried to fix this by using levelOwner
for visibility
. This requires some tweaks to the tests. The commit for this change: dotty-staging@d8c44aa.
Most tweaks look reasonable to me, and it resolves the expressiveness problem. There are something in this change that looks suspicious to me. Like here a local c
in function body shows up in the hidden set inside function result. But it does not look unsound to me. I guess a more general question is what does it mean for a capture set to have an owner
? I have the intuition that for capture sets created under the same owner, there can be subtle differences for them too.
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.
This looks suspicious to me. Seems like a expressiveness problem.
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.
Also looks suspicious.
We map them to fresh caps but their hidden sets should not accept new elements.
Allows us to diagnose when a specific variable is created or obtains new elements.
The previous scheme was too coarse since it treated all vaks in a method the same.
Remove old level checking code and debug infra to compare old with new.
@Linyxus I don't think using def foo =
val a =
val b: C^ = ...
b That will add the invisible |
- In levelOwner, don't skip constructors. These are significant now. - Skip non-static modules when computing the visibility of FreshCaps
3fe6080
to
de35cbb
Compare
Drop the separate level number infrastructure (which was too coarse anyway) and base level checking exclsuively on owners.