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

Remove repr-lookup attempt in derive expansion #24320

Closed
pnkfelix opened this issue Apr 11, 2015 · 3 comments
Closed

Remove repr-lookup attempt in derive expansion #24320

pnkfelix opened this issue Apr 11, 2015 · 3 comments
Labels
C-bug Category: This is a bug. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

Spawned off of #24270, in particular this discussion: #24270 (diff)

The new code for deriving(PartialOrd) attempts to look up if there is a repr attribute on the enum that is driving the deriving expansion, because we need (at least) to incorporate knowledge of whether the discriminant is meant to be interpreted as a signed integer or an unsigned one (since the current intrinsic always returns a u64 today).

However, the look up may be fragile, since @sfackler points out that a later pass of expansion could add the repr that we are seeking.

There are a number of different ways to solve this problem (see the linked discussion for some of them).

@pnkfelix
Copy link
Member Author

I do not see this as a terribly high priority item.

Exposing the problem requires injecting a repr attribute at a later pass of expansion, but I am not sure there are currently any instances of expansions that do this.

(Also, you would need to be explicitly assigning discriminant values where the sign-bit could be misinterpreted in such a test case. Which certainly can happen -- I'm just saying that you'd need quite a confluence of events to actually witness the bug.)

@pnkfelix
Copy link
Member Author

triage: P-low

@rust-highfive rust-highfive added the P-low Low priority label Apr 11, 2015
@steveklabnik steveklabnik added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed A-compiler labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 22, 2017
@Mark-Simulacrum
Copy link
Member

I think this is largely unsolveable to the extent that derives are always going to be less than perfect (we don't know enough at expansion time). In particular, we hit this issue with repr(packed) types and related desire for the type to be Copy...

I'm going to close this issue as I don't think it's worthwhile to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants