-
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
cleanup promoteds move check #134455
cleanup promoteds move check #134455
Conversation
4f94860
to
f936122
Compare
compiler/rustc_borrowck/src/lib.rs
Outdated
let promoted_body = &promoted[idx]; | ||
// FIXME: This assumes that we don't try to access most fields of the `promoted_mbcx`. | ||
// We may want to move `MoveError` construction and reporting out of `MirBorrowckCtxt`. | ||
let move_data = MoveData::gather_moves(body, tcx, |_| true); |
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.
shouldn't this be promoted_body
?
let move_data = MoveData::gather_moves(body, tcx, |_| true); | |
let move_data = MoveData::gather_moves(promoted_body, tcx, |_| true); |
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.
If so, then we probably need a test that actually exercises this, because the fact that this didn't fail makes me very nervous lol
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.
oh wow, yeah, that's... interesting
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.
from what I can tell the answer is: yes, iut should clearly be promoted_body
and also: it is entirely unused by check_movable_place
which is very gamer
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.
gonna do the refactor from my fixme i guess, because that's sus
// FIXME: This assumes that we don't try to access most fields of the `promoted_mbcx`.
// We may want to move `MoveError` construction and reporting out of `MirBorrowckCtxt`.
r? compiler-errors |
f936122
to
fc9a14d
Compare
@rustbot ready
Moving this error reporting out of the |
@bors r+ |
…ompiler-errors cleanup promoteds move check r? types
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#134455 (cleanup promoteds move check) - rust-lang#135421 (Make tidy warn on unrecognized directives) - rust-lang#135611 (Remove unnecessary assertion for reference error) - rust-lang#135620 (ci: improve github action name) - rust-lang#135621 (Move some std tests to integration tests) - rust-lang#135639 (new solver: prefer trivial builtin impls) - rust-lang#135654 (add src/librustdoc and src/rustdoc-json-types to RUSTC_IF_UNCHANGED_ALLOWED_PATHS) r? `@ghost` `@rustbot` modify labels: rollup
…piler-errors cleanup promoteds move check r? types
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry fuchsia not working |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#134455 (cleanup promoteds move check) - rust-lang#135421 (Make tidy warn on unrecognized directives) - rust-lang#135611 (Remove unnecessary assertion for reference error) - rust-lang#135620 (ci: improve github action name) - rust-lang#135639 (new solver: prefer trivial builtin impls) - rust-lang#135654 (add src/librustdoc and src/rustdoc-json-types to RUSTC_IF_UNCHANGED_ALLOWED_PATHS) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#134455 - lcnr:move-errors-in-promoteds, r=compiler-errors cleanup promoteds move check r? types
r? types