Skip to content
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

Merged
merged 3 commits into from
Jun 20, 2020
Merged

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 30, 2020

Merges test/ui/coerce with test/ui/coercion.
Updates the documentation of librustc_typeck/check/coercion.rs.
Adds 2 new coercion tests.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2020
@lcnr lcnr force-pushed the coerce-refactor branch 2 times, most recently from 13e6a47 to 3eec686 Compare June 6, 2020 10:22
@lcnr
Copy link
Contributor Author

lcnr commented Jun 6, 2020

@estebank I reverted the change fromCoerce to CoerceCtxt as it doesn't fit too well with the rest of this PR.

@bors
Copy link
Contributor

bors commented Jun 19, 2020

☔ The latest upstream changes (presumably #73486) made this pull request unmergeable. Please resolve the merge conflicts.

@lcnr lcnr changed the title Refactor coerce update coerce docs and unify relevant tests Jun 19, 2020
Copy link
Contributor

@estebank estebank left a 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`.
Copy link
Contributor

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?

Copy link
Contributor Author

@lcnr lcnr Jun 19, 2020

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`.

Copy link
Contributor

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:

//! ## 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.

Copy link
Contributor Author

@lcnr lcnr Jun 19, 2020

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.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2020

📌 Commit 06a237f has been approved by estebank

@bors bors added S-waiting-on-bors 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 Jun 19, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2020
…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
@bors bors merged commit c0a25be into rust-lang:master Jun 20, 2020
@lcnr lcnr deleted the coerce-refactor branch June 20, 2020 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants