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 issue 19729 - Constructor overloads coming from mixin are not resolved #11652

Merged
merged 1 commit into from Sep 4, 2020
Merged

fix issue 19729 - Constructor overloads coming from mixin are not resolved #11652

merged 1 commit into from Sep 4, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 30, 2020

The test b17259.d was created to check that an ICE was fixed but then this became a reject-valid.
Make it working for good this time.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @NilsLankila!

Bugzilla references

Auto-close Bugzilla Severity Description
19729 normal Constructor overloads coming from mixin are not resolved

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#11652"

Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

This needs a bit more test.

  1. Add a test with named mixin
  2. How does it work in case of conflict ?
  3. How does it work if there's an exact match mixed in and a conversion match at the top level

src/dmd/func.d Outdated Show resolved Hide resolved
src/dmd/func.d Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Aug 30, 2020

I can answer, as overload sets are tried at the end:

How does it work in case of conflict ?

mixin looses, top level decl wins

How does it work if there's an exact match mixed in and a conversion match at the top level

mixin looses, top level decl wins

But I'll push tests. The idea, as specified in the official doc is that one can name a mixin when there are conflicts. For constructor this is not possible to disambiguate so top level wins.

@ghost ghost requested a review from Geod24 August 30, 2020 14:31
@ghost
Copy link
Author

ghost commented Sep 3, 2020

ping @Geod24, I have pushed the tests you required four days ago, requested a second review but you still block this bugfix.

Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

ping @Geod24, I have pushed the tests you required four days ago, requested a second review but you still block this bugfix.

Not on purpose, I promise, because I'm looking forward to this bug fix too :)
For reference, anyone is free to discard an (addressed) review if the reviewer is busy.

I added a last test case, otherwise LGTM.

test/fail_compilation/fail19729.d Show resolved Hide resolved
…olved

The test _b17259.d_ was created to check that an ICE was fixed but then this became a reject-valid.
Make it working for good this time.
@ghost ghost added the Merge:auto-merge label Sep 4, 2020
@dlang-bot dlang-bot merged commit 893579d into dlang:master Sep 4, 2020
@ghost ghost deleted the issue-19729 branch September 4, 2020 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants