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

frewsxcv · 2016-03-15T14:15:44Z

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.

rust-highfive · 2016-03-15T14:15:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

frewsxcv · 2016-03-15T14:16:01Z

/cc @fhahn

frewsxcv · 2016-03-15T14:16:38Z

src/compiletest/runtest.rs

-                                  (acc_help || ee.kind == "help:", acc_note ||
-                                   ee.kind == "note:"));
+                                  (acc_help || ee.kind == "help:" || ee.kind == "help",
+                                   acc_note || ee.kind == "note:" || ee.kind == "note"));


I have a separate branch that cleans up this "kind" logic by using enums. I plan to open it up as a follow-up PR if this merges.

This should make the opt-in work for note and help as well, but it is still opt-in and notes and help messages are only checked in files with help and note annotations.

When making this changes, it seems like I overlooked note and help (and assumed there is more normalization going on). I think using enums there would make it clearer 👍

frewsxcv · 2016-03-15T14:18:52Z

src/test/compile-fail/issue-25386.rs

@@ -8,6 +8,10 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

+// error-pattern: in this expansion of format_args!
+// error-pattern: in this expansion of print! (defined in <std macros>)
+// field `c_object` of struct `stuff::Item` is private


I'm pretty sure I did this incorrectly. A couple errors in this testcase have errors that report on <std macros> and I'm not sure the best way to handle it.

there really isn't a good way, actually. In this case though I'd just remove the NOTEs. the issue in question (#25386) was an ICE, not a test of diagnostics (I'd not include NOTEs unless the purpose is to test that they are emitted, personally)

Cool, that's what I figured, I'll remove them now.

Removed in the latest force push.

frewsxcv · 2016-03-15T14:33:20Z

A little background on this PR: I did a bit of refactoring on compiletest and realized that some help/note messages were being ignored. I noticed this conditional and assumed it was a mistake to not also check "note" and "help", so I fixed the conditional and the relevant test cases (this PR). It wasn't until after I was done that I noticed this issue where it makes it sound like it's opt-in, presumably because it's too much work to add the extra note/help messages? In my opinion, there's no harm in adding them and it could catch diagnostic errors in the future.

frewsxcv · 2016-03-15T15:03:45Z

On second thought, I think it is still opt-in, but my changes make the opt-in work for both "NOTE" and "HELP" in addition to "NOTE:" and "HELP:"

frewsxcv · 2016-03-16T18:44:18Z

r? @nikomatsakis

nikomatsakis · 2016-03-16T19:18:52Z

src/compiletest/runtest.rs

@@ -1013,8 +1013,8 @@ fn check_expected_errors(revision: Option<&str>,
        expected_errors.iter()
                        .fold((false, false),


Pre-existing, but can't we use any here instead of fold? Seems like it more clearly expresses the intent.

Given this code block:

fn main() { let i = 1; //~ NOTE: some note let j = 2; //~ HELP: some help }

The current implementation checks for note and help in one pass through. I'm not sure how it'd be possible using any unless we were expecting both help and note on the same line. I agree it's not clean, and I'm willing to clean it up, but would rather do that in a different PR.

Do it in two passes?

On Wed, Mar 16, 2016 at 12:25:23PM -0700, Corey Farwell wrote:

@@ -1013,8 +1013,8 @@ fn check_expected_errors(revision: Option<&str>,
expected_errors.iter()
.fold((false, false),

Given this code block:

fn main() { let i = 1; //~ NOTE: some note let j = 2; //~ HELP: some help }

The current implementation checks for note and help in one pass through. I'm not sure how it'd be possible using any unless we were expecting both help and note on the same line. I agree it's not clean, and I'm willing to clean it up, but would rather do that in a different PR.

You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub:
https://github.com/rust-lang/rust/pull/32263/files/fb96cc8b5d83da8c32cdaf51a5f53edb74152024#r56398787

Spoke with you on IRC. Going to do it in a follow-up PR.

nikomatsakis · 2016-03-16T20:05:37Z

left some nits, but seems good

nikomatsakis · 2016-03-16T20:05:46Z

let me know when they're fixed :)

frewsxcv · 2016-03-16T20:11:55Z

let me know when they're fixed :)

Assuming your only nit was this one, then it's fixed

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.

frewsxcv · 2016-03-17T03:23:56Z

Merge conflicts addressed.

nikomatsakis · 2016-03-17T11:23:05Z

@bors r+

bors · 2016-03-17T11:23:06Z

📌 Commit abd1cea has been approved by nikomatsakis

bors · 2016-03-17T14:52:31Z

⌛ Testing commit abd1cea with merge be989ac...

…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.

bors · 2016-03-17T18:08:41Z

rust-highfive assigned brson Mar 15, 2016

frewsxcv reviewed Mar 15, 2016
View reviewed changes

frewsxcv force-pushed the compiletest-ignored-expected branch from 2991d5b to fb96cc8 Compare March 15, 2016 14:17

frewsxcv reviewed Mar 15, 2016
View reviewed changes

rust-highfive assigned nikomatsakis and unassigned brson Mar 16, 2016

nikomatsakis reviewed Mar 16, 2016
View reviewed changes

frewsxcv force-pushed the compiletest-ignored-expected branch from fb96cc8 to 0f8fdba Compare March 16, 2016 20:07

frewsxcv force-pushed the compiletest-ignored-expected branch from 0f8fdba to abd1cea Compare March 17, 2016 03:23

bors merged commit abd1cea into rust-lang:master Mar 17, 2016

frewsxcv deleted the compiletest-ignored-expected branch March 17, 2016 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

frewsxcv commented Mar 15, 2016

rust-highfive commented Mar 15, 2016

frewsxcv commented Mar 15, 2016

frewsxcv Mar 15, 2016

fhahn Mar 15, 2016

frewsxcv Mar 15, 2016

nikomatsakis Mar 16, 2016

frewsxcv Mar 16, 2016

frewsxcv Mar 16, 2016

frewsxcv commented Mar 15, 2016

frewsxcv commented Mar 15, 2016

frewsxcv commented Mar 16, 2016

nikomatsakis Mar 16, 2016

frewsxcv Mar 16, 2016

nikomatsakis Mar 16, 2016

frewsxcv Mar 16, 2016

nikomatsakis commented Mar 16, 2016

nikomatsakis commented Mar 16, 2016

frewsxcv commented Mar 16, 2016

frewsxcv commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

bors commented Mar 17, 2016

bors commented Mar 17, 2016

bors commented Mar 17, 2016

		@@ -1013,8 +1013,8 @@ fn check_expected_errors(revision: Option<&str>,
		expected_errors.iter()
		.fold((false, false),

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

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

Conversation

frewsxcv commented Mar 15, 2016

rust-highfive commented Mar 15, 2016

frewsxcv commented Mar 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv commented Mar 15, 2016

frewsxcv commented Mar 15, 2016

frewsxcv commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Mar 16, 2016

nikomatsakis commented Mar 16, 2016

frewsxcv commented Mar 16, 2016

frewsxcv commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

bors commented Mar 17, 2016

bors commented Mar 17, 2016

bors commented Mar 17, 2016