suggest_borrow_generic_arg: instantiate clauses properly by dianne · Pull Request #133130 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits1 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This simplifies and fixes the way suggest_borrow_generic_arg instantiates callees' predicates when testing them to see if a moved argument can instead be borrowed. Previously, it would ICE if the moved argument's type included a region variable, since it was getting passed to a call of EarlyBinder::instantiate. This makes the instantiation much more straightforward, which also fixes the ICE.
Fixes #133118
This also modifies tests/ui/moves/moved-value-on-as-ref-arg.rs to have more useful bounds on the tests for suggestions to borrow Borrow and BorrowMut arguments. With its old tautological T: BorrowMut<T> bound, this fix would make it suggest a shared borrow for that argument.
r? @estebank
rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain that this is the right fix. If we're instantiating/substituting the wrong set of region variables into a type, then this is bound to happen with a generic type or const as well, right? Those cannot be erased.
I'd at least like to see an explanation for why this issue was happening before this fix; this feels like it's just masking the underlying issue which is that this code is handling generics kinda strangely in general.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real bug is that in clause_for_ref, you're substituting it once with the self type (ref_ty), then you're substituting it again in the call to predicate_must_hold_modulo_regions.
I don't think this really makes sense -- substituting it once should be sufficient, since you've already set up the generic args to have the right self type (new_args).
I hope that's enough info to guide you towards the right solution :) -- @rustbot author
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Fixes issue 133118.
This also modifies tests/ui/moves/moved-value-on-as-ref-arg.rs to have more
useful bounds on the tests for suggestions to borrow Borrow and BorrowMut
arguments. With its old tautological T: BorrowMut<T> bound, this fix would
make it suggest a shared borrow for that argument.
This approach looks correct. Please update the description and title, then I can approve
suggest_borrow_generic_arg: erase region variables from moved_arg_ty soonersuggest_borrow_generic_arg: instantiate clauses properly
Done. Thank you! That helped. I think it reads much nicer now too.
@rustbot ready
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
📌 Commit 546ba3d has been approved by compiler-errors
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
Rollup of 7 pull requests
Successful merges:
- rust-lang#132795 (Check
use<..>in RPITIT for refinement) - rust-lang#132944 (add parentheses when unboxing suggestion needed)
- rust-lang#132993 (Make rustc consider itself a stable compiler when
RUSTC_BOOTSTRAP=-1) - rust-lang#133130 (
suggest_borrow_generic_arg: instantiate clauses properly) - rust-lang#133133 (rustdoc-search: add standalone trailing
::test) - rust-lang#133143 (Diagnostics for let mut in item context)
- rust-lang#133147 (Fixup some test directives)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#133130 - dianne:fix-133118, r=compiler-errors
suggest_borrow_generic_arg: instantiate clauses properly
This simplifies and fixes the way suggest_borrow_generic_arg instantiates callees' predicates when testing them to see if a moved argument can instead be borrowed. Previously, it would ICE if the moved argument's type included a region variable, since it was getting passed to a call of EarlyBinder::instantiate. This makes the instantiation much more straightforward, which also fixes the ICE.
Fixes rust-lang#133118
This also modifies tests/ui/moves/moved-value-on-as-ref-arg.rs to have more useful bounds on the tests for suggestions to borrow Borrow and BorrowMut arguments. With its old tautological T: BorrowMut<T> bound, this fix would make it suggest a shared borrow for that argument.
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.