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

Rustup to rustc 1.41.0-nightly (35ef33a8 2019-11-21) #4825

Merged
merged 9 commits into from
Nov 23, 2019

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Nov 18, 2019

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:

changelog: none

@Manishearth
Copy link
Member Author

Working on getting the compiletest stuff fixed, does someone want to look at the write/print test failures?

@phansch

This comment has been minimized.

@flip1995
Copy link
Member

flip1995 commented Nov 18, 2019

I did pretty much the same as this PR, but was stuck at the tester thing in compiletest.

Do we really want to depend on tester? I think, we should keep hiding it behind the #[cfg(not(rustc))] in compiletest_rs.

cc Manishearth/compiletest-rs#203 (comment)

@Manishearth
Copy link
Member Author

Using the rustc feature caused similar issues, and it would be nice to never be broken by a libtest change again.

@Manishearth
Copy link
Member Author

If we can land this with the rustc feature that's fine too. But we still have an ICE and test failures on the print/write tests.

They may have to do with the difference between litstr.symbol and litstr.symbol_unescaped

@flip1995
Copy link
Member

Both the rustc feature and the tester dep require a change in compiletest or tester respectively, so we can just go for tester, if that will be the future way to go with compiletest.

What test failures? All the print/write ui-tests are green. What am I missing?

@flip1995
Copy link
Member

require a change in compiletest or tester respectively

except we just ignore the failing ui-test for now, of course.

@Manishearth
Copy link
Member Author

Hmm, I was getting test failures locally with this branch

@flip1995
Copy link
Member

flip1995 commented Nov 18, 2019

Should we r+ this with module_name_repetitions test ignored and fix it later, once tester is renamed?

Hmm, I was getting test failures locally with this branch

Weird... I didn't and travis is also green 🤔

@Manishearth
Copy link
Member Author

Manishearth commented Nov 18, 2019 via email

@flip1995
Copy link
Member

I'm only on mobile right now. I'll get back to this tomorrow (if no one else did it before that)

@Manishearth Manishearth changed the title [WIP] Rustup to rustc 1.41.0-nightly (a0d40f8bd 2019-11-18) Rustup to rustc 1.41.0-nightly (a0d40f8bd 2019-11-18) Nov 19, 2019
@Manishearth
Copy link
Member Author

@bors r=flip1995

turns out there were no write failures 🤷‍♂️

@bors
Copy link
Collaborator

bors commented Nov 19, 2019

📌 Commit aa5a95f has been approved by flip1995

bors added a commit that referenced this pull request Nov 19, 2019
Rustup to rustc 1.41.0-nightly (a0d40f8bd 2019-11-18)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

changelog: none
@bors
Copy link
Collaborator

bors commented Nov 19, 2019

⌛ Testing commit aa5a95f with merge 37a8749...

@bors
Copy link
Collaborator

bors commented Nov 19, 2019

💔 Test failed - status-appveyor

@Manishearth
Copy link
Member Author

New failures have come in, fixed one of them but redundant_clone is still failing and I'm going to bed for now.

@@ -32,11 +32,8 @@ fn random_caller() -> u32 {

static Y: u32 = 0;

// We should not suggest to make this function `const` because const functions are not allowed to
// refer to a static variable
fn get_y() -> u32 {
Copy link
Member

@flip1995 flip1995 Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be linted still. Adding const to this function gives the error described in the comment. But for some reason rustc_mir::transform::qualify_min_const_fn::is_min_const_fn returns Ok(()) for this function:

let mir = cx.tcx.optimized_mir(def_id);
if let Err((span, err)) = is_min_const_fn(cx.tcx, def_id, &mir) {

cc @oli-obk Does optimized_mir replace the static with a const value and is_min_const_fn then thinks this could be a const fn?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did something change in the handling of Statements in MIR recently? The error in redundant_clone is similar. This code

fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext, _: mir::Location) {
match ctx {
PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return,
_ => {},
}
if *local == self.local {
self.used_other_than_drop = true;
}
}

stopped detecting, that in (a.clone(), a) both as are the same.

Copy link
Member

@flip1995 flip1995 Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean with

The error [...] is similar

is, that in both cases the code that should check for them is never actually reached by the traversal of the Statement (or the Terminator?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh... we run is_min_const_fn on the optimized mir while rustc runs it on mir_const. This will keep causing problems. One thing clippy could do is to make the optimized_mir query not do any optimizations. This is possible since rust-lang/rust#66297

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how do I override the optimized_mir query? When I just copy and paste its source code [1] from rustc and remove the run_optimization_passes call¹, a lot of tests began to ICE. I added this:

        config.override_queries = Some(|_, providers, _| {
            providers.optimized_mir = |tcx, def_id| {
                // (Mir-)Borrowck uses `mir_validated`, so we have to force it to
                // execute before we can steal.
                tcx.ensure().mir_borrowck(def_id);

                let (body, _) = tcx.mir_validated(def_id);
                let body = body.steal();
                tcx.arena.alloc(body)
            }
        });

to

impl rustc_driver::Callbacks for ClippyCallbacks {
fn config(&mut self, config: &mut interface::Config) {


¹ I also removed this:

    if tcx.is_constructor(def_id) {
        // There's no reason to run all of the MIR passes on constructors when
        // we can just output the MIR we want directly. This also saves const
        // qualification and borrow checking the trouble of special casing
        // constructors.
        return shim::build_adt_ctor(tcx, def_id);
    }

because the shim module is private.


[1] https://github.com/rust-lang/rust/blob/9d6ff1553b7debbe5c99c102ce0978b6130592f8/src/librustc_mir/transform/mod.rs#L316-L333

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. So we'd need to make more things public. But that's probably not the failure reason. One thing you can try is set the mir opt level to 0 instead of trying to replace the query. If that doesn't work, reintroduce the mir optimizations but remove all the ones that break clippy (e.g. const_prop)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing you can try is set the mir opt level to 0

How do I set the opt level? And how can I enable/disable mir passes?

@bors
Copy link
Collaborator

bors commented Nov 23, 2019

⌛ Testing commit 7bae5bd with merge 19769b5...

bors added a commit that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a8 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
@bors
Copy link
Collaborator

bors commented Nov 23, 2019

💔 Test failed - checks-travis

@flip1995
Copy link
Member

@bors retry

bors added a commit that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a8 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
@bors
Copy link
Collaborator

bors commented Nov 23, 2019

⌛ Testing commit 7bae5bd with merge 3870050...

@bors
Copy link
Collaborator

bors commented Nov 23, 2019

💔 Test failed - status-appveyor

@flip1995
Copy link
Member

uh that is bad: https://travis-ci.com/rust-lang/rust-clippy/jobs/259722402#L5480-L5487

Since when do we lint the output of build scripts? 😮

@flip1995
Copy link
Member

This is even worse: https://travis-ci.com/rust-lang/rust-clippy/jobs/259722402#L7287

The match_same_arms lint triggers once for every combination of identical match arms, so

match a {
    1 => (),
    2 => (),
    3 => (),
    _ => (),
}

would trigger 6 times: 2<->1, 3<->1, 3<->2, _<->1, _<->2, _<->3. But that's an issue for another day.

@flip1995
Copy link
Member

Since when do we lint the output of build scripts?

It seems since ever. We probably shouldn't dump the Clippy output to the travis terminal though.

bors added a commit that referenced this pull request Nov 23, 2019
[WIP] RIIR: Integration tests

In #4825 the `rust-lang/chalk` test failed because the output was too large. I didn't want to completely disabling the output, since showing the backtrace of an ICE directly in travis is pretty useful. Since finding strings in command outputs is easier in Rust, than in bash, I just RIIRed it.

This and also rewriting our tests in Rust may help with trying out new CI platforms (cc #4577)

Based on #4825, so I could test it.

changelog: none
@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 23, 2019

📌 Commit 553db87 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Nov 23, 2019

⌛ Testing commit 553db87 with merge 60e8413...

bors added a commit that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a8 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
@bors
Copy link
Collaborator

bors commented Nov 23, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 60e8413 to master...

@bors bors merged commit 553db87 into rust-lang:master Nov 23, 2019
bors added a commit that referenced this pull request Nov 23, 2019
[WIP] RIIR: Integration tests

In #4825 the `rust-lang/chalk` test failed because the output was too large. I didn't want to completely disabling the output, since showing the backtrace of an ICE directly in travis is pretty useful. Since finding strings in command outputs is easier in Rust, than in bash, I just RIIRed it.

This and also rewriting our tests in Rust may help with trying out new CI platforms (cc #4577)

changelog: none
bors added a commit that referenced this pull request Nov 25, 2019
[WIP] RIIR: Integration tests

In #4825 the `rust-lang/chalk` test failed because the output was too large. I didn't want to completely disabling the output, since showing the backtrace of an ICE directly in travis is pretty useful. Since finding strings in command outputs is easier in Rust, than in bash, I just RIIRed it.

This and also rewriting our tests in Rust may help with trying out new CI platforms (cc #4577)

changelog: none
bors added a commit that referenced this pull request Nov 28, 2019
RIIR: Integration tests

In #4825 the `rust-lang/chalk` test failed because the output was too large. I didn't want to completely disabling the output, since showing the backtrace of an ICE directly in travis is pretty useful. Since finding strings in command outputs is easier in Rust, than in bash, I just RIIRed it.

This and also rewriting our tests in Rust may help with trying out new CI platforms (cc #4577)

changelog: none
@Manishearth Manishearth deleted the rustup branch November 28, 2019 15:21
bors added a commit that referenced this pull request Nov 29, 2019
RIIR: Integration tests

In #4825 the `rust-lang/chalk` test failed because the output was too large. I didn't want to completely disabling the output, since showing the backtrace of an ICE directly in travis is pretty useful. Since finding strings in command outputs is easier in Rust, than in bash, I just RIIRed it.

This and also rewriting our tests in Rust may help with trying out new CI platforms (cc #4577)

changelog: none
bors added a commit that referenced this pull request Nov 29, 2019
RIIR: Integration tests

In #4825 the `rust-lang/chalk` test failed because the output was too large. I didn't want to completely disabling the output, since showing the backtrace of an ICE directly in travis is pretty useful. Since finding strings in command outputs is easier in Rust, than in bash, I just RIIRed it.

This and also rewriting our tests in Rust may help with trying out new CI platforms (cc #4577)

changelog: none
bors added a commit that referenced this pull request Jan 8, 2020
Fix redundant_clone lint not working with PathBuf and OsString

#4825 diabled MIR optimization in clippy, including `rustc_mir::transform::InstCombine` which reduces `&(*x)` to `x`. This PR tries to unwrap `&*` when looking into `mir::Rvalue`s.

Fixes #5014.

---

changelog: fixed `redundant_clone` lint not working with `PathBuf` and `OsString`
bors added a commit that referenced this pull request Jan 8, 2020
Fix redundant_clone lint not working with PathBuf and OsString

#4825 diabled MIR optimization in clippy, including `rustc_mir::transform::InstCombine` which reduces `&(*x)` to `x`. This PR tries to unwrap `&*` when looking into `mir::Rvalue`s.

Fixes #5014.

---

changelog: fixed `redundant_clone` lint not working with `PathBuf` and `OsString`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants