Duplicate error code usage lint removed

[bjorn3]

https://github.com/rust-lang/rust/commit/b437240ceefaad3cdf92ad7e9d1255b8da88dbb3#diff-57a272214ca8c5aee61b8ce7e023a336L43

https://github.com/rust-lang/rust/commit/b437240ceefaad3cdf92ad7e9d1255b8da88dbb3#r35071110

cc @Mark-Simulacrum

[Mark-Simulacrum]

Before I go and re-implement this (I imagine it'll be somewhat annoying to do so, fwiw), I'd like @GuillaumeGomez and @estebank (and maybe others?) to give feedback on the usefulness of such an error. If we deem it useful, then I can work on an impl.

[Mark-Simulacrum]

Maybe worth a compiler team nomination? Not sure whose domain this is.

[Centril]

Not sure whose domain this is.

The compiler team.

(Do you need to maintain state between macro invocations to fix this?)

[Mark-Simulacrum]

Well, sure, compiler team but I was hoping for less checkboxes so to speak.

I would implement this via a "global" though per-crate (lazy) static that has a hashset of (line, code). So it would be a runtime ICE vs compiletime error like before; I don't know of a way to do this at compiletime today without depending on something we explicitly want to disallow (state between proc macro invocations).

[Centril]

I don't know of a way to do this at compiletime today without depending on something we explicitly want to disallow (state between proc macro invocations).

I personally don't feel duplicated error codes is bad if they are semantically similar so imo let's skip the lint.

[Centril]

Well, sure, compiler team but I was hoping for less checkboxes so to speak.

cc @estebank @oli-obk

[Mark-Simulacrum]

I personally don't feel duplicated error codes is bad if they are semantically similar so imo let's skip the lint.

Yeah, I have the same opinion. It feels like this lint doesn't really help, since it doesn't catch bugs in practice -- typos, I guess -- but UI tests mostly cover "you emitted the wrong diagnostic during refactoring" and otherwise we can fix things as needed I feel.

[Mark-Simulacrum]

Assigning to myself so I don't forget about this and going to nominate for compiler team as a stopgap.

Edit: maybe just an fcp to close from someone on the compiler team would work here

[estebank]

It is a necessary lint when merging to master, because two different PRs adding a new EXXXX will always end up using the same one and we can't hope merge conflicts to always catch these cases because they can easily be in different crates.

[Mark-Simulacrum]

How would we consider use sites equivalent? My proposal of "by line" doesn't work well if you're producing the same error code multiple times in a single file (or, possibly, spread amongst multiple files).

We can make cross-crate overlapping definitions for error codes an error since we collect all error codes/descriptions for --explain though; wouldn't that solve the unseen merge conflict problem? (Note, this would even be an enhancement over the previous code as I believe it only checked for duplicate definitions in the same crate).

[Mark-Simulacrum]

In fact, I believe the current code is equally powerful at detecting definition duplication at compile time, and detecting use site duplication is as yet unclear what semantics we want.

I believe tidy might be ensuring definitions are unique cross-crate, or at least trying to; I can try to formalize that and ensure it works.

[pnkfelix]

@Mark-Simulacrum asked:

We can make cross-crate overlapping definitions for error codes an error since we collect all error codes/descriptions for --explain though; wouldn't that solve the unseen merge conflict problem?

Wouldn't you still face the problem of the duplication arising within a single crate? Are you assuming that the PR merging process would always catch that as a merge conflict? (Because I'm not sure that holds in general; people don't necessarily plug the new error codes that they allocated into the same rustc source line, right?)

---

Just to check my understanding, it seems like we are discussing a tension between providing ease of writing diagnostic messages vs reducing risk of assigning the same error code to distinct diagnostics. Does that sound correct?

[pnkfelix]

triage: P-high (to resolve the question of whether we still want the lint or not), leaving marked as I-nominated.

[varkor]

Wouldn't you still face the problem of the duplication arising within a single crate?

These should already error, as an error code definition can only be provided once by syntax::register_diagnostics!, no? Throwing an error for duplicate definitions rather than duplicate usages seems completely sufficient for detecting merge conflicts.

[Mark-Simulacrum]

Yes, the current state is that we will catch definition duplication within a single crate by error code, and cross-crate ignore it (silently skipping the second definition). This is easily fixable to instead panic rather than ignoring second definition in a different crate.

[oli-obk]

Could we just move to error names similar to lint names? If names collide that seems either intentional or the naming was poorly chosen, but the likelyhood of collisions should go down significantly.

[Mark-Simulacrum]

I would prefer to leave that discussion out of scope for this issue. It seems like a good fit for a planning meeting perhaps?

[pnkfelix]

This issue was discussed in last week's compiler meeting. Main conclusion/action item was: We want to make sure we check for no duplicate definitions but otherwise we will not readd functionality of use site checking

[pnkfelix]

triage: Now that the question has been resolved, removing nomination label and downgrading to P-medium.

[estebank]

One thing I don't see mentioned is the lint for at least one usage being present. Is that lint still in place?

[Mark-Simulacrum]

Yes, if there are no uses of an error code within the crate then we'll error out (via the normal unused checking).