Only warn about unused `mut` in user-written code #54787

varkor · 2018-10-03T13:15:22Z

nikomatsakis · 2018-10-03T16:30:11Z

[00:51:50] 
[00:51:50] 8    = note: to learn more, visit <https://doc.rust-lang.org/book/second-edition/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
[00:51:50] 9    = note: required by `std::iter::IntoIterator::into_iter`
[00:51:50] 10 
[00:51:50] - error: aborting due to previous error
[00:51:50] + error[E0277]: the size for values of type `dyn std::iter::Iterator<Item=&mut u8>` cannot be known at compilation time
[00:51:50] +   --> $DIR/issue-20605.rs:12:17
[00:51:50] +    |
[00:51:50] + LL |     for item in *things { *item = 0 }
[00:51:50] +    |
[00:51:50] +    |
[00:51:50] +    = help: the trait `std::marker::Sized` is not implemented for `dyn std::iter::Iterator<Item=&mut u8>`
[00:51:50] +    = note: all local variables must have a statically known size
[00:51:50] +    = help: unsized locals are gated as an unstable feature
[00:51:50] + 
[00:51:50] + error: aborting due to 2 previous errors
[00:51:50] + error: aborting due to 2 previous errors
[00:51:50] 12 
[00:51:50] 13 For more information about this error, try `rustc --explain E0277`.
[00:51:50] 14 
[00:51:50]

nikomatsakis · 2018-10-03T16:30:45Z

Curious.

nikomatsakis · 2018-10-03T16:31:04Z

Probably some span-based suppression now failing?

nikomatsakis · 2018-10-03T16:31:56Z

src/librustc/hir/lowering.rs

-                let iter_pat =
-                    self.pat_ident_binding_mode(head_sp, iter, hir::BindingAnnotation::Mutable);
+                let iter_pat = self.pat_ident_binding_mode(
+                    desugared_span,


Must be because this is now using desugared_span and not head.span — we could perhaps "normalize" the spans we use to suppress duplicate trait errors by skipping over "compiler desugarings" into the underlying span.

@nikomatsakis For my understanding could you clarify what does normalizing spans mean ?

@blitzerr: notice that the problem in this failing test is that an error about a trait bound has been duplicated. The issue here is probably that to check whether these sorts of errors should be emitted (when they might be generated multiple times), the compiler checks against the spans of previously-emitted errors. In this case, one of the errors has span head_sp and the other has desugared_span, even though they really refer to the same issue. If we instead "normalised" the spans, so that head_sp and desugared_span were considered "the same", then we could suppress the other error.

@varkor Thanks a lot for the explanation. Makes sense.

blitzerr · 2018-10-04T03:35:11Z

src/librustc/hir/lowering.rs

@@ -4058,16 +4058,16 @@ impl<'a> LoweringContext<'a> {
                // expand <head>
                let head = self.lower_expr(head);
                let head_sp = head.span;
+                let desugared_span = self.allow_internal_unstable(
+                    CompilerDesugaringKind::ForLoop,
+                    head.span,


would it be better if we use the local variable "head_sp" here instead of de-referencing head.span again ?

It doesn't really make a difference, but I suppose that would make more sense!

blitzerr · 2018-10-04T03:38:01Z

src/librustc/hir/lowering.rs

-                let iter_pat =
-                    self.pat_ident_binding_mode(head_sp, iter, hir::BindingAnnotation::Mutable);
+                let iter_pat = self.pat_ident_binding_mode(
+                    desugared_span,


@nikomatsakis For my understanding could you clarify what does normalizing spans mean ?

blitzerr · 2018-10-04T03:39:57Z

src/librustc/hir/lowering.rs

@@ -4058,16 +4058,16 @@ impl<'a> LoweringContext<'a> {
                // expand <head>
                let head = self.lower_expr(head);
                let head_sp = head.span;
+                let desugared_span = self.allow_internal_unstable(


Am I right that desugared_span is just the renaming of next_sp ?

Yes, that's correct. I just moved it so that both the spans that were being referenced multiple times were in the same place.

makes sense

blitzerr · 2018-10-04T03:45:34Z

@varkor For my own understanding, how can we generate the intermediate/generated code, to see the unused 'mut' the linter is complaining about ?

varkor · 2018-10-04T13:15:28Z

@blitzerr: I was simply using the desugar reference in lowering.rs:

rust/src/librustc/hir/lowering.rs

Lines 4037 to 4056 in 113141b

    
           // From: `[opt_ident]: for <pat> in <head> <body>` 
        
           ExprKind::ForLoop(ref pat, ref head, ref body, opt_label) => { 
        
               // to: 
        
               // 
        
               //   { 
        
               //     let result = match ::std::iter::IntoIterator::into_iter(<head>) { 
        
               //       mut iter => { 
        
               //         [opt_ident]: loop { 
        
               //           let mut __next; 
        
               //           match ::std::iter::Iterator::next(&mut iter) { 
        
               //             ::std::option::Option::Some(val) => __next = val, 
        
               //             ::std::option::Option::None => break 
        
               //           }; 
        
               //           let <pat> = __next; 
        
               //           StmtKind::Expr(<body>); 
        
               //         } 
        
               //       } 
        
               //     }; 
        
               //     result 
        
               //   }

to identify where the problem was located, but you can also pretty-print the HIR to see the desugared code, e.g. rustc unreachable_mut.rs -Zunpretty=hir, which produces (with somewhat wacky indentation):

#![feature(nll)]
#![allow(unreachable_code)]
#[prelude_import]
use ::std::prelude::v1::*;
#[macro_use]
extern crate std;

fn main() {
              {
                  let _result =
                      match ::std::iter::IntoIterator::into_iter({
                                                                     return ();
                                                                     ::std::ops::Range{start:
                                                                                           0,
                                                                                       end:
                                                                                           3,}
                                                                 }) {
                          mut iter =>
                          loop  {
                              let mut __next;
                              match ::std::iter::Iterator::next(&mut iter) {
                                  ::std::option::Option::Some(val) =>
                                  __next = val,
                                  ::std::option::Option::None => break ,
                              }
                              let _ = __next;
                              { }
                          },
                      };
                  _result
              }
          }

varkor · 2018-10-04T15:50:45Z

Okay, that seems to have fixed it!

nikomatsakis · 2018-10-04T20:18:28Z

@bors r+

Great!

bors · 2018-10-04T20:18:29Z

📌 Commit ea3d8f5 has been approved by nikomatsakis

@pnkfelix

…nikomatsakis Only warn about unused `mut` in user-written code Fixes rust-lang#54586. r? @pnkfelix cc @blitzerr

@ghost

Rollup of 11 pull requests Successful merges: - #54078 (Expand the documentation for the `std::sync` module) - #54717 (Cleanup rustc/ty part 1) - #54781 (Add examples to `TyKind::FnDef` and `TyKind::FnPtr` docs) - #54787 (Only warn about unused `mut` in user-written code) - #54804 (add suggestion for inverted function parameters) - #54812 (Regression test for #32382.) - #54833 (make `Parser::parse_foreign_item()` return a foreign item or error) - #54834 (rustdoc: overflow:auto doesn't work nicely on small screens) - #54838 (Fix typo in src/libsyntax/parse/parser.rs) - #54851 (Fix a regression in 1.30 by reverting #53564) - #54853 (Remove unneccessary error from test, revealing NLL error.) Failed merges: r? @ghost

bors · 2018-10-06T03:27:58Z

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

Only warn about unused mut in user-written code

113141b

rust-highfive assigned pnkfelix Oct 3, 2018

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

varkor mentioned this pull request Oct 3, 2018

Less conservative uninhabitedness check #54125

Merged

This comment has been minimized.

Sign in to view

nikomatsakis reviewed Oct 3, 2018

View reviewed changes

blitzerr reviewed Oct 4, 2018

View reviewed changes

Ignore desugarings when comparing duplicate trait error messages

ea3d8f5

nikomatsakis approved these changes Oct 4, 2018

View reviewed changes

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 Oct 4, 2018

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 5, 2018

Rollup merge of rust-lang#54787 - varkor:unused-mut-in-desugaring, r=…

3810657

…nikomatsakis Only warn about unused `mut` in user-written code Fixes rust-lang#54586. r? @pnkfelix cc @blitzerr

pietroalbini mentioned this pull request Oct 5, 2018

Rollup of 11 pull requests #54859

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 6, 2018

bors merged commit ea3d8f5 into rust-lang:master Oct 6, 2018

varkor deleted the unused-mut-in-desugaring branch October 6, 2018 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only warn about unused `mut` in user-written code #54787

Only warn about unused `mut` in user-written code #54787

varkor commented Oct 3, 2018

This comment has been minimized.

nikomatsakis commented Oct 3, 2018

nikomatsakis commented Oct 3, 2018

nikomatsakis commented Oct 3, 2018

nikomatsakis Oct 3, 2018

blitzerr Oct 4, 2018

varkor Oct 4, 2018

blitzerr Oct 6, 2018

blitzerr Oct 4, 2018

varkor Oct 4, 2018

blitzerr Oct 4, 2018

blitzerr Oct 4, 2018

varkor Oct 4, 2018

blitzerr Oct 4, 2018

blitzerr commented Oct 4, 2018

varkor commented Oct 4, 2018

varkor commented Oct 4, 2018

nikomatsakis commented Oct 4, 2018

bors commented Oct 4, 2018

bors commented Oct 6, 2018

Only warn about unused mut in user-written code #54787

Only warn about unused mut in user-written code #54787

Conversation

varkor commented Oct 3, 2018

This comment has been minimized.

nikomatsakis commented Oct 3, 2018

nikomatsakis commented Oct 3, 2018

nikomatsakis commented Oct 3, 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

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

blitzerr commented Oct 4, 2018

varkor commented Oct 4, 2018

varkor commented Oct 4, 2018

nikomatsakis commented Oct 4, 2018

bors commented Oct 4, 2018

bors commented Oct 6, 2018

Only warn about unused `mut` in user-written code #54787

Only warn about unused `mut` in user-written code #54787