Add edition configuration to compiletest #90070
Conversation
rust-highfive
added
the
S-waiting-on-review
llogiq
force-pushed
the
compiletest-config-edition
branch
from
a629541 to
face139
Compare
|
@Manishearth did you mean to r+? |
|
Out of interest, is there a reason clippy want to blanket upgrade tests? rustc has been pretty happy I think with the existing defaults. |
|
@llogiq nah because I wanted to check again on desktop important note: clippy uses https://github.com/Manishearth/compiletest-rs . I'd love to share compiletest with that of rustc but that has been hard to make happen. |
|
Anyway, I'd merge this if this were on compiletest-rs, i'll let the compiler team review it here |
|
The reason is we want to use the new edition for all of our tests and writing an edition header in 548 files gets old rather quickly. |
|
@Manishearth should I push the same PR to compiletest-rs? |
|
Yes please |
Yes, but I'm asking why you have that goal. Happy to be pointed at PR/issue comments. |
|
Mostly because of rust-lang/rust-clippy#7842 |
|
We could do it manually too |
|
That issue does not have justification for why you want that. I'm asking because that reasoning may be useful for rustc itself, but I haven't seen it yet - that issue just states the fact... |
|
Apart from that, I see it as a win for usability – being able to quickly check if all tests not explicitly tied to an edition still run with a new one ought to be worth those few easy to understand lines of code. |
|
I'm happy to r+ this to be clear, but without a better understanding of why you want this it's hard to know whether this is the right way to go about it. For example, the desire to verify cross-edition compatibility seems better done with a compare-mode similar to the one for NLL which ensures outputs are the same and such. Obviously, you can sort of emulate it manually, but if the goal is to ensure that compatibility going forward then that's likely the way to go. If there's no real reason for the desire to migrate all tests to 2021, that's fine, we can just accept this, but I am feeling somewhat baffled by the repeated lack of justification presented (or acknowledgement that it isn't really justified by rationale) despite repeated attempts to seek it. |
|
@Mark-Simulacrum I don't think we're asking for an r+ here anymore; though I'm sure @llogiq is happy to keep this open if y'all end up interested in taking this. I'm a bit confused as to why you would find a lack of justification provided within 15 hours to be a "baffling repeated lack of justification". The justification for that issue is that we've had a couple things silently break on the latest edition (like rust-lang/rust-clippy#7723) and it seems reasonable to switch defaults so that most tests are on the latest edition unless they're testing something more edition-specific. Overall the latest edition is roughly what people are using themselves, crates may stay on old editions but typically if someone is hacking on them they'll upgrade eventually, and unlike rustc clippy only really applies to folks actively hacking on something. We'd like to be testing where the actual users are at. We could run tests on all editions but that would take much longer for very little benefit in most cases, so setting the default and then using that process to find tests that should be running on different or multiple editions can be done later. |
To be clear, I was confused by the responses to my question without providing the very useful context you just posted. I don't expect quick justification, but I was confused by the responses which implicitly seemed to be justification (but didn't have it in my opinion). I was happy to wait, but the replies given seemed to try to respond but not actually do so. It makes sense to me that there'd be bugs like this -- particularly given the much tighter integration Clippy generally has with end-user code -- but it seems like you'd want to ensure that tests that fail on new editions are still working on old editions. I guess you can manually bump edition and then use revisions to continue testing older editions on the subset of tests which fail, it's certainly more economical than blanket running tests 3 times (2015/2018/2021). @bors r+ rollup |
|
📌 Commit face139be09be9ad05acbfb2a40075d1d77ac000 has been approved by |
bors
added
S-waiting-on-bors
llogiq
force-pushed
the
compiletest-config-edition
branch
from
face139 to
dd22b67
Compare
This comment has been minimized.
This comment has been minimized.
llogiq
force-pushed
the
compiletest-config-edition
branch
from
dd22b67 to
65b3c85
Compare
|
Sorry @Mark-Simulacrum I failed to remove spurious whitespace. It should now build correctly. |
|
@bors r+ rollup |
|
📌 Commit 65b3c85 has been approved by |
…r=Mark-Simulacrum Add edition configuration to compiletest This allows the compiletest configuration to set a default edition that can still be overridden with header annotations. Doing this will make it far easier for clippy to get our tests to the newest edition. r? `@Manishearth`
…r=Mark-Simulacrum Add edition configuration to compiletest This allows the compiletest configuration to set a default edition that can still be overridden with header annotations. Doing this will make it far easier for clippy to get our tests to the newest edition. r? ``@Manishearth``
…askrgr Rollup of 11 pull requests Successful merges: - rust-lang#83233 (Implement split_array and split_array_mut) - rust-lang#88300 (Stabilise unix_process_wait_more, extra ExitStatusExt methods) - rust-lang#89416 (nice_region_error: Include lifetime placeholders in error output) - rust-lang#89468 (Report fatal lexer errors in `--cfg` command line arguments) - rust-lang#89730 (add feature flag for `type_changing_struct_update`) - rust-lang#89920 (Implement -Z location-detail flag) - rust-lang#90070 (Add edition configuration to compiletest) - rust-lang#90087 (Sync rustfmt subtree) - rust-lang#90117 (Make RSplit<T, P>: Clone not require T: Clone) - rust-lang#90122 (CI: make docker cache download and `docker load` time out after 10 minutes) - rust-lang#90166 (Add comment documenting why we can't use a simpler solution) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This allows the compiletest configuration to set a default edition that can still be overridden with header annotations. Doing this will make it far easier for clippy to get our tests to the newest edition.
r? @Manishearth