Skip to content

Conversation

Aaron1011
Copy link
Contributor

Commit f57247c (Ensure that Rusdoc discovers all necessary auto
trait bounds) added a check to ensure that we only attempt to unify a
projection predicatre with inference variables. However, the check it
added was too strict - instead of checking that a type contains an
inference variable (e.g. '&', 'MyType<>'), it required the type to
be an inference variable (i.e. only '_' would match).

This commit relaxes the check to use 'ty.has_infer_types', ensuring that
we perform unification wherever possible.

Fixes #56822

Commit f57247c (Ensure that Rusdoc discovers all necessary auto
trait bounds) added a check to ensure that we only attempt to unify a
projection predicatre with inference variables. However, the check it
added was too strict - instead of checking that a type *contains* an
inference variable (e.g. '&_', 'MyType<_>'), it required the type to
*be* an inference variable (i.e. only '_' would match).

This commit relaxes the check to use 'ty.has_infer_types', ensuring that
we perform unification wherever possible.

Fixes rust-lang#56822
@rust-highfive
Copy link
Contributor

r? @eddyb

(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 Dec 15, 2018
@GuillaumeGomez
Copy link
Member

Awesome, thanks!

@eddyb
Copy link
Member

eddyb commented Dec 21, 2018

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Dec 21, 2018
@SimonSapin
Copy link
Contributor

Thanks for this fix @Aaron1011! This is blocking upgrading the compiler in Servo (which is needed because of unrelated changes to unstable features).

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 27, 2018

📌 Commit a375410 has been approved by nikomatsakis

@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 Dec 27, 2018
@nikomatsakis
Copy link
Contributor

@bors p=1 -- blocking servo

@nikomatsakis
Copy link
Contributor

@Aaron1011 #56822 is not tagged as a regression, so I am assuming no beta backport is required here?

@bors
Copy link
Collaborator

bors commented Dec 27, 2018

⌛ Testing commit a375410 with merge f2b9217...

bors added a commit that referenced this pull request Dec 27, 2018
Call poly_project_and_unify_type on types that contain inference types

Commit f57247c (Ensure that Rusdoc discovers all necessary auto
trait bounds) added a check to ensure that we only attempt to unify a
projection predicatre with inference variables. However, the check it
added was too strict - instead of checking that a type *contains* an
inference variable (e.g. '&_', 'MyType<_>'), it required the type to
*be* an inference variable (i.e. only '_' would match).

This commit relaxes the check to use 'ty.has_infer_types', ensuring that
we perform unification wherever possible.

Fixes #56822
@bors
Copy link
Collaborator

bors commented Dec 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing f2b9217 to master...

@bors bors merged commit a375410 into rust-lang:master Dec 27, 2018
@SimonSapin
Copy link
Contributor

@nikomatsakis #56822 is a regression in the sense that cargo doc started failing with a compiler/toolchain upgrade and no code change. I forgot to add that label when I filed the issue.

As to backporting, I first reproduced in 1.32.0-nightly (f4a421e 2018-12-13) which suggests we should backport this fix to 1.32 beta, but somehow I can’t reproduce in 1.32.0-beta.6 (a01b0bf 2018-12-19) so I’m not sure.

@SimonSapin
Copy link
Contributor

That Nightly is the first after there hadn’t been one for a few days, so the regression range is kinda large:

git log --oneline --merges 4a45578...f4a421e
f4a421e Auto merge of #56783 - alexcrichton:pinentry-mode, r=Mark-Simulacrum
7489ee9 Auto merge of #56461 - oli-obk:alloc_ids, r=RalfJung
9fe5cb5 Auto merge of #56161 - RalfJung:vecdeque-stacked-borrows, r=SimonSapin
ced7cc5 Auto merge of #56090 - nnethercote:filesearch, r=eddyb
2f35a10 Auto merge of #55982 - alexcrichton:panic-extern-abort, r=zackmdavis
0076f58 Auto merge of #55992 - cramertj:pin-docs, r=alexcrichton
dd8fc7d Auto merge of #56735 - Mark-Simulacrum:fix-sign, r=alexcrichton
bd47d68 Auto merge of #56092 - alexcrichton:no-more-std-subodules, r=Mark-Simulacrum
ae3833d Auto merge of #56039 - ljedrz:sorted_map_upgrades, r=matthewjasper
a64cdec Auto merge of #56010 - euclio:intra-doc-spans, r=QuietMisdreavus
8375ab4 Auto merge of #53497 - fukatani:test-debuginfo-function-call, r=tromey
3499575 Auto merge of #56243 - RalfJung:test-deterministic, r=alexcrichton
3a31213 Auto merge of #56703 - alexcrichton:fix-tools, r=Mark-Simulacrum
4c0116e Auto merge of #56627 - alexcrichton:update-cargo, r=alexcrichton
da1527c Auto merge of #56688 - GuillaumeGomez:rollup, r=GuillaumeGomez
a11de41 Rollup merge of #56661 - aelred:issue-55846, r=Mark-Simulacrum
b3f1650 Rollup merge of #56656 - BeatButton:liballoc_string_typo, r=Centril
b37ad66 Rollup merge of #56641 - GuillaumeGomez:span-trait-method-invalid-nb-parameters, r=estebank
33bf291 Rollup merge of #56633 - GuillaumeGomez:fix-right-arrow-display, r=QuietMisdreavus
dec7b19 Rollup merge of #56491 - euclio:assert-error, r=estebank
1137d29 Auto merge of #56666 - Xanewok:rustfmt, r=kennytm
3a75e80 Auto merge of #56157 - RalfJung:park, r=nagisa
9567a1c Auto merge of #56624 - RalfJung:miri, r=oli-obk
286dc37 Auto merge of #56369 - nnethercote:rm-Delimited, r=petrochenkov
e2c329c Auto merge of #56269 - nnethercote:_match-Matrix-SmallVec, r=simulacrum
9cb38a8 Auto merge of #56463 - ljedrz:slice_concat_join, r=nikic
b755501 Auto merge of #56444 - petrochenkov:uifull, r=davidtwco
850fc6a Auto merge of #56644 - jens1o:patch-1, r=pietroalbini
ea007c6 Auto merge of #56631 - matthiaskrgr:clippy, r=nikic
b7da2c6 Auto merge of #56630 - sinkuu:core_iter, r=kennytm
d7a9d96 Auto merge of #56615 - integer32llc:update-book, r=GuillaumeGomez
8db2342 Auto merge of #56616 - estebank:issue-56539, r=davidtwco
bdef56a Auto merge of #56632 - Eijebong:synup, r=Mark-Simulacrum
9772d02 Auto merge of #56623 - Centril:rollup, r=Centril
a8cc916 Rollup merge of #56621 - Morganamilo:fix-generators-comma, r=Centril
9f7f949 Rollup merge of #56620 - petrochenkov:noclutter, r=estebank
7f076fa Rollup merge of #56602 - dwijnand:fix-ptr-hash-docs, r=Centril
eb30d56 Rollup merge of #56599 - dlrobertson:fix_va_arg, r=eddyb
253c448 Rollup merge of #56597 - vext01:dump-mir-usage, r=wesleywiser
ac15b4f Rollup merge of #56248 - estebank:suggest-bare-pub, r=petrochenkov
1ccb5b2 Auto merge of #56583 - RalfJung:vergen, r=oli-obk
059e6a6 Auto merge of #56578 - alexreg:cosmetic-1, r=alexreg
0a77980 Auto merge of #56258 - euclio:fs-read-write, r=euclio

@Aaron1011 Aaron1011 deleted the fix/rustdoc-infer-unify branch December 27, 2018 18:54
@kinnison
Copy link
Contributor

@nikomatsakis #56822 is a regression in the sense that cargo doc started failing with a compiler/toolchain upgrade and no code change. I forgot to add that label when I filed the issue.

As to backporting, I first reproduced in 1.32.0-nightly (f4a421e 2018-12-13) which suggests we should backport this fix to 1.32 beta, but somehow I can’t reproduce in 1.32.0-beta.6 (a01b0bf 2018-12-19) so I’m not sure.

I can confirm that this fix (or at least everything from my previous testing up to fb86d60) corrects the problem I reported in #56296 which was found in 1.30, 1.31, and 1.32.0-nightly-2018-11-27 at least.

As such, I'd very much like to request the fix be backported to 1.32 if there's time, as it will unblock some docs work I wanted to get done in the new year.

@SimonSapin SimonSapin added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 28, 2018
@SimonSapin
Copy link
Contributor

Not sure who normally gets to do that, but tagged as beta-nominated.

@QuietMisdreavus QuietMisdreavus added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Dec 30, 2018
@QuietMisdreavus
Copy link
Contributor

Accepting beta nomination based on the fact that #56296 is affecting stable.

@pietroalbini
Copy link
Member

This doesn't apply cleanly to beta, since the code on the beta branch is different. Can someone prepare a backport PR against beta? Thanks!

@SimonSapin
Copy link
Contributor

@pietroalbini Is that the same bug? I could not reproduce #56822 on 1.32.0-beta.6 (and it never reached stable as far as I know).

@pietroalbini
Copy link
Member

Dunno, just saw the beta-accepted label and tried to backport it.

@kinnison
Copy link
Contributor

kinnison commented Jan 1, 2019

This doesn't apply cleanly to beta, since the code on the beta branch is different. Can someone prepare a backport PR against beta? Thanks!

@Aaron1011 is there any chance of that, or has too much changed?

@pietroalbini
Copy link
Member

Ping @nikomatsakis @Aaron1011 @QuietMisdreavus, can someone prepare a beta backport PR?

@Aaron1011
Copy link
Contributor Author

@pietroalbini: This PR builds off of #55318 - both are necessary to fix the underlying issue. Should I prepare a PR containing the commits from this PR and #55318 ?

@pietroalbini
Copy link
Member

That PR will need to be approved for backport as well then.

@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 3, 2019
bors added a commit that referenced this pull request Jan 4, 2019
[beta] Rollup backports

Cherry-picked:

* #57053: Fix alignment for array indexing
* #57181: resolve: Fix another ICE in import validation
* #57185: resolve: Fix one more ICE in import validation
* #57282: Wf-check the output type of a function in MIR-typeck
* #55318: Ensure that Rustdoc discovers all necessary auto trait bounds
* #56838: Call poly_project_and_unify_type on types that contain inference types

Rolled up:

* #57300: [beta] Update RLS to include 100% CPU on hover bugfix
* #57301: beta: bootstrap from latest stable (1.31.1)
* #57292: [BETA] Update cargo

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants