Hacky rustup #3905
Hacky rustup #3905
Conversation
| should_panic: libtest::ShouldPanic::No, | ||
| }, | ||
| // oli obk giving up | ||
| testfn: unsafe { std::mem::transmute(test.testfn) }, |
There was a problem hiding this comment.
I would put up an issue about this, there's probably someone who already knows how to write it without transmute right? 🤔
There was a problem hiding this comment.
I think oli-obk meant opening issues only for broken tests, it's probably not possible to avoid transumte until libtest gets fixed.
There was a problem hiding this comment.
Ah, I see. It's just one line though, it doesn't seem that bad right?
|
I'm trying to fix the test failures, but there's one that seems very suspicious. This part in the ExprKind::Call(ref path, ref args) => {
if let ExprKind::Path(ref qpath) = path.node {
if let Some(def_id) = resolve_node(cx, qpath, path.hir_id).opt_def_id() {
if match_def_path(cx.tcx, def_id, &paths::FROM_FROM[..]) {
let a = cx.tables.expr_ty(e);
let b = cx.tables.expr_ty(&args[0]);
if same_tys(cx, a, b) {
let sugg = snippet(cx, args[0].span.source_callsite(), "<expr>").into_owned();
let sugg_msg =
format!("consider removing `{}()`", snippet(cx, path.span, "From::from"));
span_lint_and_then(cx, IDENTITY_CONVERSION, e.span, "identical conversion", |db| {
db.span_suggestion(
e.span,
&sugg_msg,
sugg,
Applicability::MachineApplicable, // snippet
);
});
}
}
}
}
},Because ---- dogfood stdout ----
status: exit code: 101
stdout:
stderr: Checking clippy v0.0.212 (/home/felix/Documents/Programming/Rust/clippy_work/clean_clippy)
error: identical conversion
--> tests/compile-test.rs:80:16
|
80 | for dir in fs::read_dir(&config.src_base)? {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `fs::read_dir(&config.src_base)?()`: `fs::read_dir(&config.src_base)?`
|
= note: `-D clippy::identity-conversion` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
error: identical conversion
--> tests/compile-test.rs:83:21
|
83 | for file in fs::read_dir(&dir_path)? {
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `fs::read_dir(&dir_path)?()`: `fs::read_dir(&dir_path)?`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
error: aborting due to 2 previous errorsThe error is nonsensical. I think I can make a regression test for this. Anyone knows why the lint produces this error, though? I'll try to take a deeper look later in the day :) |
|
That error should not actually be emitted. The macro checks seem to be failing. Either it needs a macro check, or rustc stopped emitting macro expansion information and we need to fix it in rustc. |
|
Oh no, that's bad. Do you know where should I report that error (specifically, what part of the |
|
PS: I added some fixes at #3906 |
|
I don't have time to debug it further but Rust may be broken indeed. rust-clippy/clippy_lints/src/methods/mod.rs Line 865 in 61aa5c9 Works: pub fn main() {
let _ = [1, 2, 3].into_iter();
}debug: Doesn't work: pub fn main() {
for _ in [1, 2, 3].into_iter() {}
}debug: There is no |
|
Had a little bit of time for debugging the use std::fs;
use std::path::Path;
fn main() -> Result<(), std::io::Error> {
let path = Path::new(".");
for _ in fs::read_dir(path)? {
}
Ok(())
}
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std;
use std::fs;
use std::path::Path;
fn main() -> Result<(), std::io::Error> {
let path = <Path>::new(".");
{
let _result = match ::std::iter::IntoIterator::into_iter(
match ::std::ops::Try::into_result(fs::read_dir(path)) {
::std::result::Result::Err(err) =>
#[allow(unreachable_code)]
{
#[allow(unreachable_code)]
return ::std::ops::Try::from_error(::std::convert::From::from(err))
}
::std::result::Result::Ok(val) =>
#[allow(unreachable_code)]
{
#[allow(unreachable_code)]
val
}
},
) {
mut iter => loop {
let mut __next;
match ::std::iter::Iterator::next(&mut iter) {
::std::option::Option::Some(val) => __next = val,
::std::option::Option::None => break,
}
let _ = __next;
{}
},
};
_result
}
Ok(())
}The problem is the line |
I have just tested the snippet with all the nightly toolchains from The command I used for testing every version is this, lemme know if it's the right one:
|
|
Also, fwiw: with latest nightly ( error[E0308]: mismatched types
--> clippy_lints/src/enum_glob_use.rs:44:57
|
44 | self.lint_item(cx, cx.tcx.hir().expect_item(item.id));
| ^^^^^^^ expected struct `syntax::ast::NodeId`, found struct `rustc::hir::HirId`
|
= note: expected type `syntax::ast::NodeId`
found type `rustc::hir::HirId`
error[E0308]: mismatched types
--> clippy_lints/src/lifetimes.rs:359:92
|
359 | if let ItemKind::Existential(ref exist_ty) = self.cx.tcx.hir().expect_item(item.id).node {
| ^^^^^^^ expected struct `syntax::ast::NodeId`, found struct `rustc::hir::HirId`
|
= note: expected type `syntax::ast::NodeId`
found type `rustc::hir::HirId`
error: aborting due to 2 previous errors
For more information about this error, try `rustc --explain E0308`.
error: Could not compile `clippy_lints`.
To learn more, run the command again with --verbose. |
|
Beta will be released on April 11 but backlog here dangerously grows. |
|
Yeah :c I wonder if there's a way to add a CI command to Like, sure, 1/4 of I feel like this error could've been detected in such a way. Well, if |
|
There is such test but it can detect only first breakage (it doesn't check exact errors but failure in build or tests). |
This already happens, we get notified. The current status is due to libtest being broken in a way we can't move forward with. We are blocked on @gnzlbg and @alexcrichton resolving the issues. |
|
@mati865, I tested your snippet: pub fn main() {
for _ in [1, 2, 3].into_iter() {}
}Using the command I tested it as far back as Here's the full output for And here it is for Did I use the wrong command? Or maybe this means that the regression is somewhere else? 🤔 |
|
Added some changes at #3908 so that Clippy builds again. The weird linting error is still there, however u.u |
|
Things left to do to unbreak Clippy:
|
|
My thoughts about the
My thoughts on
My thoughts on the
EDIT: I won't have time to look into this more today, if someone wants to pick this up. |
|
More info on the current state: I've just tested adding rust-clippy/tests/compile-test.rs Line 77 in df29b99 After that change, all remaining tests pass. So, it seems we're not so far from unbreaking Clippy.
That's awesome. I'll try to look more into it later today, I'm not sure if I can fix it but I'll try. Edit: what module is that you were talking about? I want to give it a look :) |
Note that there are 2 updated *.stderr files, which shouldn't be updated, because nothing should have changed there.
do you mean
rust-clippy/clippy_lints/src/methods/mod.rs Lines 828 to 830 in 61aa5c9 If you put the debugging println! expression @mati865 posted here #3905 (comment), in front of the if statement, it will print out the into_iter MethodCall. Behind the if statement the MethodCall won't be printed.My guess is, that something changed how expn_info is generated in rustc. But that's just a really wild guess.
|
Ah! I thought it was a module on |
|
I think the dogfood tests are run with the |
|
Yeah, it works alright with |
|
Okay I tested @mati865's
The tested files had just the two snippets written by @mati865: pub fn main() {
for _ in [1, 2, 3].into_iter() {}
}pub fn main() {
let _ = [1, 2, 3].into_iter();
}Their stdout was filtered from Clippy's test output and the result was this:
These are the results for each file: First file's: Second file's: |
|
I would suggest to merge this this evening (with the Then we should open high priority issues to fix those. But 3 known bugs in Clippy is better than no Clippy in the nightlies IMHO. |
|
I agree, and we've merged rustups with test failures in the past. |
|
Though, to be clear, for rustc to accept this clippy will have to appear to pass tests, i.e. we'll have to temporarily delete the test. |
I would just use the wrongly updated *.stderr files from this PR instead of deleting them completely. Then it also appears as if the tests pass. |
|
Oh, sorry, that's basically what I meant. "Removing the test". |
|
Or use 2nd option from rust-lang/rust#59396 (comment) |
|
@bors try Appveyor will fail, but let's see if this introduced ICEs in some crates. |
Hacky rustup This gets our tests back working, but we now are calling `transmute` in the compiletest code. Also I couldn't quickly figure out some test failures and won't have time to get to them until thursday, if someone could take them over that would be great (maybe just merge this PR and open issues for all failures and start addressing them?)
|
💔 Test failed - status-appveyor |
|
Travis is green, no ICEs introduced by this. Let's merge this. |
This gets our tests back working, but we now are calling
transmutein the compiletest code.Also I couldn't quickly figure out some test failures and won't have time to get to them until thursday, if someone could take them over that would be great (maybe just merge this PR and open issues for all failures and start addressing them?)