Add lint for multiple associated types #50682

F001 · 2018-05-12T11:01:51Z

rust-highfive · 2018-05-12T11:02:01Z

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

rust-highfive · 2018-05-12T11:17:36Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:26] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:27] tidy error: /checkout/src/librustc_lint/builtin.rs:1595: line longer than 100 chars
[00:05:28] some tidy checks failed
[00:05:28] 
[00:05:28] 
[00:05:28] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:28] 
[00:05:28] 
[00:05:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:28] Build completed unsuccessfully in 0:02:15
[00:05:28] Build completed unsuccessfully in 0:02:15
[00:05:28] make: *** [tidy] Error 1
[00:05:28] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:089e33f0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

abonander · 2018-05-12T18:01:01Z

src/librustc_lint/builtin.rs

+    fn check_bindings(&self, ctxt: &EarlyContext, bindings: &[ast::TypeBinding]) {
+        let mut dup_binds = HashMap::with_capacity(bindings.len());
+        for b in bindings {
+            dup_binds.entry(b.ident.name).or_insert(vec![]).push(b.ident.span);


So the lint doesn't allocate for every path it checks, I think it'd be better just to do a nested loop search for any duplicates and push their spans to a Vec.

Well, using a HashMap<Name, Vec> to collect all the spans with the same associate type name can avoid the inner loop.
What do you mean by "the lint doesn't allocate for every path it checks"?

Your approach is fine, actually, I just think it shouldn't allocate until a duplicate is found, so use HashMap::new(). That way we're not allocating when we don't end up using it.

Thanks. If we want to avoid allocation every time, how about keeping the HashMap as a member of struct struct DupAssocTypeBinding?

I don't think that's quite necessary, I was just concerned about allocating every time this function is called.

I think you are right. I have updated this function. Thanks.

michaelwoerister · 2018-05-16T09:15:24Z

r? @nikomatsakis for re-assignment

petrochenkov · 2018-05-16T09:32:58Z

Everybody forgets about hygiene :(
Rather than repeating name resolution's job incorrectly, this lint should use its results instead.
Associated type bindings are resolved to their definitions in fn ast_type_binding_to_poly_projection_predicate and then checked for privacy/stability in the same place.
The duplication check should be performed somewhere in the same place too.

abonander · 2018-05-16T09:57:30Z

@petrochenkov It's probably my mistake. This and #50763 were both started from my mentoring instructions and I was purely looking at the AST types since anything further in-depth would probably not be a good starting ticket.

bors · 2018-05-16T11:24:25Z

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

F001 · 2018-05-16T13:26:09Z

I will fix this when I have time in the next two days. Thanks very much!

nikomatsakis · 2018-05-16T21:33:21Z

@F001 do you think you know what to do from @petrochenkov's notes?

nikomatsakis · 2018-05-16T21:50:19Z

r? @petrochenkov — since you seem to have a pretty specific idea for what you want, can you take on the review? (let me know if that's a problem)

F001 · 2018-05-18T16:25:35Z

@petrochenkov This lint is re-implemented. Please help to review. Thanks!

petrochenkov · 2018-05-18T18:03:14Z

src/librustc/lint/builtin.rs

@@ -279,6 +279,12 @@ declare_lint! {
    "detects name collision with an existing but unstable method"
 }

+declare_lint! {
+    pub DUPLICATE_ASSOCIATED_TYPE_BINDING,


Could you add this lint to the list of deprecation lints?
(See e.g. PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES as an example.)

Also duplicate_associated_type_binding -> duplicate_associated_type_bindings (see https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#lints).

petrochenkov · 2018-05-18T18:09:47Z

src/librustc_typeck/astconv.rs

@@ -565,6 +577,7 @@ impl<'o, 'gcx: 'tcx, 'tcx> AstConv<'gcx, 'tcx>+'o {
            tcx.sess.span_err(binding.span, &msg);
        }
        tcx.check_stability(assoc_ty.def_id, Some(ref_id), binding.span);
+        dup_bindings.entry(assoc_ty.def_id).or_insert(Vec::new()).push(binding.span);


Could we model this error after duplicated fields:

error[E0062]: field `a` specified more than once --> src/main.rs:4:23 | 4 | let s = S { a: 0, a: 1 }; | ---- ^ used more than once | | | first use of `a`

We keep FxHashMap<DefId, Span> instead of FxHashMap<DefId, Vec<Span>> and if the inserted DefId was already in the table, report an error pointing to the first use right here.
I'd even reuse all the wording for fields (except for replacing "field a" with "associated type binding a", of course).

(You can attach labels and notes to the object returned by struct_span_lint_node, there are plenty of examples in the codebase.)

petrochenkov · 2018-05-19T15:12:00Z

src/test/ui/lint/issue-50589-multiple-associated-types.stderr

+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #50589 <https://github.com/rust-lang/rust/issues/50589>
+
+warning: associated type binding `Item` specified more than once


Hmm, the warning is reported twice.
I suspect it needs to be reported only when speculative is false.

It seems the warning is reported twice when speculative is false. I don't know why.

bors · 2018-05-19T17:07:59Z

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

petrochenkov · 2018-05-20T12:55:50Z

src/librustc_typeck/astconv.rs

@@ -566,6 +568,23 @@ impl<'o, 'gcx: 'tcx, 'tcx> AstConv<'gcx, 'tcx>+'o {
        }
        tcx.check_stability(assoc_ty.def_id, Some(ref_id), binding.span);

+        if speculative {


if speculative -> if !speculative

petrochenkov · 2018-05-20T12:56:19Z

r=me after inverting the speculative flag
@bors delegate+

bors · 2018-05-20T12:56:20Z

✌️ @F001 can now approve this pull request

F001 · 2018-05-22T01:12:29Z

@petrochenkov Thank you for your detailed mentoring. As I mentioned in the previous comment, when I use if !speculative, the warning is still reported twice.
I tried to use if speculative, this time the warning is reported only once. I think if !speculative is not the best way to solve this problem, though I can't explain why.

petrochenkov · 2018-05-22T09:10:39Z

Weird. I'll look what happens.

petrochenkov · 2018-05-22T10:59:40Z

Ok, this may look logically impossible unless you know that fully identical diagnostics are merged!
if speculative actually emits the warning once from the privacy pass, but if !speculative emits it thrice and the first one is not like the others due to note: #[warn(duplicate_associated_type_bindings)] on by default emitted by lint machinery itself, so it means two non-identical warnings.

This happens because instantiate_poly_trait_ref is called several times in astconv/collect for some reason.
Fixing this is probably out of scope for this PR, so let's land this with if !speculative and duplicate diagnostics.

F001 · 2018-05-23T03:07:41Z

@bors: r=petrochenkov

bors · 2018-05-23T03:07:41Z

📌 Commit 88f810f has been approved by petrochenkov

bors · 2018-05-23T05:09:20Z

⌛ Testing commit 88f810f with merge 9a4e5df...

@abonander

Add lint for multiple associated types Fix #50589. cc @abonander

bors · 2018-05-23T07:15:28Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 9a4e5df to master...

Do not deduplicate diagnostics in UI tests Error reporting infrastructure deduplicates identical diagnostics with identical spans. While it's preferable to do this in "release"/"user-facing" mode, it sometimes brings [confusion](rust-lang#50682 (comment)) and hides details that may be important during development. Do we run some passes multiple times when we could do it once? How many times we run them exactly? Can this number be large? Can the multiplied error construction be expensive? Can speculative checks be made cheaper if they don't report errors? *Relying* on this mechanism to deduplicate some specific error never looks like a proper solution to me personally. In this PR I attempt to disable this deduplication by applying `-Z deduplicate-diagnostics=no` to UI tests.

rust-highfive assigned michaelwoerister May 12, 2018

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

F001 force-pushed the issue-50589 branch from 1185a2a to 0ad2040 Compare May 12, 2018 13:00

abonander reviewed May 12, 2018

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned michaelwoerister May 16, 2018

petrochenkov self-assigned this May 16, 2018

rust-highfive unassigned nikomatsakis May 16, 2018

petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2018

F001 force-pushed the issue-50589 branch from 5e8d234 to aff1850 Compare May 18, 2018 16:18

petrochenkov reviewed May 18, 2018

View reviewed changes

petrochenkov reviewed May 19, 2018

View reviewed changes

F001 added 3 commits May 20, 2018 10:16

re-implement

4bb3996

Fix according to comments

c332255

avoid reporting twice

f837c34

F001 force-pushed the issue-50589 branch from 0ebcf21 to f837c34 Compare May 20, 2018 02:24

petrochenkov reviewed May 20, 2018

View reviewed changes

inverting speculative flag

88f810f

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2018

bors added a commit that referenced this pull request May 23, 2018

Auto merge of #50682 - F001:issue-50589, r=petrochenkov

9a4e5df

Add lint for multiple associated types Fix #50589. cc @abonander

bors merged commit 88f810f into rust-lang:master May 23, 2018

bors mentioned this pull request May 23, 2018

Implementation of RFC 2086 - Allow Irrefutable Let patterns #49469

Merged

F001 deleted the issue-50589 branch August 28, 2018 10:25

petrochenkov mentioned this pull request Dec 7, 2019

Do not deduplicate diagnostics in UI tests #67122

Merged

Add lint for multiple associated types #50682

Add lint for multiple associated types #50682

Conversation

F001 commented May 12, 2018

rust-highfive commented May 12, 2018

rust-highfive commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

F001 May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented May 16, 2018

petrochenkov commented May 16, 2018

abonander commented May 16, 2018

bors commented May 16, 2018

F001 commented May 16, 2018

nikomatsakis commented May 16, 2018

nikomatsakis commented May 16, 2018

F001 commented May 18, 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

bors commented May 19, 2018

Choose a reason for hiding this comment

petrochenkov commented May 20, 2018

bors commented May 20, 2018

F001 commented May 22, 2018

petrochenkov commented May 22, 2018

petrochenkov commented May 22, 2018

F001 commented May 23, 2018

bors commented May 23, 2018

bors commented May 23, 2018

bors commented May 23, 2018

F001 May 14, 2018 •

edited

Loading