Expect all help/note messages are specified in a cfail test #30778

fhahn · 2016-01-08T11:31:53Z

This is a PR for #21195. It changes the way unspecified help and ǹote messages are handled in compile-fail tests as suggested by @oli-obk in the issue: if there are some note or help annotations, there must be annotations for all help or note messages of this test. Maybe it makes also sense to add an option to specify that the this test should fail if there are unspecified help or note messages.

With this change, the following tests fail:

[compile-fail] compile-fail/changing-crates.rs
[compile-fail] compile-fail/default_ty_param_conflict_cross_crate.rs
[compile-fail] compile-fail/lifetime-inference-give-expl-lifetime-param.rs
[compile-fail] compile-fail/privacy1.rs
[compile-fail] compile-fail/svh-change-lit.rs
[compile-fail] compile-fail/svh-change-significant-cfg.rs
[compile-fail] compile-fail/svh-change-trait-bound.rs
[compile-fail] compile-fail/svh-change-type-arg.rs
[compile-fail] compile-fail/svh-change-type-ret.rs
[compile-fail] compile-fail/svh-change-type-static.rs
[compile-fail] compile-fail/svh-use-trait.rs

I'll add the missing annotations if we decide to accept this change.

rust-highfive · 2016-01-08T11:32:04Z

r? @nikomatsakis

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

nikomatsakis · 2016-01-08T19:50:26Z

Nominated for discussion at the next @rust-lang/compiler meeting. But feel free to throw in your 2 cents now. :)

nikomatsakis · 2016-01-08T19:51:15Z

Actually, let's converse at #21195, not on the PR itself.

pnkfelix · 2016-01-14T21:24:20Z

@fhahn okay, compiler team thumbs-up'ed the idea of this PR at #21195, so I'd say go ahead and put in the updates to the tests.

fhahn · 2016-01-15T21:22:11Z

Great to hear. I've updated the tests.

pnkfelix · 2016-01-15T23:32:22Z

src/compiletest/runtest.rs

+        expected_errors.iter()
+                        .fold((false, false),
+                              |(acc_help, acc_note), ee|
+                                  (acc_help || ee.kind == "help:", acc_note ||


so, the logic here is that "if there is a help, then all help must be present", plus "if there is a note, then all notes must be present."

In hindsight I had thought that what we were discussing was "if there is either a help or a note, then all help and notes must be present."

I suspect that the logic you have chosen is actually more convenient in practice than what I was expecting, but I still figured I should point it out explicitly, in case someone else was as confused as I was about the proposal.

Yep, thanks for making that clear. I probably should have been more specific in the description. By the way, is there a place to document this?

@pnkfelix:

In hindsight I had thought that what we were discussing was "if there is either a help or a note, then all help and notes must be present."

I actually wondered later which version we meant, and figured I'd probably be ok with either one...

@fhahn

By the way, is there a place to document this?

We've gone back and forth on this. I know that @brson has been working on making some kind of "wiki-like" page for compiler internal docs and the like, I'm not sure what's the current status on that? I'm not sure what happened to the existing docs on the compiletest infrastruture, but it seems like it should go in there.

Ok. Should we delay merging this PR until there's a place for the documentation?

nikomatsakis · 2016-01-28T17:22:59Z

@fhahn

Ok. Should we delay merging this PR until there's a place for the documentation?

Nah. Let's land it.

nikomatsakis · 2016-01-28T17:23:08Z

It's not like the rest of this system is documented :)

nikomatsakis · 2016-01-28T17:23:14Z

(Well, it used to be?)

nikomatsakis · 2016-01-28T17:23:18Z

@bors r+

bors · 2016-01-28T17:23:19Z

📌 Commit 08c590f has been approved by nikomatsakis

bors · 2016-01-28T20:27:37Z

⌛ Testing commit 08c590f with merge 2fe164d...

bors · 2016-01-28T22:13:24Z

💔 Test failed - auto-win-msvc-64-opt

pnkfelix · 2016-01-28T23:14:51Z

This looks like it could be a legitimate failure?

In particular, while I do see that this PR touched changing-crates.rs, it seems like it effectively added only the two lines:

  //~| NOTE: crate `a` path #1:
  //~| NOTE: crate `b` path #1:

but in the output linked via homu, I see a few more notes being emitted, including one that has a path #2 for a (as well as a distinct path for a that is also labelled #1 ???)

@fhahn is there a way for a test to opt out of the stronger checking being performed here, e.g. via some test option embedded in a comment near the top of the file?

…tains help/note annotations, closes rust-lang#21195

fhahn · 2016-01-29T23:59:20Z

Yeah it seems like it could be a legitimate failure.

It seems like for the Windows build, two additional notes are emitted, e.g. for changing-crates.rs

C:/bot/slave/auto-win-msvc-64-opt/build/src/test/compile-fail/changing-crates.rs:17:1: 17:16 note: crate `a` path #1: \\?\C:\bot\slave\auto-win-msvc-64-opt\build\obj\x86_64-pc-windows-msvc\test\compile-fail\changing-crates.stage2-x86_64-pc-windows-msvc.compile-fail.libaux\a.dll
C:/bot/slave/auto-win-msvc-64-opt/build/src/test/compile-fail/changing-crates.rs:17:1: 17:16 note: crate `b` path #1: \\?\C:\bot\slave\auto-win-msvc-64-opt\build\obj\x86_64-pc-windows-msvc\test\compile-fail\changing-crates.stage2-x86_64-pc-windows-msvc.compile-fail.libaux\b.dll
C:/bot/slave/auto-win-msvc-64-opt/build/src/test/compile-fail/changing-crates.rs:17:1: 17:16 help: please recompile this crate using --crate-type lib
C:/bot/slave/auto-win-msvc-64-opt/build/src/test/compile-fail/changing-crates.rs:17:1: 17:16 note: crate `a` path #1: x86_64-pc-windows-msvc/test/compile-fail\changing-crates.stage2-x86_64-pc-windows-msvc.compile-fail.libaux\a.dll.lib
C:/bot/slave/auto-win-msvc-64-opt/build/src/test/compile-fail/changing-crates.rs:17:1: 17:16 note: crate `a` path #2: C:\bot\slave\auto-win-msvc-64-opt\build\obj\x86_64-pc-windows-msvc\stage2\lib\rustlib\x86_64-pc-windows-msvc\lib\arena-db5a760f.dll.lib

@pnkfelix as you already mentioned, the last two notes seem fishy and probably shouldn't be there. The first of those seem to point to the one already suggested, except that the first part is missing and slashes instead of backslashes are used. The last note suggests arena-db5a760f.dll.lib, but that does not seem right at all.

At the moment there's no option to opt out of the new behavior, but I could add one, if we want to merge this PR before resolving the Windows issue.

alexcrichton · 2016-01-30T00:34:41Z

It'd probably be fine to tag these with // ignore-msvc with an issue opened about how this should be fixed at some point. It's not really a regression on part of this PR, so seems a shame to prevent it from landing!

fhahn · 2016-01-30T13:28:54Z

@alexcrichton thanks for pointing me to ignore-msvc, hope that does the trick! I've marked the failing tests as ignore-msvc.

alexcrichton · 2016-01-30T18:34:14Z

@bors: r=nikomatsakis 526965a

bors · 2016-01-30T18:51:14Z

⌛ Testing commit 526965a with merge 14f33a5...

@oli-obk

This is a PR for #21195. It changes the way unspecified `help` and `ǹote` messages are handled in compile-fail tests as suggested by @oli-obk in the issue: if there are some `note` or `help` annotations, there must be annotations for all `help` or `note` messages of this test. Maybe it makes also sense to add an option to specify that the this test should fail if there are unspecified `help` or `note` messages. With this change, the following tests fail: [compile-fail] compile-fail/changing-crates.rs [compile-fail] compile-fail/default_ty_param_conflict_cross_crate.rs [compile-fail] compile-fail/lifetime-inference-give-expl-lifetime-param.rs [compile-fail] compile-fail/privacy1.rs [compile-fail] compile-fail/svh-change-lit.rs [compile-fail] compile-fail/svh-change-significant-cfg.rs [compile-fail] compile-fail/svh-change-trait-bound.rs [compile-fail] compile-fail/svh-change-type-arg.rs [compile-fail] compile-fail/svh-change-type-ret.rs [compile-fail] compile-fail/svh-change-type-static.rs [compile-fail] compile-fail/svh-use-trait.rs I'll add the missing annotations if we decide to accept this change.

bors · 2016-01-30T20:48:17Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

Original issue: rust-lang#21195 Relevant PR: rust-lang#30778 Prior to this commit, if a compiletest testcase included the text "HELP:" or "NOTE:" (note the colons), then it would indicate to the compiletest suite that we should verify "help" and "note" expected messages. This commit updates this check to also check "HELP" and "NOTE" (not the absense of colons) so that we always verify "help" and "note" expected messages.

…atsakis Stop ignoring expected note/help messages in compiletest suite. Original issue: #21195 Relevant PR: #30778 Prior to this commit, if a compiletest testcase included the text "HELP:" or "NOTE:" (note the colons), then it would indicate to the compiletest suite that we should verify "help" and "note" expected messages. This commit updates this check to also check "HELP" and "NOTE" (not the absense of colons) so that we always verify "help" and "note" expected messages.

…nkov Enable svh tests on msvc These tests were ignored for msvc in rust-lang#30778 because of additional notes that were being added for some reason. I'm fairly confident this has been fixed in the intervening years so lets try re-enabling these tests. Fixes rust-lang#31306

rust-highfive assigned nikomatsakis Jan 8, 2016

nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Jan 8, 2016

nikomatsakis removed the I-nominated label Jan 8, 2016

pnkfelix mentioned this pull request Jan 9, 2016

"help" messages in compiler output is ignored #21195

Closed

fhahn force-pushed the issue-21195-expect-help branch from d866942 to 08c590f Compare January 15, 2016 21:10

pnkfelix reviewed Jan 15, 2016
View reviewed changes

fhahn added 2 commits January 30, 2016 00:27

Expect all help/note messages are specified in a cfail test if it con…

36656f8

…tains help/note annotations, closes rust-lang#21195

Add missings NOTE and HELP annotations to tests

efe56c8

fhahn mentioned this pull request Jan 30, 2016

Invalid paths are suggested for "possibly newer version of crate" error on Windows #31306

Closed

Mark test failing with msvc due to rust-lang#31306 as ignore-msvc

526965a

fhahn force-pushed the issue-21195-expect-help branch from 08c590f to 526965a Compare January 30, 2016 13:27

bors merged commit 526965a into rust-lang:master Jan 30, 2016

fhahn deleted the issue-21195-expect-help branch January 30, 2016 21:38

frewsxcv mentioned this pull request Mar 15, 2016

Stop ignoring expected note/help messages in compiletest suite. #32263

Merged

ChrisDenton mentioned this pull request Dec 1, 2021

Enable svh tests on msvc #91411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expect all help/note messages are specified in a cfail test #30778

Expect all help/note messages are specified in a cfail test #30778

fhahn commented Jan 8, 2016

rust-highfive commented Jan 8, 2016

nikomatsakis commented Jan 8, 2016

nikomatsakis commented Jan 8, 2016

pnkfelix commented Jan 14, 2016

fhahn commented Jan 15, 2016

pnkfelix Jan 15, 2016

fhahn Jan 15, 2016

nikomatsakis Jan 19, 2016

fhahn Jan 26, 2016

nikomatsakis commented Jan 28, 2016

nikomatsakis commented Jan 28, 2016

nikomatsakis commented Jan 28, 2016

nikomatsakis commented Jan 28, 2016

bors commented Jan 28, 2016

bors commented Jan 28, 2016

bors commented Jan 28, 2016

pnkfelix commented Jan 28, 2016

fhahn commented Jan 29, 2016

alexcrichton commented Jan 30, 2016

fhahn commented Jan 30, 2016

alexcrichton commented Jan 30, 2016

bors commented Jan 30, 2016

bors commented Jan 30, 2016

Expect all help/note messages are specified in a cfail test #30778

Expect all help/note messages are specified in a cfail test #30778

Conversation

fhahn commented Jan 8, 2016

rust-highfive commented Jan 8, 2016

nikomatsakis commented Jan 8, 2016

nikomatsakis commented Jan 8, 2016

pnkfelix commented Jan 14, 2016

fhahn commented Jan 15, 2016

pnkfelix Jan 15, 2016

Choose a reason for hiding this comment

fhahn Jan 15, 2016

Choose a reason for hiding this comment

nikomatsakis Jan 19, 2016

Choose a reason for hiding this comment

fhahn Jan 26, 2016

Choose a reason for hiding this comment

nikomatsakis commented Jan 28, 2016

nikomatsakis commented Jan 28, 2016

nikomatsakis commented Jan 28, 2016

nikomatsakis commented Jan 28, 2016

bors commented Jan 28, 2016

bors commented Jan 28, 2016

bors commented Jan 28, 2016

pnkfelix commented Jan 28, 2016

fhahn commented Jan 29, 2016

alexcrichton commented Jan 30, 2016

fhahn commented Jan 30, 2016

alexcrichton commented Jan 30, 2016

bors commented Jan 30, 2016

bors commented Jan 30, 2016