Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need compare-mode=2018 (or similar) to run test suite under multiple editions #52979

Open
pnkfelix opened this issue Aug 2, 2018 · 5 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Aug 2, 2018

To resolve #48879 we added --compare-mode=nll. It allows us to keep track of whether the diagnostics deviate, and in what fashion, for the whole ui/ test suite.

We should have something similar for the 2018 edition. That would force us to be honest about how much things are going to change for people who try to upgrade their edition.

(Also, I believe such a mode would have caught #52967)


The new compare-mode=2018 could either be added as a new thing on its own, or we could replace the existing compare-mode=nll with compare-mode=2018.

  • The main motivation I can imagine for replacing rather than a pure addition would be to minimize the impact on the time our autobuilds spend in the ui/ test suite. I would like feedback from @rust-lang/infra about which route is best here.
@pietroalbini pietroalbini added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 2, 2018
@pnkfelix
Copy link
Member Author

pnkfelix commented Aug 2, 2018

There's a policy question here in terms of what to do with tests that are clearly making use of syntax that will need to be rustfix'ed to work with the 2018 edition.

For example, my initial draft of trying to make a compare-mode=2018 currently has a failure on a test case like this:

#![feature(use_nested_groups)]
#![allow(dead_code)]
#![deny(unused_imports)]
mod foo {
pub mod bar {
pub mod baz {
pub struct Bar();
}
pub mod foobar {}
}
pub struct Foo();
}
use foo::{Foo, bar::{baz::{}, foobar::*}, *};
//~^ ERROR unused imports: `*`, `Foo`, `baz::{}`, `foobar::*`
use foo::bar::baz::{*, *};
//~^ ERROR unused import: `*`
use foo::{};
//~^ ERROR unused import: `use foo::{};`
fn main() {
let _: Bar;
}

The problem is that when you compile this file adding --edition 2018, you (as expected) get a totally different error message, one that complains about not being able to find a crate for foo.

So the policy question that leads me to is:

  • Should one just add a .2018.stderr file for such a test that encodes the error message that we generate here?
  • Or should one treat this test as fundamentally linked to the 2015 edition, and add // ignore-test-compare-mode-2018 (and probably also add // edition:2015) to its test headers?
    • If we take the latter route, it probably behooves us to make a 2018-compatible version of the same test, right? By, in this particular case, prepending (if I understand correct) self:: to the paths in the relevant use items.
  • But that last point then raises a new question: Is the right thing here in fact to change the test to use some syntax that is compatible with both the 2015 and 2018 edition, and thus try to make it focus on producing the same error in both cases?

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 4, 2018

Another potential option: the compare-mode=2018 could run rustfix itself, either unconditionally on each input, or as a fallback after an unexpected error or diagnostic mismatch...

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 4, 2018

Reading over #50084, it sounds like there is supposed to already be a foo.fixed file present when we expect rustfix to be run on the input.

So maybe that is an answer: compare-mode=2018 should check if there is foo.fixed next to the foo.rs, and if so, use foo.fixed as its compile input (and then use the usual fallback logic for finding which .stdout and .stderr to expect). If there is no foo.fixed, then use foo.rs as the compile input.

This avoids the need to run rustfix ourselves, while also forcing the test data to keep a concrete record of the cases where we expect rustfix to get run.

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 6, 2018

Private experimentation along with discussions with @alexcrichton made me realize that you cannot always get useful rustfix results from input source that does not compile.

In the general case, you need to start with something that compiles, and then you should be able to see the diagnostics that rustfix uses for rewriting the code.

This implies that compare-mode=2018 is almost certainly not going to be applicable to the // compile-fail tests (which is the default for the ui/ tests)

Having said that, we can still apply it to just the tests that are marked // run-pass (or // compile-pass). and that may still provide useful information.

@pnkfelix
Copy link
Member Author

pnkfelix commented Nov 2, 2018

Now that RC2 is out, I'm wondering whether there is enough time to actually implement this and get value from it... It seems unlikely, especially since the biggest issue (the migration mode imposed by NLL) is now already covered by the change from #55118

@Mark-Simulacrum Mark-Simulacrum added the A-testsuite Area: The testsuite used to check the correctness of rustc label Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management
Projects
None yet
Development

No branches or pull requests

4 participants