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

Downgrade unit struct match via S(..) warnings to errors #30753

Merged

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jan 7, 2016

Downgrade unit struct match via S(..) warnings to errors

The error signalling was introduced in #29383

It was noted as a warning-cycle-less regression in #30379

Fix #30379

@rust-highfive
Copy link
Collaborator

r? @jroesch

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

@pnkfelix
Copy link
Member Author

pnkfelix commented Jan 7, 2016

r? @nikomatsakis

(This is high priority regression fix)

@rust-highfive rust-highfive assigned nikomatsakis and unassigned jroesch Jan 7, 2016
@@ -659,6 +659,12 @@ pub fn check_pat_enum<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
let report_bad_struct_kind = |is_warning| {
bad_struct_kind_err(tcx.sess, pat.span, path, is_warning);
if is_warning {
// Boo! Too painful to attach this to the actual warning,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think we should modify bad_struct_kind_err to look at is_warning and add a lint in the case of is_warning == true. This will also require adding a new lint corresponding to this future incompatibility. That said, I was going to do a quick PR to improve that tooling so as to address #30746, so maybe I could just roll that change into that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I debated about doing this but decided I was better off keeping this PR minimal to ease backporting to beta... what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

(I think I should have pushed back harder here. But then again I didn't expect bors to take four days to get to this)

@pnkfelix pnkfelix force-pushed the downgrade-29383-struct-warnings-to-errors branch from 66bafbe to 40e2ac2 Compare January 8, 2016 00:09
@pnkfelix
Copy link
Member Author

pnkfelix commented Jan 8, 2016

@nikomatsakis Okay I think this is what you want w.r.t. the lint structure. How does this look to you?

@nikomatsakis
Copy link
Contributor

Looks great!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 8, 2016

📌 Commit 40e2ac2 has been approved by nikomatsakis

@pnkfelix
Copy link
Member Author

pnkfelix commented Jan 8, 2016

failures:

    [compile-fail] compile-fail/match-pattern-field-mismatch-2.rs

    [compile-fail] compile-fail/pattern-error-continue.rs

hmm, why did that arise on the travis build

@pnkfelix
Copy link
Member Author

pnkfelix commented Jan 8, 2016

@bors r-

i need to look at the two failure cases

@pnkfelix
Copy link
Member Author

pnkfelix commented Jan 8, 2016

@bors r=nikomatsakis fa027d1

…nd then exit.

I think that behavior is fine, so I am removing the expected warnings from these tests.
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 8, 2016

📌 Commit fa027d1 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors r-

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 8, 2016

📌 Commit fa027d1 has been approved by nikomatsakis

@brson brson added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 8, 2016
@bors
Copy link
Contributor

bors commented Jan 11, 2016

⌛ Testing commit fa027d1 with merge 41f7bf7...

@bors
Copy link
Contributor

bors commented Jan 11, 2016

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

@alexcrichton
Copy link
Member

@bors: retry

On Mon, Jan 11, 2016 at 4:53 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-msvc-32-opt
http://buildbot.rust-lang.org/builders/auto-win-msvc-32-opt/builds/1878


Reply to this email directly or view it on GitHub
#30753 (comment).

@pnkfelix
Copy link
Member Author

@bors p=1

(upping priority for regression fixing or beta-nominated PR's)

@bors
Copy link
Contributor

bors commented Jan 11, 2016

⌛ Testing commit fa027d1 with merge 5cf69aa...

bors added a commit that referenced this pull request Jan 11, 2016
…rors, r=nikomatsakis

Downgrade unit struct match via S(..) warnings to errors

The error signalling was introduced in #29383

It was noted as a warning-cycle-less regression in #30379

Fix #30379
@bors bors merged commit fa027d1 into rust-lang:master Jan 11, 2016
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2016
@pnkfelix
Copy link
Member Author

@nikomatsakis I added #30724 to the beta-nominated set because it seemed like a natural candidate for a back port to beta (and also because 40e2ac2 in this PR depends on infrastructure added by that PR)

however, given that the beta is going to be cut very soon, perhaps back porting PR #30724 would be unwise, and thus I should make a separate PR against beta branch that only goes up to 8aed830 (inclusive)

Thoughts?

Cc @brson @rust-lang/compiler

@nikomatsakis
Copy link
Contributor

@pnkfelix and I discussed on IRC -- seems like best thing is to backport this PR and just the (very few) bits of infrastructure needed to make it work.

@brson brson added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants