Preserve substitutions when making trait obligations for suggestions #71618

ecstatic-morse · 2020-04-27T19:50:04Z

Resolves #71394.

I think map_bound_ref is correct here. In any case, I think a lot of the diagnostic code is using skip_binder more aggressively than it should be, so I doubt that this is worse than the status quo. The assertion that new_self_ty has no escaping bound vars should be enough.

r? @estebank

cc @nikomatsakis Is the call to skip_binder on line 551 (and elsewhere in this file) appropriate?

rust/src/librustc_trait_selection/traits/error_reporting/suggestions.rs

Lines 537 to 565 in 46ec74e

    
           fn suggest_add_reference_to_arg( 
        
               &self, 
        
               obligation: &PredicateObligation<'tcx>, 
        
               err: &mut DiagnosticBuilder<'tcx>, 
        
               trait_ref: &ty::Binder<ty::TraitRef<'tcx>>, 
        
               points_at_arg: bool, 
        
               has_custom_message: bool, 
        
           ) -> bool { 
        
               if !points_at_arg { 
        
                   return false; 
        
               } 
        
               let span = obligation.cause.span; 
        
               let param_env = obligation.param_env; 
        
               let trait_ref = trait_ref.skip_binder(); 
        
               if let ObligationCauseCode::ImplDerivedObligation(obligation) = &obligation.cause.code { 
        
                   // Try to apply the original trait binding obligation by borrowing. 
        
                   let self_ty = trait_ref.self_ty(); 
        
                   let found = self_ty.to_string(); 
        
                   let new_self_ty = self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, self_ty); 
        
                   let substs = self.tcx.mk_substs_trait(new_self_ty, &[]); 
        
                   let new_trait_ref = ty::TraitRef::new(obligation.parent_trait_ref.def_id(), substs); 
        
                   let new_obligation = Obligation::new( 
        
                       ObligationCause::dummy(), 
        
                       param_env, 
        
                       new_trait_ref.without_const().to_predicate(), 
        
                   ); 
        
                   if self.predicate_must_hold_modulo_regions(&new_obligation) {

estebank

The code changes themselves look ok, but I am hesitant to erasing the original cause.

estebank · 2020-04-27T20:06:48Z

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

-                        trait_ref.def_id,
-                        trait_type,
-                        ObligationCause::dummy(),
+                    let new_obligation = self.mk_trait_obligation_with_new_self_ty(
                        obligation.param_env,
+                        trait_ref,
+                        suggested_ty,


ignoring the previous obligation entirely? We used to use the same cause, right?

Is this what causes the "note: std::convert::From<std::string::String> is implemented for &mut str, but not for &str" to go away?

No. That comes from propagating substitutions correctly. From<String> is not implemented for any of &mut str or &str.

My impression was that the cause for an obligation is where it originates from in the source. There is no underlying cause for the new obligation we create, since we're just trying various "self" types for diagnostic hints. From this POV, obligations that are only for speculation should have no cause code.

nikomatsakis · 2020-04-28T16:43:10Z

@ecstatic-morse in answer to your question, I think this call to skip_binder looks wrong:

rust/src/librustc_trait_selection/traits/error_reporting/suggestions.rs

Line 551 in 46ec74e

let trait_ref = trait_ref.skip_binder();

really, the only correct way to use skip_binder is if you are going to be comparing or extracting some information that can't involve any bound regions/types/things. This code seems to go on to construct obligations and try to prove them:

rust/src/librustc_trait_selection/traits/error_reporting/suggestions.rs

Lines 560 to 564 in 46ec74e

    
           let new_obligation = Obligation::new( 
        
               ObligationCause::dummy(), 
        
               param_env, 
        
               new_trait_ref.without_const().to_predicate(), 
        
           );

so that seems wrong. Probably better would be to do something like invoking replace_bound_vars_with_placeholders.

nikomatsakis · 2020-04-28T16:45:43Z

Should I review this PR?

ecstatic-morse · 2020-04-28T16:49:02Z

@nikomatsakis That would be great! I considered implementing a TypeFolder for changing the self type in a trait predicate, but I'm not sure if that is the idiomatic approach. It seems maybe like overkill? I was trying to follow the style used elsewhere in this module, but I think it's not quite correct (your comment supports this notion). Any tips would be much appreciated. I'm not very familiar with the trait solver, but I'm trying to level up enough that I can implement rust-lang/rfcs#2632.

estebank · 2020-04-28T17:51:37Z

R? @nikomatsakis

bors · 2020-05-12T02:53:35Z

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

ecstatic-morse · 2020-05-17T00:04:20Z

ping @nikomatsakis

nikomatsakis · 2020-05-18T15:44:55Z

src/librustc_trait_selection/traits/error_reporting/mod.rs

-        let new_trait_ref =
-            ty::TraitRef { def_id, substs: self.tcx.mk_substs_trait(output_ty, &[]) };
-        Obligation::new(cause, param_env, new_trait_ref.without_const().to_predicate())
+        assert!(!new_self_ty.has_escaping_bound_vars());


I think we should comment this function and clarify that new_self_ty cannot have escaping bound variables

nikomatsakis · 2020-05-18T15:46:45Z

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

            obligation.param_env,
+            trait_ref,
+            output_ty.skip_binder(),


Is there a reason that we think output_ty cannot reference bound variables? (The assertion suggests we do, but I don't immediately see it.)

I'd prefer to see output_ty.no_bound_vars().unwrap() if we believe this to be true (no_bound_vars docs)

I think a function return type can indeed have late bound variables. fn<'a>() -> &'a dyn Any is late bound, correct? Perhaps we should just bail out if it does? Alternatively, is there a way to preserve binding levels within mk_trait_obligation_with_new_self_ty?

nikomatsakis · 2020-05-18T15:48:14Z

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

@@ -671,17 +669,16 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                return;
            }

-            let mut trait_type = trait_ref.self_ty();
+            let mut suggested_ty = trait_ref.self_ty();


I'm a bit confused how the types work out here. This is a Binders<Ty>, right?

It's a plain Ty if that helps.

Yes, I noticed that recently. Hiding the skip_binder call like that is...just so wrong. But it explains how this code type-checks, anyway.

nikomatsakis · 2020-05-18T15:48:26Z

src/librustc_trait_selection/traits/error_reporting/suggestions.rs


            for refs_remaining in 0..refs_number {
-                if let ty::Ref(_, t_type, _) = trait_type.kind {
-                    trait_type = t_type;
+                if let ty::Ref(_, inner_ty, _) = suggested_ty.kind {


...but I must be wrong, because how do we access the suggested_ty.kind then?

nikomatsakis · 2020-05-18T15:49:41Z

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

                    obligation.param_env,
+                    &trait_ref,
+                    suggested_ty,


as above I don't 100% understand where this type is coming from or how we can be sure that it has no bound references in it -- doesn't it come from the trait_ref?

PolyTraitRef::self_ty is equivalent to self.skip_binder().substs.type_at(0). It seems we are assuming that the self type has no escaping bound variables. Is this assumption correct?

One other thing that I found confusing: According to the docs for TraitRef,

[it] introduces a level of region binding, to account for higher-ranked trait bounds like T: for<'a> Foo<&'a U> or higher-ranked object types.

If that's true, why do we use PolyTraitRef in so many places? Is it just for higher-ranked self types? If so, why is PolyTraitRef::self_ty, which calls skip_binder, correct?

These docs are wrong. Binders are always represented explicitly by a Binder -- so TraitRef alone cannot account for for<'a>, only a PolyTraitRef can.

Is this assumption correct?

I think this is sort of what I was asking. I doubt it's correct. I think a reasonable thing to do might be to test the assumption by using no_bound_vars instead of relying on skip_binder (and, if the assumption is false, not generating the suggestions).

It seems like we want to change PolyTraitRef::self_ty to return a Binder<Ty>. Is this correct? This will have a significant amount of fallout I think. Do you want me to open an issue to track this? I could check the result of self_ty for no_bound_vars in this PR, but we need to be doing this across all diagnostics code, not just here.

nikomatsakis · 2020-05-18T15:50:01Z

@ecstatic-morse left some questions!

nikomatsakis · 2020-05-21T21:18:39Z

OK, so, I think these changes are definitely going in the right direction. I'd probably be happy to merge as-is, seems like an improvement, though I suspect we could do a bit more refactoring to avoid some potential ICEs with higher-ranked regions down the line (along the lines of using no_bound_vars more often).

bors · 2020-05-22T01:38:08Z

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

`mk_obligation_for_def_id` is only correct if the trait and self type do not have any substitutions. Use a different method, `mk_trait_obligation_with_new_self_ty` that is more clear about what is happening.

Otherwise inserting it to the `Binder` used by `trait_ref` would cause problems. This is just to be extra carefult: we aren't going to start recommending that the user start using HKTs anytime soon.

ecstatic-morse · 2020-05-22T20:35:46Z

Rebased to fix merge conflict. Can I r+ this? The original intent was to fix a rather embarrassing diagnostics bug. I will open an issue for removing self_ty and replacing it with no_bound_vars, as well as one for auditing uses of skip_binder in the diagnostics code more generally.

nikomatsakis · 2020-05-22T21:11:54Z

@bors r+

bors · 2020-05-22T21:11:55Z

📌 Commit 1fad3b7 has been approved by nikomatsakis

bors · 2020-05-22T21:11:56Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

ecstatic-morse · 2020-05-23T01:03:07Z

@bors rollup

@estebank

…atsakis Preserve substitutions when making trait obligations for suggestions Resolves rust-lang#71394. I *think* `map_bound_ref` is correct here. In any case, I think a lot of the diagnostic code is using `skip_binder` more aggressively than it should be, so I doubt that this is worse than the status quo. The assertion that `new_self_ty` has no escaping bound vars should be enough. r? @estebank cc @nikomatsakis Is the call to `skip_binder` on line 551 (and elsewhere in this file) appropriate? https://github.com/rust-lang/rust/blob/46ec74e60f238f694b46c976d6217e7cf8d4cf1a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs#L537-L565

@estebank

…atsakis Preserve substitutions when making trait obligations for suggestions Resolves rust-lang#71394. I *think* `map_bound_ref` is correct here. In any case, I think a lot of the diagnostic code is using `skip_binder` more aggressively than it should be, so I doubt that this is worse than the status quo. The assertion that `new_self_ty` has no escaping bound vars should be enough. r? @estebank cc @nikomatsakis Is the call to `skip_binder` on line 551 (and elsewhere in this file) appropriate? https://github.com/rust-lang/rust/blob/46ec74e60f238f694b46c976d6217e7cf8d4cf1a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs#L537-L565

@ghost

Rollup of 5 pull requests Successful merges: - rust-lang#71618 (Preserve substitutions when making trait obligations for suggestions) - rust-lang#72092 (Unblock font loading in rustdoc.css) - rust-lang#72400 (Add missing ASM arena declarations to librustc_middle) - rust-lang#72489 (Fix ice-72487) - rust-lang#72502 (fix discriminant type in generator transform) Failed merges: r? @ghost

…matsakis Make `PolyTraitRef::self_ty` return `Binder<Ty>` This came up during review of rust-lang#71618. The current implementation is the same as a call to `skip_binder` but harder to audit. Make it preserve binding levels and add a call to `skip_binder` at all use sites so they can be audited as part of rust-lang#72507.

rust-highfive assigned estebank Apr 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2020

estebank approved these changes Apr 27, 2020

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned estebank Apr 28, 2020

ecstatic-morse force-pushed the issue-71394 branch from 271acc1 to eb28214 Compare April 28, 2020 20:09

joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2020

ecstatic-morse force-pushed the issue-71394 branch from eb28214 to 91b52e7 Compare May 13, 2020 17:16

nikomatsakis reviewed May 18, 2020

View reviewed changes

ecstatic-morse force-pushed the issue-71394 branch from 91b52e7 to 955000c Compare May 18, 2020 18:19

This comment has been minimized.

Sign in to view

nikomatsakis approved these changes May 21, 2020

View reviewed changes

ecstatic-morse added 7 commits May 22, 2020 13:34

Preserve substitutions when trying to prove trait obligation

730c6f3

`mk_obligation_for_def_id` is only correct if the trait and self type do not have any substitutions. Use a different method, `mk_trait_obligation_with_new_self_ty` that is more clear about what is happening.

Add regression test for rust-lang#71394

8ea828b

Bless other example of rust-lang#71394

4d9e9c6

Ensure that new_self_ty has no escaping bound vars

d2bacb1

Otherwise inserting it to the `Binder` used by `trait_ref` would cause problems. This is just to be extra carefult: we aren't going to start recommending that the user start using HKTs anytime soon.

Document invariants of mk_trait_obligation_with_new_self_ty

7278e29

Bail out if output_ty has bound variables

f99519b

Use mk_trait_obligation_with_new_self_ty for new suggestion

1fad3b7

ecstatic-morse force-pushed the issue-71394 branch from 47c539a to 1fad3b7 Compare May 22, 2020 20:36

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 May 22, 2020

RalfJung mentioned this pull request May 23, 2020

Rollup of 6 pull requests #72490

Closed

This was referenced May 23, 2020

Audit uses of skip_binder in diagnostics code #72507

Open

Make PolyTraitRef::self_ty return Binder<Ty> #72508

Merged

Dylan-DPC-zz mentioned this pull request May 23, 2020

Rollup of 5 pull requests #72516

Merged

bors merged commit ff48fc9 into rust-lang:master May 24, 2020

SimonSapin mentioned this pull request May 26, 2020

Regression: spurious "error: expected ," in serde::Serialize on macro_rules-variable path enum discriminant #72608

Closed

ecstatic-morse deleted the issue-71394 branch October 6, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve substitutions when making trait obligations for suggestions #71618

Preserve substitutions when making trait obligations for suggestions #71618

ecstatic-morse commented Apr 27, 2020 •

edited

Loading

estebank left a comment

estebank Apr 27, 2020

ecstatic-morse Apr 27, 2020 •

edited

Loading

nikomatsakis commented Apr 28, 2020 •

edited

Loading

nikomatsakis commented Apr 28, 2020

ecstatic-morse commented Apr 28, 2020 •

edited

Loading

estebank commented Apr 28, 2020

bors commented May 12, 2020

ecstatic-morse commented May 17, 2020

nikomatsakis May 18, 2020

nikomatsakis May 18, 2020

ecstatic-morse May 18, 2020 •

edited

Loading

nikomatsakis May 18, 2020

ecstatic-morse May 18, 2020

nikomatsakis May 21, 2020

nikomatsakis May 18, 2020

nikomatsakis May 18, 2020

ecstatic-morse May 18, 2020

ecstatic-morse May 18, 2020

nikomatsakis May 21, 2020

nikomatsakis May 21, 2020

ecstatic-morse May 21, 2020

nikomatsakis commented May 18, 2020

This comment has been minimized.

nikomatsakis commented May 21, 2020

bors commented May 22, 2020

ecstatic-morse commented May 22, 2020

nikomatsakis commented May 22, 2020

bors commented May 22, 2020

bors commented May 22, 2020

ecstatic-morse commented May 23, 2020

	fn suggest_add_reference_to_arg(
	&self,
	obligation: &PredicateObligation<'tcx>,
	err: &mut DiagnosticBuilder<'tcx>,
	trait_ref: &ty::Binder<ty::TraitRef<'tcx>>,
	points_at_arg: bool,
	has_custom_message: bool,
	) -> bool {
	if !points_at_arg {
	return false;
	}

	let span = obligation.cause.span;
	let param_env = obligation.param_env;
	let trait_ref = trait_ref.skip_binder();

	if let ObligationCauseCode::ImplDerivedObligation(obligation) = &obligation.cause.code {
	// Try to apply the original trait binding obligation by borrowing.
	let self_ty = trait_ref.self_ty();
	let found = self_ty.to_string();
	let new_self_ty = self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, self_ty);
	let substs = self.tcx.mk_substs_trait(new_self_ty, &[]);
	let new_trait_ref = ty::TraitRef::new(obligation.parent_trait_ref.def_id(), substs);
	let new_obligation = Obligation::new(
	ObligationCause::dummy(),
	param_env,
	new_trait_ref.without_const().to_predicate(),
	);
	if self.predicate_must_hold_modulo_regions(&new_obligation) {

Preserve substitutions when making trait obligations for suggestions #71618

Preserve substitutions when making trait obligations for suggestions #71618

Conversation

ecstatic-morse commented Apr 27, 2020 • edited Loading

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Apr 28, 2020 • edited Loading

nikomatsakis commented Apr 28, 2020

ecstatic-morse commented Apr 28, 2020 • edited Loading

estebank commented Apr 28, 2020

bors commented May 12, 2020

ecstatic-morse commented May 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 18, 2020

This comment has been minimized.

nikomatsakis commented May 21, 2020

bors commented May 22, 2020

ecstatic-morse commented May 22, 2020

nikomatsakis commented May 22, 2020

bors commented May 22, 2020

bors commented May 22, 2020

ecstatic-morse commented May 23, 2020

ecstatic-morse commented Apr 27, 2020 •

edited

Loading

ecstatic-morse Apr 27, 2020 •

edited

Loading

nikomatsakis commented Apr 28, 2020 •

edited

Loading

ecstatic-morse commented Apr 28, 2020 •

edited

Loading

ecstatic-morse May 18, 2020 •

edited

Loading