-
Notifications
You must be signed in to change notification settings - Fork 13.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
refinement typing go brrrrrr #90016
refinement typing go brrrrrr #90016
Conversation
bbfea47
to
d8966af
Compare
This comment has been minimized.
This comment has been minimized.
src/test/mir-opt/boxy_thing/otherwise_refinements.foo.BoxyThing.diff
Outdated
Show resolved
Hide resolved
a53e13e
to
9f4beb3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
03b6261
to
a5bcda9
Compare
This comment has been minimized.
This comment has been minimized.
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.
Probably a good idea to squash some (most?) commits
SetDiscriminant { place, .. } => trans.clear(place.local), | ||
Assign(box (lhs_p, rhs)) => { | ||
trans.clear(lhs_p.local); | ||
if let Rvalue::Ref(_, BorrowKind::Mut { .. }, ref_p) = rhs { |
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.
Probably need to do the same for AddressOf. Also immut borrows can be a problem with interior mut
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 don't think interior mut is problematic though im having trouble explaining why I think that 🤔
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 had the same reaction as you, but couldn't explain it. Some thinking lead me to believe the reason is that interior mutability means you can only access the data via methods, so you lose all refinements anyway.
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 am going to just clear refinements on AddressOf
even when its not mutable to be on the safe side. I think that even though it seems unproblematic i don't trust 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.
Looking at how UnsafeCell
is implemented in core
I think that its probably fine right now given the combination of no refinements on projections and only being able to get at the inner value through methods. This all feels rather sketchy though...
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.
The current implementation also makes implicit assumption that an assignment invalidates all existing borrows.
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 this assumption not correct
6364ac8
to
19898d6
Compare
for refine in self.0.iter_mut() { | ||
*refine = Some(RefineRange::from(0..=(u128::MAX))); | ||
} |
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.
for refine in self.0.iter_mut() { | |
*refine = Some(RefineRange::from(0..=(u128::MAX))); | |
} | |
self.0.fill(Some(RefineRange::from(0..=u128::MAX))); |
my understanding is that this is now: @rustbot label -S-waiting-on-review +S-waiting-on-author (btw I miss the meaning implied by the title of this PR) |
☔ The latest upstream changes (presumably #91469) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: |
just gonna close this because im tired and dont want to think about this PR lol |
r? @oli-obk