Add check for missing tests for error codes #65135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some changes occurred in diagnostic error codes |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.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 |
|
I think there's some similar code in tidy already (with a plausibly similar purpose? Not sure). Could you move this into tidy instead of adding another crate? |
|
Sure! I wanted to ask you a few questions about to handle this but you answered it. :) |
GuillaumeGomez
force-pushed
the
add-error-code-check
branch
from
35a70c0 to
5ecc0d3
Compare
|
Moved it into tidy. |
|
The job Click to expand the log.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 |
|
☔ The latest upstream changes (presumably #65152) made this pull request unmergeable. Please resolve the merge conflicts. |
GuillaumeGomez
force-pushed
the
add-error-code-check
branch
from
5ecc0d3 to
c4b3a4f
Compare
|
The job Click to expand the log.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 |
GuillaumeGomez
force-pushed
the
add-error-code-check
branch
from
c4b3a4f to
8f70805
Compare
|
Conflicts... |
| impl Foo for MyStruct { | ||
| const BAR: f64 = 0f64; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can this be moved to the next commit?
| use std::path::Path; | ||
|
|
||
| const WHITELIST: &[&'static str] = &[ | ||
| "E0183", |
There was a problem hiding this comment.
Could we add a comment to this whitelist? Should we aim to remove it eventually? Is it "we know these are actually fine but we can't detect it"?
There was a problem hiding this comment.
Some of those tests can't be tested but a lot of could and should. I'll add a comment in that sense.
| "E0729", | ||
| ]; | ||
|
|
||
| fn visit_dirs(dir: &Path, level: usize, cb: &mut dyn FnMut(&DirEntry)) -> io::Result<()> { |
There was a problem hiding this comment.
There should be existing helpers in tidy that do this. For a good example look at src/tools/tidy/src/errors.rs.
There was a problem hiding this comment.
Indeed, code is much smaller now.
| if let Some(err_code) = s.splitn(2, ']').next() { | ||
| if let Some(err_code) = err_code.splitn(2, '[').skip(1).next() { | ||
| let nb = error_codes.entry(err_code.to_owned()).or_insert(0); | ||
| *nb += 1; |
There was a problem hiding this comment.
Why keep a count instead of bool yes/no? We appear to only compare with 0...
There was a problem hiding this comment.
For now. I have in mind to use this to track error codes with no long error explanation. But for now a bool would be enough so I'll update it.
| use std::fs::{self, DirEntry, File}; | ||
| use std::path::Path; | ||
|
|
||
| const WHITELIST: &[&'static str] = &[ |
There was a problem hiding this comment.
You don't need the 'static here.
There was a problem hiding this comment.
Compiler is evolving too fast for me...
| errors.push(format!("Error code {} needs to have at least one UI test!", err_code)); | ||
| } | ||
| } | ||
| errors.sort(); |
There was a problem hiding this comment.
This isn't really all that helpful I imagine to get these in a sorted order -- most of the time, you're probably only going to see one or two -- can we just print immediately? If we care so much, then we can have error_codes be a BTreeSet which we delete items from when finding the error code in tests.
There was a problem hiding this comment.
No it's very useful since they are not sorted at all. For example, if you read an error_codes.rs file and it has both long error explanations and error code declaration, the order is already broken. Also, please keep in mind that we also read from two different contexts: ui tests and long error explanations. If we remove it because we found a test and then find it again with no test (just an error code declaration for instance), then it'll throw an error.
There was a problem hiding this comment.
It also made my life a lot simpler when I had to go through spotted errors. :)
There was a problem hiding this comment.
I don't quite understand why sorting is useful (since we're sorting by error code, which has no relation to where the code is declared, at least for the most part); however, if you feel it is, then that's fine. I would still like to see us move to the BTreeSet and removal from it as we find tests, since I think that'll be (much) cleaner.
There was a problem hiding this comment.
We can't move to a BTreeSet. For example:
- You find the ui test file with the error code, no need to add it into the BTreeSet so we continue.
- You find the same error code declaration (so no long error explanation and therefore, no test) in an
error_codes.rsfile. No test, you add it into the BTreeSet. - The check fails because this error code (which has a test) is detected as not having tests.
Or maybe I misunderstood what you tried to explain to me?
There was a problem hiding this comment.
Ah, I was expecting to do this in two passes - discover error codes in one pass and then use that information. But since we're not already doing that lets leave it as is for now.
GuillaumeGomez
force-pushed
the
add-error-code-check
branch
from
8f70805 to
5fbb3fd
Compare
100 tasks
Centril
reviewed
Oct 6, 2019
src/librustc_mir/error_codes.rs
Outdated
| @@ -953,6 +953,8 @@ https://doc.rust-lang.org/std/cell/ | |||
| "##, | |||
|
|
|||
| E0388: r##" | |||
| #### Note: this error code is no longer emitted by the compiler. | |||
|
|
|||
| E0388 was removed and is no longer issued. | |||
There was a problem hiding this comment.
Says the same thing as above.
|
☔ The latest upstream changes (presumably #65169) made this pull request unmergeable. Please resolve the merge conflicts. |
GuillaumeGomez
force-pushed
the
add-error-code-check
branch
from
5fbb3fd to
7a84158
Compare
|
Updated. |
|
@bors r+ Thanks! |
|
📌 Commit 7a84158 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 8, 2019
…, r=Mark-Simulacrum Add check for missing tests for error codes Fixes rust-lang#64811. r? @Mark-Simulacrum
bors
added a commit
that referenced
this pull request
Oct 8, 2019
Rollup of 7 pull requests Successful merges: - #64284 (Warn if include macro fails to include entire file) - #65081 (Remove -Zprofile-queries) - #65133 (typeck: prohibit foreign statics w/ generics) - #65135 (Add check for missing tests for error codes) - #65141 (Replace code of conduct with link) - #65194 (Use structured suggestion for removal of `as_str()` call) - #65213 (Ignore `ExprKind::DropTemps` for some ref suggestions) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #64811.
r? @Mark-Simulacrum