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 ICE on recursively defined enum variant discriminant. #26515

Merged
merged 2 commits into from
Jul 9, 2015

Conversation

quantheory
Copy link
Contributor

Fixes #23302.

Note that there's an odd situation regarding the following, most likely due to some inadequacy in const_eval:

enum Y {
    A = 1usize,
    B,
}

In this case, Y::B as usize might be considered a constant expression in some cases, but not others. (See #23513, for a related problem where there is only one variant, with no discriminant, and it doesn't behave nicely as a constant expression either.)

Most of the complexity in this PR is basically future-proofing, to ensure that when Y::B as usize is fully made to be a constant expression, it can't be used to set Y::A, and thus indirectly itself.

@rust-highfive
Copy link
Collaborator

r? @huonw

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

@quantheory
Copy link
Contributor Author

Two more things:

  1. I removed E0266, because it didn't look to me like it should be possible to trigger it, i.e. it should be span_bug and not span_err!.

  2. I guess that the broader issue of when an enum is a constant expression is now in enum variants aren't considered to be const during const-evaluation #23898.

@huonw
Copy link
Member

huonw commented Jul 7, 2015

I... don't really know much about this.

r? @nrc (random selection from the compiler subteam)

@rust-highfive rust-highfive assigned nrc and unassigned huonw Jul 7, 2015
// option. This file may not be copied, modified, or distributed
// except according to those terms.

enum X {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment explaining what is being tested

@nrc
Copy link
Member

nrc commented Jul 7, 2015

r=me with a couple of comments added

@quantheory
Copy link
Contributor Author

@nrc Is this better? (I commented the first discriminant_map definition, rather than the one you pointed out.)

@nrc
Copy link
Member

nrc commented Jul 9, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 9, 2015

📌 Commit b952c0e has been approved by nrc

@nrc
Copy link
Member

nrc commented Jul 9, 2015

yes, thanks!

@bors
Copy link
Contributor

bors commented Jul 9, 2015

⌛ Testing commit b952c0e with merge afe25a2...

bors added a commit that referenced this pull request Jul 9, 2015
Fixes #23302.

Note that there's an odd situation regarding the following, most likely due to some inadequacy in `const_eval`:

```rust
enum Y {
    A = 1usize,
    B,
}
```

In this case, `Y::B as usize` might be considered a constant expression in some cases, but not others.  (See #23513, for a related problem where there is only one variant, with no discriminant, and it doesn't behave nicely as a constant expression either.)

Most of the complexity in this PR is basically future-proofing, to ensure that when `Y::B as usize` is fully made to be a constant expression, it can't be used to set `Y::A`, and thus indirectly itself.
@bors bors merged commit b952c0e into rust-lang:master Jul 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants