Add error codes duplicates check

[GuillaumeGomez]

As asked in #67086.

r? @Mark-Simulacrum

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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.

2020-01-29T12:42:01.0953297Z ========================== Starting Command Output ===========================
2020-01-29T12:42:01.0955171Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/ca148837-195a-4f9d-988b-dec431d07497.sh
2020-01-29T12:42:01.0955358Z 
2020-01-29T12:42:01.0958410Z ##[section]Finishing: Disable git automatic line ending conversion
2020-01-29T12:42:01.0963754Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68639/merge to s
2020-01-29T12:42:01.0965227Z Task         : Get sources
2020-01-29T12:42:01.0965303Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-01-29T12:42:01.0965336Z Version      : 1.0.0
2020-01-29T12:42:01.0965369Z Author       : Microsoft
---
2020-01-29T12:42:01.8796952Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-29T12:42:01.8805865Z ##[command]git config gc.auto 0
2020-01-29T12:42:01.8807995Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-29T12:42:01.8810440Z ##[command]git config --get-all http.proxy
2020-01-29T12:42:01.8816029Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68639/merge:refs/remotes/pull/68639/merge

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)

[GuillaumeGomez]

Fixed formatting issue.

[Mark-Simulacrum]

I don't have much time to do review here, so would appreciate a more elaborate PR description that tells me:

• what this is doing ("duplicates check" is a bit vague, or at least I'm not sure I understand it)
• in particular, e.g. a description of exactly the conditions intended to be caught, and the cases that we are not intending to catch
• brief description of what we had before that (may) overlap with this, i.e., other error code checks

[GuillaumeGomez]

It counts the occurences of all error code, and detects the duplicates: you're not supposed to be able to use a same error code in two different places (otherwise, we might use one error code for two different errors).

So the check is pretty simple in itself: we read files, count the occurrences and check the numbers.

Before that, it was done through a macro which has been removed since then.

[Mark-Simulacrum]

Looks like this currently depends on the other PR, too, as we currently have non-stringy error codes.

[Mark-Simulacrum]

Can we instead push into the errors vec instead of having two separate modes?

[Mark-Simulacrum]

Do we have consensus as to not having duplicate error code usage? I personally feel like that's a pretty odd policy to have, but if @estebank and @rust-lang/wg-diagnostics feel that we should enforce this then I'm not opposed.

[oli-obk]

The policy existed for error codes because it was easy to mix up numbers. If we move to stringy error codes, all we need is a check that each has a test emitting it.

[Mark-Simulacrum]

@oli-obk That's only true if we truly stringify error codes (e.g. borrow-check-use-after-drop), not the current "E0303", right? (And would need a different parser anyway).

But if mixing up number is the concern, then I guess we can do this. I still feel like that's something that can be fixed whenever it comes up and this doesn't in practice help much, but I also don't usually write errors or diagnostics in general :)

[oli-obk]

But if mixing up number is the concern, then I guess we can do this. I still feel like that's something that can be fixed whenever it comes up and this doesn't in practice help much, but I also don't usually write errors or diagnostics in general :)

We've never had problems with this because we've had this check. I have no idea if it would be a problem in practice ;)

[Mark-Simulacrum]

We've not had this check for... some time now, right? In any case, I guess it doesn't matter. I'm fine with landing this (once my comments are resolved), and I probably also need to do another review pass after that to make sure the implementation is doing what it says it is.

[Mark-Simulacrum]

I think there was some discussion in Zulip as well, but I don't have time to track that down anyway. The code here is (loosely) waiting on the diagnostics WG anyway probably or blocked on the other PR. Moving to waiting-on-team.

[crlf0710]

@rustbot modify labels to +T-compiler

[pnkfelix]

@rustbot assign @oli-obk

[pnkfelix]

Discussed in T-compiler meeting. reassigning to @oli-obk as member of WG-diagnostics to decide whether to close as unnecessary, or to land.

[oli-obk]

Closing as we haven't had this check for quite a while and don't seem to have problems without it.

[GuillaumeGomez]

I strongly disagree on this opinion: before I migrated to the librustc_error_code, we had a check performed at compile-time through a macro. We lost this check and I had already to write fixes about new errors added (with no explanation) which already existed. So definitely, this is needed so I'll reopen the PR.

[Mark-Simulacrum]

I had already to write fixes about new errors added (with no explanation) which already existed

I am confused by this -- I thought this lint was for accidentally reusing the same error code (accidentally), not about not having an error code? I've just re-read your comment #68639 (comment) and I feel like it matches my understanding, so it may be helpful to open a new issue or PR instead? Or maybe I'm just misunderstanding...

[GuillaumeGomez]

The other lint checks if all error codes have an explanation. This lint checks that each error code is only used once.

[Centril]

I am in agreement with @oli-obk and @Mark-Simulacrum. One thing I noted at the meeting is that disallowing reusing an error code can have ungreat architectural implications force you have to have fn foo_error(..) -> DiagnosticBuilder<'_> and that can harm the effort towards #65031.

[GuillaumeGomez]

At this point, I just wonder why I try to maintain/clean up this whole error system. No one wants it and no one wants to make another. That's one problem less off my shoulders. Good luck!