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

Fix various bugs around empty structs and patterns #29383

Merged
merged 4 commits into from
Nov 28, 2015

Conversation

petrochenkov
Copy link
Contributor

Fixes #28692
Fixes #28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens UnitVariant(..) for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@nikomatsakis
Copy link
Contributor

Heh, yeah, I like to do Foo(..) if I only care about the variant, so that it is forwards compatible with adding parameters later (which happens pretty regularly).

@pnkfelix
Copy link
Member

This PR looks fine to me. It does need a crater run.

(I have not yet successfully run crater but I'm wiling to give it another shot.)

@bors
Copy link
Contributor

bors commented Nov 2, 2015

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

@petrochenkov
Copy link
Contributor Author

Rebased.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2015

@petrochenkov great; I am seeing if I can do a crater run now with it.

@alexcrichton alexcrichton added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 2, 2015
@alexcrichton
Copy link
Member

I've made a new needs-crater tag on GitHub so we can keep track of which PRs need a crater run so we know how to move them forward

@bors
Copy link
Contributor

bors commented Nov 4, 2015

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

@petrochenkov
Copy link
Contributor Author

Rebased.

@bors
Copy link
Contributor

bors commented Nov 5, 2015

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

@pnkfelix
Copy link
Member

pnkfelix commented Nov 5, 2015

@petrochenkov thanks for rebasing

@pnkfelix
Copy link
Member

pnkfelix commented Nov 5, 2015

I did a crater run (on the revisions from before the rebase).

Zero regressions.

crater results here: https://gist.github.com/pnkfelix/4b800d1faf17eb494d8e


Update: I just looked at those results again, and realized that, if I understand them correctly, they are saying that all crates failed to build on both of my compilers. :(

I'm not quite sure what I did wrong, if anything. I'll try a new run on the rebased versions.

@pnkfelix pnkfelix removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 5, 2015
@pnkfelix
Copy link
Member

pnkfelix commented Nov 5, 2015

(So, r=me after next rebase.)


Update: see above comment. The aforementioned crater run seemed problematic, not b/c of this PR, but probably user error.

@pnkfelix pnkfelix added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 5, 2015
@petrochenkov
Copy link
Contributor Author

Rebased.

@bors
Copy link
Contributor

bors commented Nov 11, 2015

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

@petrochenkov
Copy link
Contributor Author

Rebased.

@alexcrichton, could you run crater on this?

@alexcrichton
Copy link
Member

Starting a crater run.

@bors
Copy link
Contributor

bors commented Nov 16, 2015

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

@alexcrichton
Copy link
Member

Crater reports eight root regressions with as many as up to 34. I glanced at the root ones and they at least all look legitimate.

Perhaps that error could be a warning for a few cycles? Seems like a good bugfix to have, but may be good to give fair warning.

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 16, 2015
@petrochenkov
Copy link
Contributor Author

Rebased.

@petrochenkov
Copy link
Contributor Author

ping @pnkfelix
beta is soon

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 27, 2015

📌 Commit af96402 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Nov 28, 2015

⌛ Testing commit af96402 with merge f0fcef8...

@bors
Copy link
Contributor

bors commented Nov 28, 2015

💔 Test failed - auto-mac-32-opt

@petrochenkov
Copy link
Contributor Author

fatal: unable to connect to github.com:
github.com[0: 192.30.252.130]: errno=Operation timed out

Needs retry

@bluss
Copy link
Member

bluss commented Nov 28, 2015

@bors retry

@bors
Copy link
Contributor

bors commented Nov 28, 2015

⌛ Testing commit af96402 with merge e9ac440...

bors added a commit that referenced this pull request Nov 28, 2015
Fixes #28692
Fixes #28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens `UnitVariant(..)` for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.
@bors bors merged commit af96402 into rust-lang:master Nov 28, 2015
@bluss
Copy link
Member

bluss commented Nov 30, 2015

It looks like it's a breaking change anyway, conv 0.3.1 0.3.0 + git

src/errors.rs:576:17: 576:32 warning: `NegOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:576             Err(NegOverflow(..)) => T::neg_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:576:17: 576:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:576:17: 576:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for det
ails.
src/errors.rs:576             Err(NegOverflow(..)) => T::neg_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:577:17: 577:32 warning: `PosOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:577             Err(PosOverflow(..)) => T::pos_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:577:17: 577:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:577:17: 577:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for details.
src/errors.rs:577             Err(PosOverflow(..)) => T::pos_infinity(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:602:17: 602:32 warning: `NegOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:602             Err(NegOverflow(..)) => T::saturated_min(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:602:17: 602:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:602:17: 602:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for details.
src/errors.rs:602             Err(NegOverflow(..)) => T::saturated_min(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:603:17: 603:32 warning: `PosOverflow` does not name a tuple variant or a tuple struct [E0164]
src/errors.rs:603             Err(PosOverflow(..)) => T::saturated_max(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:603:17: 603:32 help: run `rustc --explain E0164` to see a detailed explanation
src/errors.rs:603:17: 603:32 note: this warning will become a HARD ERROR in a future release. See RFC 218 for details.
src/errors.rs:603             Err(PosOverflow(..)) => T::saturated_max(),
                                  ^~~~~~~~~~~~~~~
src/errors.rs:574:9: 578:10 error: non-exhaustive patterns: `Err(_)` not covered [E0004]
src/errors.rs:574         match self.map_err(Into::into) {
src/errors.rs:575             Ok(v) => v,
src/errors.rs:576             Err(NegOverflow(..)) => T::neg_infinity(),
src/errors.rs:577             Err(PosOverflow(..)) => T::pos_infinity(),
src/errors.rs:578         }
src/errors.rs:574:9: 578:10 help: run `rustc --explain E0004` to see a detailed explanation
src/errors.rs:600:9: 604:10 error: non-exhaustive patterns: `Err(_)` not covered [E0004]
src/errors.rs:600         match self.map_err(Into::into) {
src/errors.rs:601             Ok(v) => v,
src/errors.rs:602             Err(NegOverflow(..)) => T::saturated_min(),
src/errors.rs:603             Err(PosOverflow(..)) => T::saturated_max(),
src/errors.rs:604         }

@petrochenkov
Copy link
Contributor Author

Hm, the warning should not affect exhaustiveness check, I'll investigate what happens.

@bluss
Copy link
Member

bluss commented Nov 30, 2015

I apologise if I'm pointing to the wrong PR, but it seems related.

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
@petrochenkov petrochenkov deleted the empstr branch September 21, 2016 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants