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

Disallow Unsized Enums #37111

Merged
merged 3 commits into from
Oct 26, 2016
Merged

Disallow Unsized Enums #37111

merged 3 commits into from
Oct 26, 2016

Conversation

TimNN
Copy link
Contributor

@TimNN TimNN commented Oct 12, 2016

Fixes #16812.

This PR is a potential fix for #16812, an issue which is reported again and again, with over a dozen duplicates by now.

This PR is mainly meant to promoted discussion about the issue and the correct way to fix it.

This is a [breaking-change] since the error is now reported during wfchecking, so that even the definition of a (potentially) unsized enum will cause an error (whereas it would previously cause an ICE at trans time if the enum was used in an unsized manner).

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 12, 2016

Hmm. So this is intended to work -- but I agree that it'd be better to give a clean error, short of making it work. Probably this would require a crater run to get some idea.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler @rust-lang/lang and @eddyb in particular =)

This PR proposes to remove the ability for an enum to be unsized. They were intended to support this, but it was never fuly implemented, and the compiler currently ICEs if you really try to do anything. There is some backwards compatbility risk here, but it's pretty minor: you have to have enums that are potentially unsized but not used as such, I think. I'll try to kick off a crater run to get more data.

I think I'm in favor of making them a hard error (to fix the ICEs) but opening an issue to implement the support properly.

@nrc
Copy link
Member

nrc commented Oct 13, 2016

How difficult would it be to fix unsized enums? Is there design work to be done there or does it just need the implementation finished off?

@eddyb
Copy link
Member

eddyb commented Oct 13, 2016

@nrc I don't think anything about them was ever specified, and I'd expect a RFC or at least ammendments.

@nikomatsakis
Copy link
Contributor

@eddyb

I don't think anything about them was ever specified, and I'd expect a RFC or at least ammendments.

Can you say a bit more here? Off the top of my head I'm not sure what complications you are imagining would require amending RFCs and the like.

@eddyb
Copy link
Member

eddyb commented Oct 17, 2016

@nikomatsakis There are no defined semantics. Which cases are allowed? One maybe-unsized variant? Potentially all of them? Does the pointer metadata behave as an union tagged by the enum itself?
(Right now we have no unsized types that need the pointer to determine information like size/align)

I suppose CoerceUnsized is orthogonal so there's less to define than I initially thought.

If we can agree on something and we can implement it by reusing univariant functionality in most places (as opposed to needing a lot of special logic), then I'm okay with just a RFC-less FCP.

@nikomatsakis
Copy link
Contributor

Started crater run from a29c49f to c5483ff3b9fb8b5fdb633707b3e38c66c83ddae6.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 19, 2016

Crater results: https://gist.github.com/13d56401d4ceee692d4ad9374c8d3600

6540 crates tested: 4832 working / 1648 broken / 6 regressed / 0 fixed / 54 unknown.

UPDATE: They all appear to be false positives.

@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 19, 2016
@eddyb
Copy link
Member

eddyb commented Oct 19, 2016

@nikomatsakis The google crates are auto-generated, ouch. I'd rather just make it work then 😞.

EDIT I should have waited.

@nikomatsakis
Copy link
Contributor

Appears to be no regressions. I'm inclined to think we should just make this an error for now. I don't know that I think that an RFC is necessarily required to implement these changes, but it couldn't hurt in any case -- and ICEing is no good.

@rfcbot fcp merge

@nikomatsakis
Copy link
Contributor

@eddyb I think it's all false, no? Though we could try (and perhaps should) testing those crates by hand.

@rfcbot
Copy link

rfcbot commented Oct 19, 2016

Team member nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.
Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@TimNN
Copy link
Contributor Author

TimNN commented Oct 19, 2016

Great! Then I'll see about updating / adding tests in the next few days.

@TimNN
Copy link
Contributor Author

TimNN commented Oct 19, 2016

I could successfully build all listed "regressions" with a local stage 1 of c5483ff3b9fb8b5fdb633707b3e38c66c83ddae6.

@TimNN
Copy link
Contributor Author

TimNN commented Oct 24, 2016

I have adapted and added the test cases.

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

// ignore-test the unsized enum no longer compiles
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have ignored this test for now, but it has effectively become useless and could be deleted but I'm unsure what the policy is in this case.

@withoutboats
Copy link
Contributor

@rfcbot reviewed

Seems like making this an error is the correct thing to do for now.

@rfcbot
Copy link

rfcbot commented Oct 25, 2016

All relevant subteam members have reviewed. No concerns remain.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 25, 2016

📌 Commit db03257 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

thanks tim

On Wed, Oct 19, 2016 at 01:40:23PM -0700, Tim Neumann wrote:

I could successfully build all listed "regressions" with a local stage 1 of c5483ff3b9fb8b5fdb633707b3e38c66c83ddae6.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#37111 (comment)

bors added a commit that referenced this pull request Oct 25, 2016
Disallow Unsized Enums

Fixes #16812.

This PR is a potential fix for #16812, an issue which is reported [again](#36801) and [again](#36975), with over a dozen duplicates by now.

This PR is mainly meant to promoted discussion about the issue and the correct way to fix it.

This is a [breaking-change] since the error is now reported during wfchecking, so that even the definition of a (potentially) unsized enum will cause an error (whereas it would previously cause an ICE at trans time if the enum was used in an unsized manner).
@bors
Copy link
Contributor

bors commented Oct 25, 2016

⌛ Testing commit db03257 with merge aef18be...

@bors bors merged commit db03257 into rust-lang:master Oct 26, 2016
@TimNN TimNN deleted the sized-enums branch October 26, 2016 08:08
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 1, 2016
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. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: Unexpected type returned from struct_tail
9 participants