-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Extend Polonius fact generation for (some) move tracking #62800
Extend Polonius fact generation for (some) move tracking #62800
Conversation
This comment has been minimized.
This comment has been minimized.
(for now!) |
This comment has been minimized.
This comment has been minimized.
Ping from triage. This PR is in need of review. Should it be re-assigned? @nikomatsakis @cramertj |
@joelpalmer Thanks for asking, but it's still very WIP; the fact generation is currently slightly wrong. I expect to fix it this week, but there are probably more issues than that hiding behind that issue. However, before this PR can be merged, Polonius needs to be updated from another branch (I will rebase and fixup the commit using my local Polonius to in stead pull the new release). So in short; I suspect @nikomatsakis might be back from their vacation before I'm done. If not, I'll ask around in the Polonius working group. Please don't reassign for now, thank you! |
☔ The latest upstream changes (presumably #61393) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
This pull request is waiting for rust-lang/polonius#110 to be merged into Polonius. |
This comment has been minimized.
This comment has been minimized.
Thank you for your update, |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #63579) made this pull request unmergeable. Please resolve the merge conflicts. |
- var_starts_path - parent - initialized_at - moved_out_at This also switches to the intended emission of `var_drop_used` fact emission, where that fact is always emitted on a drop-use of a variable, regardless of its initialization status, as Polonius now handles that.
Ready for review, as rust-lang/polonius#110 has landed! 🎉 This should be enough facts to implement move analysis and liveness tracking in Polonius, meaning that an entire new part of the Polonius analysis is finally fully unlocked! |
This comment has been minimized.
This comment has been minimized.
all_facts.var_starts_path.extend(move_data.rev_lookup.iter_locals_enumerated().map(|(v, &m)| (v, m))); | ||
|
||
for (idx, move_path) in move_data.move_paths.iter_enumerated() { | ||
all_facts.parent.extend(move_path.parents(&move_data.move_paths).iter().map(|&parent| (parent, idx))); |
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.
as noted in the polonius PR, ultimately I think we should leave the transitive computation here to polonius. But I guess I don't wnat to do that in this PR.
// The initialization happened in (or rather, when arriving at) | ||
// the successors, but not in the unwind block. | ||
let first_statement = Location { block: successor, statement_index: 0}; | ||
all_facts.initialized_at.push((init.path, location_table.start_index(first_statement))); |
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.
Interesting quirk. I guess this makes sense.
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.
Let's just say I scratched my head for quite some time before I figured out why this happened. The results were completely wrong otherwise. If you are curious about the details, there's a ton of logs on Zulip where I figure this out in the initialization topic.
self.path_accessed_at.push((path, self.location_to_index(location))); | ||
} | ||
|
||
fn place_to_mpi(&self, place: &Place<'_>) -> Option<MovePathIndex> { |
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.
might be nice to have some docs here -- when does this return None
?
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 guess it is when the place
is rooted in a static, though
I'm not going to block on those nits. Doesn't seem worth it. |
@bors r+ |
📌 Commit 28312b5 has been approved by |
⌛ Testing commit 28312b5 with merge b6eac35c6f9719fb329d9f6c259eda2c47f6fff8... |
💔 Test failed - checks-azure |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry probably spurious |
Extend Polonius fact generation for (some) move tracking This PR will extend rustc to emit facts used for tracking moves and initialization in Polonius. It is most likely the final part of my master's thesis work.
☀️ Test successful - checks-azure |
This PR will extend rustc to emit facts used for tracking moves and initialization in Polonius. It is most likely the final part of my master's thesis work.