-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
update coerce docs and unify relevant tests #72791
Conversation
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
13e6a47
to
3eec686
Compare
@estebank I reverted the change from |
☔ The latest upstream changes (presumably #73486) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Only one nitpick, r=me.
//! we may want to adjust precisely when coercions occur. | ||
//! foo(&mut 7i32, &7i32); | ||
//! // This does not compile, as we first infer `T` to be `&mut i32` | ||
//! // and are then unable to coerce `&7i32` to `&mut i32`. |
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.
Should we add an explanation for the foo::<&i32>(&7, &mut 7)
case?
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.
Hmm isn't foo::<&i32>(&7, &mut 7)
fairly trivial as there are no type variables at play here?
Or do you want me to add a more detailed explanation for foo(&7, &mut 7)
? My current explanation is
fairly short.
foo(&7i32, &mut 7i32);
// This compiles, as we first infer `T` to be `&i32`,
// and then coerce `&mut 7i32` to `&7i32`.
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'm referring at keeping part of the following info around, unless it is no longer valid:
rust/src/librustc_typeck/check/coercion.rs
Lines 34 to 51 in 72417d8
//! ## Subtler note | |
//! | |
//! However, right now, if the user manually specifies the | |
//! values for the type variables, as so: | |
//! | |
//! foo::<&int>(@1, @2) | |
//! | |
//! then we *will* auto-borrow, because we can't distinguish this from a | |
//! function that declared `&int`. This is inconsistent but it's easiest | |
//! at the moment. The right thing to do, I think, is to consider the | |
//! *unsubstituted* type when deciding whether to auto-borrow, but the | |
//! *substituted* type when considering the bounds and so forth. But most | |
//! of our methods don't give access to the unsubstituted type, and | |
//! rightly so because they'd be error-prone. So maybe the thing to do is | |
//! to actually determine the kind of coercions that should occur | |
//! separately and pass them in. Or maybe it's ok as is. Anyway, it's | |
//! sort of a minor point so I've opted to leave it for later -- after all, | |
//! we may want to adjust precisely when coercions occur. |
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 think this isn't relevant anymore as we now auto-borrow both foo::<&i32>(&7, &mut 7)
and foo(&7, &mut 7)
.
Afaict there was a time where we did not compile foo(&7, &mut 7)
to prevent the inconsistency we have rn (where foo(&7, &mut 7)
compiles and foo(&mut 7, &7)
does not).
So the fact that coercion doesn't distinguish foo<&i32>
from fn foo(_: &i32, _: &i32)
shouldn't matter anymore.
@bors r+ |
📌 Commit 06a237f has been approved by |
…arth Rollup of 16 pull requests Successful merges: - rust-lang#71420 (Specialization is unsound) - rust-lang#71899 (Refactor `try_find` a little) - rust-lang#72689 (add str to common types) - rust-lang#72791 (update coerce docs and unify relevant tests) - rust-lang#72934 (forbid mutable references in all constant contexts except for const-fns) - rust-lang#73027 (Make `need_type_info_err` more conservative) - rust-lang#73347 (Diagnose use of incompatible sanitizers) - rust-lang#73359 (shim.rs: avoid creating `Call` terminators calling `Self`) - rust-lang#73399 (Clean up E0668 explanation) - rust-lang#73436 (Clean up E0670 explanation) - rust-lang#73440 (Add src/librustdoc as an alias for src/tools/rustdoc) - rust-lang#73442 (pretty/mir: const value enums with no variants) - rust-lang#73452 (Unify region variables when projecting associated types) - rust-lang#73458 (Use alloc::Layout in DroplessArena API) - rust-lang#73484 (Update the doc for std::prelude to the correct behavior) - rust-lang#73506 (Bump Rustfmt and RLS) Failed merges: r? @ghost
Merges
test/ui/coerce
withtest/ui/coercion
.Updates the documentation of
librustc_typeck/check/coercion.rs
.Adds 2 new coercion tests.