Don't display default generic parameters in diagnostics that compare types #52244

glandium · 2018-07-11T07:39:58Z

In errors like:

   expected type: `RawVec<foo, Global>`
      found type: `foo`

RawVec being defined as RawVec<T, A: Alloc = Global>, the error is better written as

   expected type: `RawVec<foo>`
      found type: `foo`

In fact, that is already what happens when foo is not an ADT, because in that case, the diagnostic handler doesn't try to highlight something, and just uses the Display trait instead of its own logic.

e.g.

   expected type: `RawVec<usize>`
      found type: `usize`

rust-highfive · 2018-07-11T07:40:02Z

r? @estebank

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

glandium · 2018-07-11T07:45:14Z

src/test/ui/type-mismatch.stderr

@@ -194,16 +194,16 @@ LL |     want::<Foo<foo, B>>(f); //~ ERROR mismatched types
   |                         ^ expected struct `B`, found struct `A`


Note this error becomes awkward:

^ expected struct `B`, found struct `A` = note: expected type `Foo<_, B>` found type `Foo<_, A>` found type `Foo<_>`

Maybe this particular case should display the parameters.

You should probably create a new method similar to strip_generic_default_params that checks wether the default associated type has been changed, and if it has, don't call strip_generic_default_params. Does that make sense?

This is part of the reason we haven't unified ppaux and this: this code is conditional on the state of two types with very specific interactions.

Turns out addressing the awkwardness made the change simpler. I added more tests, too.

glandium · 2018-07-11T07:48:17Z

I'll note that I'm also not particularly proud of the code duplication from PrintContext::parameterized and that I have no idea if the following from there is needed or not:

rust/src/librustc/util/ppaux.rs

Lines 312 to 334 in acf50b7

    
           if let Some(def_id) = generics.parent { 
        
               // Methods. 
        
               assert!(is_value_path); 
        
               child_types = child_own_counts.types; 
        
               generics = tcx.generics_of(def_id); 
        
               own_counts = generics.own_counts(); 
        
               if has_self { 
        
                   print!(f, self, write("<"), print_display(substs.type_at(0)), write(" as "))?; 
        
               } 
        
               path_def_id = def_id; 
        
           } else { 
        
               item_name = None; 
        
               if is_value_path { 
        
                   // Functions. 
        
                   assert_eq!(has_self, false); 
        
               } else { 
        
                   // Types and traits. 
        
                   own_counts = child_own_counts; 
        
               } 
        
           }

(I don't know in what case generics.parent can be Some())

estebank · 2018-07-11T16:46:11Z

@glandium Regarding your last comment, this is a complete hack at the moment, and we need to figure out a good design to merge the code you're modifying and ppaux.

estebank

Otherwise, it looks sound to me.

estebank · 2018-07-11T16:51:46Z

src/test/ui/type-mismatch.stderr

@@ -194,16 +194,16 @@ LL |     want::<Foo<foo, B>>(f); //~ ERROR mismatched types
   |                         ^ expected struct `B`, found struct `A`


You should probably create a new method similar to strip_generic_default_params that checks wether the default associated type has been changed, and if it has, don't call strip_generic_default_params. Does that make sense?

This is part of the reason we haven't unified ppaux and this: this code is conditional on the state of two types with very specific interactions.

They highlight how types are displayed in those cases, and will show how the current output is altered by the next patch.

…types. Fixes rust-lang#52097

estebank · 2018-07-12T16:30:16Z

@bors r+ rollup

bors · 2018-07-12T16:30:17Z

📌 Commit b5c2b79 has been approved by estebank

estebank · 2018-07-12T16:37:19Z

src/test/ui/type-mismatch.stderr

+   |                         ^ expected struct `bar`, found struct `foo`
+   |
+   = note: expected type `Foo<bar, _, B>`
+              found type `Foo<foo, _, A>`


Just out of curiosity, are bar, foo, B and A highlighted in the output here? I would think yes, but want to confirm (regardless, this PR is going in, I'm asking just to see if I have to file a ticket to add that).

Yes, they are. It would be great if the ui tests actually tested for that too. It would become more apparent that there's some inconsistency for e.g. usize/Foo<usize> where everything is highlighted as opposed to foo/Foo<foo> where foo is not highlighted, which is because usize is not an Adt and doesn't fall in the same code path as foo.

I think that the latter is not because of that but rather because foo is detected as the first type argument to Foo, which means that the think to highlight is only Foo, indirectly telling you "you need to wrap this with a Foo!". It's the same as when you have an Option<Foo> and a Result<Foo, _> was expected, Foo doesn't get highlighted.

It would be great if the ui tests actually tested for that too.

It would, but it probably would be a pain to look at :-/ The current situation is good enough™ that we haven't pushed to change it yet.

No, really, it's only because foo is an Adt, and usize isn't. The comparison between foo and Foo<foo> hits this pattern:

rust/src/librustc/infer/error_reporting/mod.rs

Line 715 in 64f7de9

(&ty::TyAdt(def1, sub1), &ty::TyAdt(def2, sub2)) => {

while the comparison between usize and Foo<usize> hits this one:

rust/src/librustc/infer/error_reporting/mod.rs

Line 857 in 64f7de9

_ => {

Oh! You're absolutely right. I though I had expanded a few more branches to account for this case before, but now looking at the code in master I see that I must have never submitted that PR.

Don't display default generic parameters in diagnostics that compare types In errors like: ``` expected type: `RawVec<foo, Global>` found type: `foo` ``` `RawVec` being defined as `RawVec<T, A: Alloc = Global>`, the error is better written as ``` expected type: `RawVec<foo>` found type: `foo` ``` In fact, that is already what happens when `foo` is not an ADT, because in that case, the diagnostic handler doesn't try to highlight something, and just uses the `Display` trait instead of its own logic. e.g. ``` expected type: `RawVec<usize>` found type: `usize` ```

@ghost

Rollup of 16 pull requests Successful merges: - #51962 (Provide llvm-strip in llvm-tools component) - #52003 (Implement `Option::replace` in the core library) - #52156 (Update std::ascii::ASCIIExt deprecation notes) - #52242 (NLL: Suggest `ref mut` and `&mut self`) - #52244 (Don't display default generic parameters in diagnostics that compare types) - #52290 (Deny bare trait objects in src/librustc_save_analysis) - #52293 (Deny bare trait objects in librustc_typeck) - #52299 (Deny bare trait objects in src/libserialize) - #52300 (Deny bare trait objects in librustc_target and libtest) - #52302 (Deny bare trait objects in the rest of rust) - #52310 (Backport 1.27.1 release notes to master) - #52314 (Fix ICE when using a pointer cast as array size) - #52315 (Resolve FIXME(#27942)) - #52316 (task: remove wrong comments about non-existent LocalWake trait) - #52322 (Update llvm-rebuild-trigger in light of LLVM 7 upgrade) - #52332 (dead-code lint: say "constructed", "called" for structs, functions) Failed merges: r? @ghost

bors · 2018-07-14T06:24:49Z

⌛ Testing commit b5c2b79 with merge 0a8275f...

Don't display default generic parameters in diagnostics that compare types In errors like: ``` expected type: `RawVec<foo, Global>` found type: `foo` ``` `RawVec` being defined as `RawVec<T, A: Alloc = Global>`, the error is better written as ``` expected type: `RawVec<foo>` found type: `foo` ``` In fact, that is already what happens when `foo` is not an ADT, because in that case, the diagnostic handler doesn't try to highlight something, and just uses the `Display` trait instead of its own logic. e.g. ``` expected type: `RawVec<usize>` found type: `usize` ```

bors · 2018-07-14T08:25:39Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 0a8275f to master...

rust-highfive assigned estebank Jul 11, 2018

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

glandium commented Jul 11, 2018

View reviewed changes

glandium mentioned this pull request Jul 11, 2018

Associate an allocator to boxes #50882

Closed

estebank reviewed Jul 11, 2018

View reviewed changes

glandium added 2 commits July 12, 2018 17:06

Add UI tests for diagnostics comparing types

5336df7

They highlight how types are displayed in those cases, and will show how the current output is altered by the next patch.

Don't display default generic parameters in diagnostics that compare …

b5c2b79

…types. Fixes rust-lang#52097

glandium force-pushed the issue52097 branch from 1656acf to b5c2b79 Compare July 12, 2018 08:15

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 Jul 12, 2018

estebank reviewed Jul 12, 2018

View reviewed changes

Mark-Simulacrum mentioned this pull request Jul 13, 2018

Rollup of 16 pull requests #52344

Closed

bors merged commit b5c2b79 into rust-lang:master Jul 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't display default generic parameters in diagnostics that compare types #52244

Don't display default generic parameters in diagnostics that compare types #52244

glandium commented Jul 11, 2018

rust-highfive commented Jul 11, 2018

glandium Jul 11, 2018

estebank Jul 11, 2018

glandium Jul 12, 2018

glandium commented Jul 11, 2018

estebank commented Jul 11, 2018

estebank left a comment

estebank Jul 11, 2018

estebank commented Jul 12, 2018

bors commented Jul 12, 2018

estebank Jul 12, 2018

glandium Jul 12, 2018

estebank Jul 12, 2018

glandium Jul 12, 2018

estebank Jul 12, 2018

bors commented Jul 14, 2018

bors commented Jul 14, 2018

		@@ -194,16 +194,16 @@ LL \| want::<Foo<foo, B>>(f); //~ ERROR mismatched types
		\| ^ expected struct `B`, found struct `A`

Don't display default generic parameters in diagnostics that compare types #52244

Don't display default generic parameters in diagnostics that compare types #52244

Conversation

glandium commented Jul 11, 2018

rust-highfive commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glandium commented Jul 11, 2018

estebank commented Jul 11, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jul 12, 2018

bors commented Jul 12, 2018

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

bors commented Jul 14, 2018

bors commented Jul 14, 2018