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 Issues 19399 and 10560 - Different Conversion Rules for Same Value and Type Enum #10099

Closed
wants to merge 1 commit into from

Conversation

RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Jun 27, 2019

Once the enum value has been analyzed and lowered to a literal expression, if the enum type cannot be implicitly converted to the desired type, VRP steps in and does the appropriate type modifications; this is problematic because it bypasses the type that the user has provided. To fix this, I added a check that disables VRP if the user provided type cannot be implicitly converted to the desired type; if no type is provided for the enum, the initial behavior is still in place (i.e. [1]).

[1] https://issues.dlang.org/show_bug.cgi?id=9999

@RazvanN7 RazvanN7 requested a review from ibuclaw as a code owner June 27, 2019 13:12
@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 27, 2019

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
10560 critical Enum typed as int with value equal to 0 or 1 prefer bool over int overload
19399 critical Different Conversion Rules for Same Value and Type -- Enum

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#10099"

@RazvanN7
Copy link
Contributor Author

Phobos fix: dlang/phobos#7094

@thewilsonator
Copy link
Contributor

This should target stable

@RazvanN7
Copy link
Contributor Author

This cannot target stable. It is silent change of code behavior; I thought about issuing a deprecation, but deprecations are issued for errors.

@thewilsonator
Copy link
Contributor

Fair enough.

src/dmd/dcast.d Outdated
if (auto edType = e.type.isTypeEnum)
{
auto ed = edType.sym;
if (ed.hasUserDefinedType && m == MATCH.nomatch)
Copy link
Member

Choose a reason for hiding this comment

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

Merge above two lines, no point to ed variable.

src/dmd/denum.d Outdated
@@ -65,6 +66,8 @@ extern (C++) final class EnumDeclaration : ScopeDsymbol
//printf("EnumDeclaration() %s\n", toChars());
type = new TypeEnum(this);
this.memtype = memtype;
if (memtype)
hasUserDefinedType = true;
Copy link
Member

Choose a reason for hiding this comment

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

This introduces a very subtle bug. Take a look at the function getMemType(). If memtype is not provided, it is set to a default value. Now take a look at syntaxCopy(), which creates a new EnumDeclaration and passes memtype to it. But if memtype was set by getMemType(), it is not null, and hasUserDefinedType is incorrectly set to true.

The easiest way to fix this is have syntaxCopy() also copy hasUserDefinedType.

Copy link
Member

Choose a reason for hiding this comment

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

Or more properly, make hasUserDefinedType a parameter to the constructor.

@RazvanN7 RazvanN7 closed this Feb 15, 2020
@RazvanN7 RazvanN7 reopened this Jun 16, 2020
@WalterBright
Copy link
Member

The referenced issues are not compiler bugs and are not language bugs. See explanations added to https://issues.dlang.org/show_bug.cgi?id=19399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants