Only report Self: Sized violation once. #26027

nham · 2015-06-05T04:34:28Z

Background

The cause of #20692 seems to be in middle::trait::object_safety::object_safety_violations. The supertrait_def_ids call returns an iterator that iterates over trait definition ids. The iteration seems to start at the current trait and then traverse supertraits in order. For example, in the following block of code, it goes Bar -> Foo -> Sized:

trait Foo: Sized { }
trait Bar: Foo { }

fn foo<T: Bar>(x: &T) {
    let _ = x as &Bar; //~ ERROR cannot convert to a trait object because trait `Bar` is not object-safe
}

fn main () {}

However for each trait in the hierarchy, object_safety_violations_in_trait gets called (in the .flat_map() call). Each call (all 3 of them) results in a Self : Sized violation since each trait in the hierarchy has Sized as a supertrait. That is why the above code compiles with 2 extra notes today:

: rustc 20692.rs
20692.rs:5:13: 5:14 error: cannot convert to a trait object because trait `Bar` is not object-safe [E0038]
20692.rs:5     let _ = x as &Bar; //~ ERROR cannot convert to a trait object because trait `Bar` is not object-safe
                       ^
20692.rs:5:13: 5:14 note: the trait cannot require that `Self : Sized`
20692.rs:5     let _ = x as &Bar; //~ ERROR cannot convert to a trait object because trait `Bar` is not object-safe
                       ^
20692.rs:5:13: 5:14 note: the trait cannot require that `Self : Sized`
20692.rs:5     let _ = x as &Bar; //~ ERROR cannot convert to a trait object because trait `Bar` is not object-safe
                       ^
20692.rs:5:13: 5:14 note: the trait cannot require that `Self : Sized`
20692.rs:5     let _ = x as &Bar; //~ ERROR cannot convert to a trait object because trait `Bar` is not object-safe
                       ^

The fix

The fix I've submitted is to move the trait_has_sized_self check out of object_safety_violations_in_trait to object_safety_violations so it's only called once, and not for each supertrait.

How to test?

I need help here. I don't see how to test it. This doesn't work:

trait Foo: Sized { }
trait Bar: Foo { }

fn foo<T: Bar>(x: &T) {
    let _ = x as &Bar; //~ ERROR cannot convert to a trait object because trait `Foo` is not object-safe
                       //~^ NOTE the trait cannot require that `Self : Sized`
}

fn main() {}

Putting this in compile-fail passes without the fix. It seems that this doesn't try to match the number of notes exactly, so even though compilation results in 3 notes emitted today, it matches the one that is expected in the test and ignores the extra error output.

Any advice on how to test this fix?

Fixes #20692.

rust-highfive · 2015-06-05T04:34:40Z

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2015-06-05T16:44:38Z

src/librustc/middle/traits/object_safety.rs

@@ -104,9 +110,6 @@ fn object_safety_violations_for_trait<'tcx>(tcx: &ty::ctxt<'tcx>,
        .collect();

    // Check the trait itself.
-    if trait_has_sized_self(tcx, trait_def_id) {


Could this perhaps be left in the same place but check that the violations array doesn't already contain SizedSelf?

No, I also thought about that, but it doesn't seem to work. This is the violations vector inside object_safety_violations_for_trait. This one will only ever have one SizedSelf in it.

alexcrichton · 2015-06-05T16:47:32Z

Any advice on how to test this fix?

Hm I think that compiletest does indeed check that all error messages are accounted for, but it must be using the same pattern to match the error message more than once. Some possibilities for adding a test could be:

Fix compiletest to use a // ERROR directive at most once (this is probably the best solution).
Add a run-make test which uses grep, for example. This may be overkill.

nham · 2015-06-05T20:32:19Z

@alexcrichton Thanks for the suggestion. The problem seems to be that it only ensures that the initial compiler errors match all existing error patterns in the test file. After the error patterns are all matched, any remaining compiler errors get ignored. I believe I have a fix for this bug, but fixing it revealed over 100 broken tests! I am now going through the tests and trying to fix them.

Edit: or rather, not broken, but tests that relied on not having to match every single error

alexcrichton · 2015-06-05T22:55:09Z

Oh thanks for the cleanup @nham! Looks like there's a tidy error here but once that's fixed r=me

nham · 2015-06-06T18:50:59Z

@alexcrichton Sorry for being unclear. The tests are not fixed yet, there are still 17 compile-fail test failures.

Also, when is it acceptable to ignore a test? I've tried to fix each of the remaining 17 but have not yet been able to figure out how to do so

For reference, here's what's left in compile-fail:

failures:
    [compile-fail] compile-fail/dead-code-ret.rs
    [compile-fail] compile-fail/huge-enum.rs
    [compile-fail] compile-fail/internal-unstable-noallow.rs
    [compile-fail] compile-fail/issue-10536.rs
    [compile-fail] compile-fail/issue-10656.rs
    [compile-fail] compile-fail/issue-14091-2.rs
    [compile-fail] compile-fail/issue-14091.rs
    [compile-fail] compile-fail/issue-16966.rs
    [compile-fail] compile-fail/issue-21146.rs
    [compile-fail] compile-fail/issue-8727.rs
    [compile-fail] compile-fail/lint-non-snake-case-crate-2.rs
    [compile-fail] compile-fail/lint-stability2.rs
    [compile-fail] compile-fail/lint-stability3.rs
    [compile-fail] compile-fail/mod_file_not_owning.rs
    [compile-fail] compile-fail/multiple-plugin-registrars.rs
    [compile-fail] compile-fail/no-capture-arc.rs
    [compile-fail] compile-fail/nolink-with-link-args.rs

test result: FAILED. 1740 passed; 17 failed; 15 ignored; 0 measured

alexcrichton · 2015-06-07T17:25:54Z

@nham oh wow this has a lot more fallout than I would have initially anticipated, thanks for doing this! What's the error message associated with these failing tests? I would have expected some extra //~ ERROR annotations would be all that's needed, but I may be misunderstanding the fix needed for compiletest.

bors · 2015-06-08T03:26:18Z

☔ The latest upstream changes (presumably #26087) made this pull request unmergeable. Please resolve the merge conflicts.

nham · 2015-06-08T17:24:09Z

@alexcrichton Many of the test failures were because of missing error messages, but you're right that some of these test failures are unexpected. A common example of such a failure is a test looking like this:

// error-pattern: mismatched types

fn f(x: isize) { }

fn main() { let i: (); i = f(()); }

This test began failing after the change, but it works if I remove the "error-pattern" comment and add a //~ ERROR mismatched types comment to the line containing i = f(());. It was specifically the "mismatched types" error, plus a couple others, where this happened multiple times, and I'm still not sure why.

alexcrichton · 2015-06-08T18:13:03Z

Yeah I'm not sure why those would suddenly start failing, but at least it's good cleanup to go from error-pattern to //~ ERROR!

nham · 2015-06-09T00:09:59Z

@alexcrichton This passes make check-stage1 now on my system. I've only had to ignore 4 tests so far. Three of these are using compiletest commands that I'm unfamiliar with. For example:

// check-stdout
// error-pattern:thread 'test_foo' panicked at
// compile-flags: --test
// ignore-pretty: does not work well with `--test`

Do I need to try to fix these or can I ignore them for now?

The other test that I ignored is compile-fail/issue-21146.rs. I'm totally stumped on how to fix it. Nothing seems to work.

alexcrichton · 2015-06-09T16:46:18Z

It looks like check-stdout means that the output is checked against both the stdout and stderr of the process, although I'm not quite sure why that would be specified? Perhaps the check-stdout flag could be dropped?

What's the error message you're getting from issue-21146?

nham · 2015-06-09T19:43:55Z

Here's a more complete example. It makes sense to me why check-stdout is used:

// run-fail/test-panic.rs

// check-stdout
// error-pattern:thread 'test_foo' panicked at
// compile-flags: --test
// ignore-pretty: does not work well with `--test`

#[test]
fn test_foo() {
    panic!()
}

It's verifying that this test panics. Currently the test fails with "no more error patterns to match against", but to be honest I don't see any other error messages in the "test_foo stdout" section (I tried adding // error-pattern: failures but that isn't part of it.

failures:

---- [run-fail] run-fail/test-panic.rs stdout ----

error: no more error patterns to match against
status: exit code: 101
command: x86_64-unknown-linux-gnu/test/run-fail/test-panic.stage1-x86_64-unknown-linux-gnu 
stdout:
------------------------------------------

running 1 test
test test_foo ... FAILED

failures:

---- test_foo stdout ----
    thread 'test_foo' panicked at 'explicit panic', /home/nham/code/other/rust2/src/test/run-fail/test-panic.rs:17



failures:
    test_foo

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured


------------------------------------------
stderr:
------------------------------------------
thread '<main>' panicked at 'Some tests failed', /home/nham/code/other/rust2/src/libtest/lib.rs:255

------------------------------------------

thread '[run-fail] run-fail/test-panic.rs' panicked at 'explicit panic', /home/nham/code/other/rust2/src/compiletest/runtest.rs:1521

Removing the check-stdout doesn't seem to work either!

nham · 2015-06-09T19:44:08Z

issue-21146 looks like this:

// error-pattern: expected item, found `parse_error`
include!("../auxiliary/issue-21146-inc.rs");
fn main() {}

and ../auxiliary/issue-21146-inc.rs looks like this:

parse_error

Trying to compile issue-21146 results in exactly one error:

src/test/compile-fail/../auxiliary/issue-21146-inc.rs:13:1: 13:12 error: expected item, found `parse_error`
src/test/compile-fail/../auxiliary/issue-21146-inc.rs:13 parse_error
                                                         ^~~~~~~~~~~

Which seems like it should match the error-pattern in the file. But for some reason it doesn't:

failures:

---- [compile-fail] compile-fail/issue-21146.rs stdout ----

error: no more error patterns to match against
status: exit code: 101
command: x86_64-unknown-linux-gnu/stage1/bin/rustc /home/nham/code/other/rust2/src/test/compile-fail/issue-21146.rs -L x86_64-unknown-linux-gnu/test/compile-fail/ --target=x86_64-unknown-linux-gnu -L x86_64-unknown-linux-gnu/test/compile-fail/issue-21146.stage1-x86_64-unknown-linux-gnu.compile-fail.libaux -C prefer-dynamic -o x86_64-unknown-linux-gnu/test/compile-fail/issue-21146.stage1-x86_64-unknown-linux-gnu --cfg rtopt -O -L x86_64-unknown-linux-gnu/rt
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
/home/nham/code/other/rust2/src/test/compile-fail/../auxiliary/issue-21146-inc.rs:13:1: 13:12 error: expected item, found `parse_error`
/home/nham/code/other/rust2/src/test/compile-fail/../auxiliary/issue-21146-inc.rs:13 parse_error
                                                                                     ^~~~~~~~~~~

------------------------------------------

thread '[compile-fail] compile-fail/issue-21146.rs' panicked at 'explicit panic', /home/nham/code/other/rust2/src/compiletest/runtest.rs:1521

no more error patterns to match against indicates there are additional compilation errors that do not have patterns, but I don't see any other errors...?

alexcrichton · 2015-06-10T15:30:27Z

Perhaps lines of the output are being considered a compilation error that need to be matched against by accident? It may be useful to instrument compiletest to print out what addition errors are expected to be matched but weren't.

nham · 2015-06-10T21:55:21Z

Thanks. I did this and I managed to make all the tests pass without ignoring any.

alexcrichton · 2015-06-10T23:23:02Z

@bors: r+ c41b947c7d0889c190da021e3884719725744161 p=1

Heroic work @nham!

bors · 2015-06-11T00:12:39Z

⌛ Testing commit c41b947 with merge ea41672...

nham · 2015-06-11T00:16:09Z

I ran make check (instead of make check-stage1 as above) on my system and found a failing test. I didn't realize there was an ignore-stage1 directive. Fix incoming!

nham · 2015-06-11T00:38:23Z

r? @alexcrichton . This passes make check now on my system. (sorry, should've caught this earlier)

alexcrichton · 2015-06-11T00:56:11Z

@bors: r+ a55d310

No worries!

bors · 2015-06-11T02:42:52Z

⌛ Testing commit a55d310 with merge 4d3bded...

bors · 2015-06-11T03:25:43Z

💔 Test failed - auto-mac-64-opt

bors · 2015-06-11T21:38:25Z

☔ The latest upstream changes (presumably #26190) made this pull request unmergeable. Please resolve the merge conflicts.

Fixes rust-lang#20692.

Also fixes tests to pass.

Fallout from the change to compiletest that makes it not ignore compiler errors that don't have an error pattern.

nham · 2015-06-12T01:06:58Z

Rebased. This passes make check on my system, so not sure what that test failure is about.

alexcrichton · 2015-06-12T16:34:08Z

@bors: r+ 5c0fb26

bors · 2015-06-12T17:42:40Z

⌛ Testing commit 5c0fb26 with merge 4cca4f9...

bors · 2015-06-12T18:25:54Z

💔 Test failed - auto-mac-64-opt

alexcrichton · 2015-07-05T17:12:11Z

Closing due to inactivity, but feel free to reopen with tests fixed!

nham · 2015-07-05T20:40:38Z

Unfortunately I'm not sure how to investigate a mac-only test failure. That test passes on my system :\

rust-highfive assigned Aatch Jun 5, 2015

alexcrichton reviewed Jun 5, 2015
View reviewed changes

nham force-pushed the fix_E0038_dup branch from e0836a9 to c6157f0 Compare June 6, 2015 18:45

nham force-pushed the fix_E0038_dup branch 2 times, most recently from 68feaac to 9ea922d Compare June 6, 2015 19:31

nham force-pushed the fix_E0038_dup branch 4 times, most recently from deffb5b to c824e0d Compare June 9, 2015 00:06

nham force-pushed the fix_E0038_dup branch from c824e0d to c41b947 Compare June 10, 2015 21:26

rust-highfive assigned alexcrichton and unassigned Aatch Jun 11, 2015

Nick Hamann added 5 commits June 11, 2015 18:25

Only report Self: Sized violation once.

c7d26a9

Fixes rust-lang#20692.

Fix compiletest to not ignore additional errors.

1deefe5

Also fixes tests to pass.

Fix many tests.

e517478

Fallout from the change to compiletest that makes it not ignore compiler errors that don't have an error pattern.

Add test for fix of rust-lang#20692

40088fb

Fix two more tests.

5c0fb26

nham force-pushed the fix_E0038_dup branch from a55d310 to 5c0fb26 Compare June 12, 2015 01:03

alexcrichton closed this Jul 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only report Self: Sized violation once. #26027

Only report Self: Sized violation once. #26027

nham commented Jun 5, 2015

rust-highfive commented Jun 5, 2015

alexcrichton Jun 5, 2015

nham Jun 5, 2015

alexcrichton commented Jun 5, 2015

nham commented Jun 5, 2015

alexcrichton commented Jun 5, 2015

nham commented Jun 6, 2015

alexcrichton commented Jun 7, 2015

bors commented Jun 8, 2015

nham commented Jun 8, 2015

alexcrichton commented Jun 8, 2015

nham commented Jun 9, 2015

alexcrichton commented Jun 9, 2015

nham commented Jun 9, 2015

nham commented Jun 9, 2015

alexcrichton commented Jun 10, 2015

nham commented Jun 10, 2015

alexcrichton commented Jun 10, 2015

bors commented Jun 11, 2015

nham commented Jun 11, 2015

nham commented Jun 11, 2015

alexcrichton commented Jun 11, 2015

bors commented Jun 11, 2015

bors commented Jun 11, 2015

bors commented Jun 11, 2015

nham commented Jun 12, 2015

alexcrichton commented Jun 12, 2015

bors commented Jun 12, 2015

bors commented Jun 12, 2015

alexcrichton commented Jul 5, 2015

nham commented Jul 5, 2015

Only report Self: Sized violation once. #26027

Only report Self: Sized violation once. #26027

Conversation

nham commented Jun 5, 2015

Background

The fix

How to test?

rust-highfive commented Jun 5, 2015

alexcrichton Jun 5, 2015

Choose a reason for hiding this comment

nham Jun 5, 2015

Choose a reason for hiding this comment

alexcrichton commented Jun 5, 2015

nham commented Jun 5, 2015

alexcrichton commented Jun 5, 2015

nham commented Jun 6, 2015

alexcrichton commented Jun 7, 2015

bors commented Jun 8, 2015

nham commented Jun 8, 2015

alexcrichton commented Jun 8, 2015

nham commented Jun 9, 2015

alexcrichton commented Jun 9, 2015

nham commented Jun 9, 2015

nham commented Jun 9, 2015

alexcrichton commented Jun 10, 2015

nham commented Jun 10, 2015

alexcrichton commented Jun 10, 2015

bors commented Jun 11, 2015

nham commented Jun 11, 2015

nham commented Jun 11, 2015

alexcrichton commented Jun 11, 2015

bors commented Jun 11, 2015

bors commented Jun 11, 2015

bors commented Jun 11, 2015

nham commented Jun 12, 2015

alexcrichton commented Jun 12, 2015

bors commented Jun 12, 2015

bors commented Jun 12, 2015

alexcrichton commented Jul 5, 2015

nham commented Jul 5, 2015