Associated types can be specified multiple times in trait objects

[Marwes]

fn test() ->  Box<Iterator<Item = (), Item = ()>> {
    Box::new(None.into_iter())
}

It seems rather surprising that the above code actually compiles. Making it an error wouldn't be backwards compatible but a warning would be nice (the same happens with impl Future<Item = (), Item = ()> which is where I got burnt, thinking I had specified the Error type).

[abonander]

Implementing a lint for this would be a good beginner's task I think.

[gootorov]

I'd like to work on this starting from Tuesday, if that's ok. Never hacked on a compiler before, anything I need to know besides the rustc guide for this particular issue? Would appreciate any help!

[F001]

Hi @abonander , I find it is easier to implement this warning based on normal diagnostics than lint in parser.rs.

We just need to add a new argument enable_warning : bool for the function fn parse_generic_args, and then detect whether there are duplicates in bindings, finally report issues by self.diagnostic().struct_span_warn(). Whereas, constructing a new lint is more complicated.

Is this solution acceptable?

[abonander]

@F001 Hardcoded warnings are strongly discouraged, but it's not that difficult to create a lint anyway. This should get you started; add the following to src/librustc_lint/builtin.rs:

declare_lint! {
    // we can bikeshed the lint name
    DUP_ASSOC_TYPE_BINDING,
    Warn,
    "warns about duplicate associated type bindings in generics, e.g. `Iterator<Item = (), Item = ()>`"
}

pub struct DupAssocTypeBinding;

impl LintPass for DupAssocTypeBinding {
    fn get_lints(&self) -> LintArray {
        lint_array!(DUP_ASSOC_TYPE_BINDING)
    }
}

impl EarlyLintPass for DupAssocTypeBinding {
    fn check_path(&self, ctxt: &EarlyContext, path: &ast::Path) {
        // could be `Iterator` or `std::iter::Iterator` or `Iterator<Item = ()>::next`
        // need to check all segments
        for segment in &path.segments {
            // if its `Iterator<..>` or `Iterator(...)` (for closure traits)
            if let Some(ref params) = segment.parameters {
                // if it's `Iterator<...>`
                if let ast::AngleBracketed(ref params_data) = &**params {
                    // in `params_data.bindings`, if two elements have the same 
                    // `ident` and `ty` fields but differing `id` fields,
                    // emit a lint message with:
                    ctxt.span_lint(DUP_ASSOC_TYPE_BINDING, 
                                   vec![param_span_1, param_span_2], 
                                   "duplicate associated type binding");
                }
            }
        }
    }
}

We can bikeshed the lint message.

Also, try to avoid emitting a lint for the same pair twice if you're doing a nested for loop; the inner loop should only iterate the elements after the current one. In fact, you should accumulate the duplicated spans inside the inner loop and then emit the lint message in the outer loop, so you lint for all the same duplicated type bindings at once, e.g. Iterator<Item = (), Item = (), Item = ()> emits one lint message pointing to them all.

And then in src/librustc_lint/lib.rs, in this macro invocation add DupAssocTypeBinding where it fits alphabetically.

Then you'll need to implement a UI test (which you would need to implement if you were emitting the warning in the parser anyway). I can add instructions for that if you need it.

[F001]

Wow, what an informative instruction. Thanks very much. I will create a merge request tomorrow.

[abonander]

@F001 I had to edit my comment because check_generics() was not the right method, just making sure you've seen the latest version.