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

Choose predicates without inference variables over those with them #55453

Merged
merged 1 commit into from Nov 7, 2018

Conversation

Projects
None yet
5 participants
@Aaron1011
Contributor

Aaron1011 commented Oct 28, 2018

Fixes #54705

When constructing synthetic auto trait impls, we may come across
multiple predicates involving the same type, trait, and substitutions.
Since we can only display one of these, we pick the one with the 'most
strict' lifetime paramters. This ensures that the impl we render the
user is actually valid (that is, a struct matching that impl will
actually implement the auto trait in question).

This commit exapnds the definition of 'more strict' to take into account
inference variables. We always choose a predicate without inference
variables over a predicate with inference variables.

Choose predicates without inference variables over those with them
Fixes #54705

When constructing synthetic auto trait impls, we may come across
multiple predicates involving the same type, trait, and substitutions.
Since we can only display one of these, we pick the one with the 'most
strict' lifetime paramters. This ensures that the impl we render the
user is actually valid (that is, a struct matching that impl will
actually implement the auto trait in question).

This commit exapnds the definition of 'more strict' to take into account
inference variables. We always choose a predicate without inference
variables over a predicate with inference variables.
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 28, 2018

r? @michaelwoerister

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

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Nov 1, 2018

Thanks for the PR, @Aaron1011!
I think I'll hand this off to @pnkfelix for re-assignment since I don't know the logic involved here very well.
r? @pnkfelix

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 6, 2018

@Aaron1011 you said here #54705 (comment) that you had a minimal reproduction, and it does not appear to utilize rustdoc.

The test you have included on this PR does require using rustdoc ...

(Meanwhile the minimal "reproduction" does not cause an error in play, so maybe I've misunderstood how it works or what its reproducing...)

  • Update: Oh, the minimal reproduction must be something you are intending to be fed into rustdoc ...

Anyway: Can you provide a unit test that does not require rustdoc; i.e. one in either ui or run-pass? I know in my own workflow that i don't tend to run the rustdoc tests locally, but I always run the suite of ui+compile-fail+run-pass before I attempt to post a PR.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 6, 2018

Anyway this seems fine, apart from the question about the current test.

r=me once that question is answered.

@Aaron1011

This comment has been minimized.

Contributor

Aaron1011 commented Nov 6, 2018

@pnkfelix: I've only incliuded a rustdoc test because AutoTraitFinder is only used by rustdoc. A run-pass test wouldn't actually exercise any of the code I've changed, since AutoTraitFinder is only used to synthesize impls for rustdoc to render.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 7, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 7, 2018

📌 Commit 20aa751 has been approved by pnkfelix

@bors

This comment has been minimized.

Contributor

bors commented Nov 7, 2018

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #55549
@bors

This comment has been minimized.

Contributor

bors commented Nov 7, 2018

📌 Commit 20aa751 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 7, 2018

Accidentally did rollup originally and editing my comment didn’t undo it. Going to r- and then r+

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 7, 2018

@bors r-

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 7, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 7, 2018

📌 Commit 20aa751 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 7, 2018

Hmmm

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 7, 2018

@bors rollup-

kennytm added a commit to kennytm/rust that referenced this pull request Nov 7, 2018

Rollup merge of rust-lang#55453 - Aaron1011:fix/rustdoc-lifetime, r=p…
…nkfelix

Choose predicates without inference variables over those with them

Fixes rust-lang#54705

When constructing synthetic auto trait impls, we may come across
multiple predicates involving the same type, trait, and substitutions.
Since we can only display one of these, we pick the one with the 'most
strict' lifetime paramters. This ensures that the impl we render the
user is actually valid (that is, a struct matching that impl will
actually implement the auto trait in question).

This commit exapnds the definition of 'more strict' to take into account
inference variables. We always choose a predicate without inference
variables over a predicate with inference variables.

bors added a commit that referenced this pull request Nov 7, 2018

Auto merge of #55746 - kennytm:rollup, r=kennytm
Rollup of 15 pull requests

Successful merges:

 - #55377 (Slight copy-editing for `std::cell::Cell` docs)
 - #55441 (Remove unused re import in gdb_rust_pretty_printing)
 - #55453 (Choose predicates without inference variables over those with them)
 - #55495 (Don't print opt fuel messages to stdout because it breaks Rustbuild)
 - #55501 (Make `process_obligations`' computation of `completed` optional.)
 - #55510 (Fix feature gate only being checked on first repr attr.)
 - #55609 (Run name-anon-globals after LTO passes as well)
 - #55688 (Standardised names and location of ui issue tests)
 - #55692 (-C remark: fix incorrect warning about requiring "--debuginfo" instead of "-C debuginfo=n")
 - #55702 (Add `aarch64-pc-windows-msvc` to deployed targets)
 - #55717 (Bubble up an overflow error so that rustdoc can ignore it)
 - #55719 (Sidestep link error from rustfix'ed code by using a *defined* static.)
 - #55728 (Update lldb)
 - #55730 (Use trait impl method span when type param mismatch is due to impl Trait)
 - #55734 (refactor: use shorthand fields)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Nov 7, 2018

Auto merge of #55746 - kennytm:rollup, r=kennytm
Rollup of 14 pull requests

Successful merges:

 - #55377 (Slight copy-editing for `std::cell::Cell` docs)
 - #55441 (Remove unused re import in gdb_rust_pretty_printing)
 - #55453 (Choose predicates without inference variables over those with them)
 - #55495 (Don't print opt fuel messages to stdout because it breaks Rustbuild)
 - #55501 (Make `process_obligations`' computation of `completed` optional.)
 - #55510 (Fix feature gate only being checked on first repr attr.)
 - #55609 (Run name-anon-globals after LTO passes as well)
 - #55645 (do not print wrapping ranges like normal ranges in validity diagnostics)
 - #55688 (Standardised names and location of ui issue tests)
 - #55692 (-C remark: fix incorrect warning about requiring "--debuginfo" instead of "-C debuginfo=n")
 - #55702 (Add `aarch64-pc-windows-msvc` to deployed targets)
 - #55728 (Update lldb)
 - #55730 (Use trait impl method span when type param mismatch is due to impl Trait)
 - #55734 (refactor: use shorthand fields)

@bors bors merged commit 20aa751 into rust-lang:master Nov 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

alexreg added a commit to alexreg/rust that referenced this pull request Nov 7, 2018

Auto merge of rust-lang#55746 - kennytm:rollup, r=kennytm
Rollup of 14 pull requests

Successful merges:

 - rust-lang#55377 (Slight copy-editing for `std::cell::Cell` docs)
 - rust-lang#55441 (Remove unused re import in gdb_rust_pretty_printing)
 - rust-lang#55453 (Choose predicates without inference variables over those with them)
 - rust-lang#55495 (Don't print opt fuel messages to stdout because it breaks Rustbuild)
 - rust-lang#55501 (Make `process_obligations`' computation of `completed` optional.)
 - rust-lang#55510 (Fix feature gate only being checked on first repr attr.)
 - rust-lang#55609 (Run name-anon-globals after LTO passes as well)
 - rust-lang#55645 (do not print wrapping ranges like normal ranges in validity diagnostics)
 - rust-lang#55688 (Standardised names and location of ui issue tests)
 - rust-lang#55692 (-C remark: fix incorrect warning about requiring "--debuginfo" instead of "-C debuginfo=n")
 - rust-lang#55702 (Add `aarch64-pc-windows-msvc` to deployed targets)
 - rust-lang#55728 (Update lldb)
 - rust-lang#55730 (Use trait impl method span when type param mismatch is due to impl Trait)
 - rust-lang#55734 (refactor: use shorthand fields)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment