Ensure that Rustdoc discovers all necessary auto trait bounds #55318

Aaron1011 · 2018-10-24T15:51:12Z

This commit makes several improvements to AutoTraitFinder:

Call infcx.resolve_type_vars_if_possible before processing new
predicates. This ensures that we eliminate inference variables wherever
possible.
Process all nested obligations we get from a vtable, not just ones
with depth=1.
- The 'depth=1' check was a hack to work around issues processing
  certain predicates. The other changes in this commit allow us to
  properly process all predicates that we encounter, so the check is no
  longer necessary,
Ensure that we only display predicates without inference variables
to the user, and only attempt to unify predicates that have an
inference variable as their type.

Additionally, the internal helper method is_of_param now operates
directly on a type, rather than taking a Substs. This allows us to use
the 'self_ty' method, rather than directly dealing with Substs.

rust-highfive · 2018-10-24T15:51:17Z

r? @estebank

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

Aaron1011 · 2018-10-24T15:51:46Z

r? @GuillaumeGomez

GuillaumeGomez · 2018-10-24T20:13:51Z

Thanks a lot! However, considering that it changes things in librustc, I'll let someone from the @rust-lang/compiler team review it.

eddyb · 2018-11-05T08:39:59Z

r? @nikomatsakis

nikomatsakis · 2018-11-12T20:36:17Z

Sorry for being slow-ish, will get on this this week =)

nikomatsakis

Changes all seem good. I'm game to r+ but I have a few questions about the underlying logic.

nikomatsakis · 2018-11-14T20:08:17Z

src/librustc/traits/auto_trait.rs

-                    let substs = &p.skip_binder().trait_ref.substs;
+                    if self.is_of_param(p.skip_binder().self_ty())
+                        && !only_projections
+                        && is_new_pred {


Why don't we check for inference variables for the other parts of p?

That was an oversight on my part - I intended to check all of the parameters in substs, but forgot to change it. I've pushed a new commit fixing it.

nikomatsakis · 2018-11-14T20:09:38Z

src/librustc/traits/auto_trait.rs


-                    if self.is_of_param(substs) && !only_projections && is_new_pred {
                        self.add_user_pred(computed_preds, predicate);
                    }
                    predicates.push_back(p.clone());


Pre-existing, but can you explain to me why this adds a user-predicate (above) and then still pushes p back onto the list to be selected again?

Selecting p might result in other predicates (e.g. TypeOutlives or RegionOutlives) that are necessary for FulfillmentContext.select_all_or_error to succeed.

In general, AutoTraitFinder always tries to drive select all the way to the end. A subset of the predicates that it finds along the way are suitable for displaying to a user (e.g. rendering in docs), which we track separately.

Aaron1011 · 2018-11-16T23:39:08Z

Once rust-lang/crater#366 is deployed, it might be useful to do a Crater rustdoc run with this PR.

Aaron1011 · 2018-11-20T18:08:11Z

@nikomatsakis: Are there any other changes that you'd like me to make?

nikomatsakis

I'm happy but if you want to do a crater run, that sounds fine to me. I'm not sure if you can do rustdoc crater runs, I guess @pietroalbini would know

pietroalbini · 2018-11-26T21:54:32Z

Yeah, it's possible, just use mode=rustdoc.

Aaron1011 · 2018-11-26T22:20:44Z

@nikomatsakis @pietroalbini: I don't have permission to start a crater run - can one of you start one?

Mark-Simulacrum · 2018-11-27T00:38:10Z

Since I didn't see a try build here yet... @bors try for crater

bors · 2018-11-27T00:38:24Z

⌛ Trying commit 69e82c1 with merge f5a0bd7...

Ensure that Rustdoc discovers all necessary auto trait bounds Fixes #50159 This commit makes several improvements to AutoTraitFinder: * Call infcx.resolve_type_vars_if_possible before processing new predicates. This ensures that we eliminate inference variables wherever possible. * Process all nested obligations we get from a vtable, not just ones with depth=1. * The 'depth=1' check was a hack to work around issues processing certain predicates. The other changes in this commit allow us to properly process all predicates that we encounter, so the check is no longer necessary, * Ensure that we only display predicates *without* inference variables to the user, and only attempt to unify predicates that *have* an inference variable as their type. Additionally, the internal helper method is_of_param now operates directly on a type, rather than taking a Substs. This allows us to use the 'self_ty' method, rather than directly dealing with Substs.

bors · 2018-11-27T03:13:23Z

☀️ Test successful - status-travis
State: approved= try=True

pietroalbini · 2018-11-27T08:33:26Z

@craterbot run start=master#6bfb46e4ac9a2704f06de1a2ff7a4612cd70c8cb end=try#f5a0bd723553ea4b7556bd7087b9f0919cafb483 mode=rustdoc

craterbot · 2018-11-27T08:33:29Z

👌 Experiment pr-55318 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2018-11-27T08:33:31Z

🚧 Experiment pr-55318 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2018-11-28T17:17:45Z

🎉 Experiment pr-55318 is completed!
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

pietroalbini · 2018-11-30T20:55:33Z

@craterbot run start=master#147e60c5f89cfa2d3ffc247413956a37582c98e7 end=try#f86f76f89ea2b1ccbfb3741962ec2029d878a389 mode=rustdoc

craterbot · 2018-11-30T20:55:34Z

👌 Experiment pr-55318-1 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2018-12-01T02:35:00Z

🚧 Experiment pr-55318-1 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2018-12-02T10:53:05Z

🎉 Experiment pr-55318-1 is completed!
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Aaron1011 · 2018-12-02T21:48:15Z

The one 'regression' appears spurious.

@nikomatsakis: I believe this is ready to merge.

nikomatsakis · 2018-12-06T16:33:47Z

@bors r+

bors · 2018-12-06T16:33:49Z

📌 Commit 9139374 has been approved by nikomatsakis

nikomatsakis · 2018-12-06T16:33:54Z

@bors r+

bors · 2018-12-06T16:33:55Z

💡 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: Delay gensym creation for "underscore items" (use foo as _/const _) until name resolution #56392

bors · 2018-12-06T16:33:58Z

📌 Commit 9139374 has been approved by nikomatsakis

bors · 2018-12-06T22:23:30Z

⌛ Testing commit 9139374 with merge 5182cc1...

…matsakis Ensure that Rustdoc discovers all necessary auto trait bounds Fixes #50159 This commit makes several improvements to AutoTraitFinder: * Call infcx.resolve_type_vars_if_possible before processing new predicates. This ensures that we eliminate inference variables wherever possible. * Process all nested obligations we get from a vtable, not just ones with depth=1. * The 'depth=1' check was a hack to work around issues processing certain predicates. The other changes in this commit allow us to properly process all predicates that we encounter, so the check is no longer necessary, * Ensure that we only display predicates *without* inference variables to the user, and only attempt to unify predicates that *have* an inference variable as their type. Additionally, the internal helper method is_of_param now operates directly on a type, rather than taking a Substs. This allows us to use the 'self_ty' method, rather than directly dealing with Substs.

bors · 2018-12-07T00:47:55Z

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

pietroalbini · 2019-01-03T16:22:45Z

Ping @rust-lang/rustdoc, this PR is a prerequisite for backporting #56838, do you want to accept it for backport on beta?

QuietMisdreavus · 2019-01-03T17:01:07Z

Accepting beta nomination for the same reasons as #56838.

@ghost

[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

rust-highfive assigned estebank Oct 24, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2018

rust-highfive assigned GuillaumeGomez and unassigned estebank Oct 24, 2018

kennytm changed the title ~~Ensure that Rusdoc discovers all necessary auto trait bounds~~ Ensure that Rustdoc discovers all necessary auto trait bounds Oct 24, 2018

rust-highfive assigned nikomatsakis and unassigned GuillaumeGomez Nov 5, 2018

nikomatsakis approved these changes Nov 14, 2018

View reviewed changes

nikomatsakis reviewed Nov 14, 2018

View reviewed changes

nikomatsakis approved these changes Nov 26, 2018

View reviewed changes

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2018

craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 28, 2018

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2018

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 2, 2018

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 6, 2018

bors merged commit 9139374 into rust-lang:master Dec 7, 2018

Aaron1011 mentioned this pull request Jan 3, 2019

Call poly_project_and_unify_type on types that contain inference types #56838

Merged

pietroalbini added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 3, 2019

QuietMisdreavus added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 3, 2019

pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 3, 2019

pietroalbini mentioned this pull request Jan 3, 2019

[beta] Rollup backports #57305

Merged

Aaron1011 mentioned this pull request Jan 4, 2019

[beta] Rustdoc auto-trait fixes #57324

Closed

Ensure that Rustdoc discovers all necessary auto trait bounds #55318

Ensure that Rustdoc discovers all necessary auto trait bounds #55318

Conversation

Aaron1011 commented Oct 24, 2018

rust-highfive commented Oct 24, 2018

Aaron1011 commented Oct 24, 2018

GuillaumeGomez commented Oct 24, 2018

eddyb commented Nov 5, 2018

nikomatsakis commented Nov 12, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

Aaron1011 Nov 14, 2018

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

Aaron1011 Nov 14, 2018

Choose a reason for hiding this comment

Aaron1011 commented Nov 16, 2018

Aaron1011 commented Nov 20, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

pietroalbini commented Nov 26, 2018

Aaron1011 commented Nov 26, 2018

Mark-Simulacrum commented Nov 27, 2018

bors commented Nov 27, 2018

bors commented Nov 27, 2018

pietroalbini commented Nov 27, 2018

craterbot commented Nov 27, 2018

craterbot commented Nov 27, 2018

craterbot commented Nov 28, 2018

pietroalbini commented Nov 30, 2018

craterbot commented Nov 30, 2018

craterbot commented Dec 1, 2018

craterbot commented Dec 2, 2018

Aaron1011 commented Dec 2, 2018

nikomatsakis commented Dec 6, 2018

bors commented Dec 6, 2018

nikomatsakis commented Dec 6, 2018

bors commented Dec 6, 2018

bors commented Dec 6, 2018

bors commented Dec 6, 2018

bors commented Dec 7, 2018

pietroalbini commented Jan 3, 2019

QuietMisdreavus commented Jan 3, 2019