Fix Issue 20863 - Passing aggregate types by alias parameter drops qualifier #11320

ntrel · 2020-06-24T11:53:03Z

No description provided.

dlang-bot · 2020-06-24T11:53:05Z

Thanks for your pull request and interest in making D better, @ntrel! 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
✓	20863	normal	Passing aggregate types into templates by alias drops qualifier

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

andralex

Thanks!

andralex · 2020-06-24T16:02:56Z

Looks like the autotester doesn't like it. Can you please take a look? E.g. https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=4093573&isPull=true

Depends on dlang/dmd#11320

ntrel · 2020-06-25T15:39:38Z

So I think dmd tests are now passing, but there is a wrong assert in Phobos's std.meta. This pull should fix that - is there a way of making the auto-tester merge that before testing this pull?

Necessary for dlang/dmd#11320

UplinkCoder · 2020-07-08T12:03:51Z

This does look like a somewhat scary change.
It does change semantics in a subtle way.
Perhaps a revert switch should be there to be able to have a way out if this causes problems.

ibuclaw

This is a breaking change that affects production/active projects.

e.g: https://buildkite.com/dlang/dmd/builds/14142#aabac065-d196-4981-bc2b-c796800aebe9/106-432

At the very least need to either deprecate the old behaviour, or pick the -transition or -revert route.

ntrel · 2020-07-10T07:17:27Z

I added a revert switch, I don't think a deprecation can work here.

ibuclaw

I added a revert switch, I don't think a deprecation can work here.

In the absence of a clear deprecation path then, cannot merge until...

All downstream projects covered by buildkite are patched and new releases made.
Changelog entry added saying what was fixed (demonstrating old and new behaviour, link to bugzilla issue), and that this is a breaking change.
A test for the new -revert switch has been added
C++ headers have been updated

ntrel · 2020-07-10T08:26:00Z

All downstream projects covered by buildkite are patched

That's only one, here's the fix:
vibe-d/vibe.d#2458

UplinkCoder · 2020-07-10T11:33:08Z

Hmm I wonder how many production projects are accurately sampled by buildkite.
If a change to a buildkite tested projects has to be made, it is likely that a project which is not tested by buildkite also has to be changed.

schveiguy · 2020-07-10T13:29:26Z

Given that most templates that had to deal with both symbols and types had to use two overloads (one with alias or one with a type), or use a tuple, I'd say the breakage is going to be pretty minimal.

I think the vibe.d case is a problem with the unittest possibly. The test was added 7 years ago, and the implementation was changed at that same commit (vibe-d/vibe.d@53b6947). I bet @mihails-strasuns just copied what the thing spit out for the unit test, not really intending it to be so inconsistent.

schveiguy · 2020-07-10T13:33:21Z

Note that this fixes a latent bug in NoDuplicates which treats things like const(Object) and Object to be the same.

Edit: of course, there is a bug report, I should have looked :)

https://issues.dlang.org/show_bug.cgi?id=11577

WalterBright · 2021-05-02T18:09:51Z

src/dmd/dtemplate.d

@@ -8108,8 +8109,13 @@ MATCH matchArg(TemplateParameter tp, Scope* sc, RootObject oarg, size_t i, Templ
        if (sa)
        {
            if ((cast(Dsymbol)sa).isAggregateDeclaration())
+            {
+                // Don't lose type qualifiers


I'm curious why it lost the qualifiers in the first place.

I don't want to merge this until I find out why there's a special case for aggregates (the bug doesn't appear for non-aggregates). I suspect the real flaw is elsewhere.

Dsymbols doesn't have qualifiers and you get one as result of the getDsymbol call a little above.
We need to investigate why is it there and if it's possible to drop it, or just avoid getting a symbol if the type has any qualifier.

WalterBright

Need to find out why the qualifier was stripped only from aggregates in the first place.

See dlang/dmd#11320

RazvanN7 · 2021-09-09T07:22:20Z

@WalterBright any progress on this? This is currently blocking work in phobos

dlang-bot added the Severity:Bug Fix label Jun 24, 2020

ntrel marked this pull request as draft June 24, 2020 12:09

andralex approved these changes Jun 24, 2020

View reviewed changes

ntrel added a commit to ntrel/phobos that referenced this pull request Jun 25, 2020

std.meta: Fix wrong assert for Issue #20863

68ab740

Depends on dlang/dmd#11320

ntrel mentioned this pull request Jun 25, 2020

std.meta: Fix wrong assert for Issue #20863 dlang/phobos#7542

Merged

ntrel force-pushed the alias-qual branch from 098bf9b to e7f394c Compare June 25, 2020 15:43

ntrel mentioned this pull request Jun 25, 2020

Heads up: dmd fix for alias parameter missing qualifier breaks unittest vibe-d/vibe.d#2455

Open

ntrel marked this pull request as ready for review June 26, 2020 11:02

ntrel added a commit to ntrel/phobos that referenced this pull request Jul 6, 2020

std.meta: Comment out (fix) wrong assert for Issue #20863

af629b1

Necessary for dlang/dmd#11320

ntrel force-pushed the alias-qual branch from e7f394c to f564e33 Compare July 8, 2020 11:46

ibuclaw requested changes Jul 8, 2020

View reviewed changes

ntrel added 2 commits July 9, 2020 17:06

Fix Issue 20863 - Passing aggregate types by alias param drops qualifier

5d76bd3

Only use workaround if type qualifiers present

e7c930c

ntrel force-pushed the alias-qual branch from f564e33 to 44cbd10 Compare July 10, 2020 07:15

ibuclaw requested changes Jul 10, 2020

View reviewed changes

Add -revert=aliasaggquals

45b63ad

ntrel force-pushed the alias-qual branch from 44cbd10 to 45b63ad Compare July 10, 2020 07:48

ntrel added a commit to ntrel/vibe.d that referenced this pull request Jul 10, 2020

Fix buildkite/dmd for dlang/dmd#11320

7d904c2

ntrel mentioned this pull request Jul 10, 2020

meta/codegen.d: Fix getSymbols for dlang/dmd#11320 vibe-d/vibe.d#2458

Open

Geod24 pushed a commit to ntrel/vibe.d that referenced this pull request Dec 16, 2020

Fix buildkite/dmd for dlang/dmd#11320

624ed90

Merge branch 'master' into alias-qual

ce39632

ntrel mentioned this pull request May 1, 2021

[std.meta] Remove workarounds for an alias parameter not accepting basic types dlang/phobos#7513

Closed

ntrel requested a review from kinke as a code owner May 1, 2021 11:40

ntrel added 2 commits May 1, 2021 12:51

Add test for -revert=aliasaggquals

dbe8b9a

Update C++ header from autotester diff

7ed5481

ntrel force-pushed the alias-qual branch from e75afc8 to 7ed5481 Compare May 1, 2021 11:51

WalterBright reviewed May 2, 2021

View reviewed changes

WalterBright requested changes May 2, 2021

View reviewed changes

s-ludwig added a commit to vibe-d/vibe.d that referenced this pull request May 3, 2021

Ensure that getSymbols unqualifies types.

fd614a6

See dlang/dmd#11320

s-ludwig mentioned this pull request May 3, 2021

Ensure that getSymbols unqualifies types. vibe-d/vibe.d#2556

Merged

dlang-bot added Review:Needs Rebase Review:Needs Work labels May 26, 2021

dlang-bot added the Review:stalled label Aug 1, 2021

gedaiu pushed a commit to gedaiu/vibe.d that referenced this pull request Aug 6, 2021

Ensure that getSymbols unqualifies types.

ee114a2

See dlang/dmd#11320

dlang-bot removed the Review:stalled label Sep 9, 2021

dlang-bot added the Review:stalled label Dec 8, 2021

dlang-bot added Merge:needs rebase stalled labels Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 20863 - Passing aggregate types by alias parameter drops qualifier #11320

Fix Issue 20863 - Passing aggregate types by alias parameter drops qualifier #11320

ntrel commented Jun 24, 2020

dlang-bot commented Jun 24, 2020

andralex left a comment

andralex commented Jun 24, 2020

ntrel commented Jun 25, 2020

UplinkCoder commented Jul 8, 2020

ibuclaw left a comment

ntrel commented Jul 10, 2020

ibuclaw left a comment •

edited by maxhaton

Loading

ntrel commented Jul 10, 2020

UplinkCoder commented Jul 10, 2020

schveiguy commented Jul 10, 2020

schveiguy commented Jul 10, 2020 •

edited

Loading

WalterBright May 2, 2021

WalterBright May 2, 2021

BorisCarvajal May 2, 2021

WalterBright left a comment

RazvanN7 commented Sep 9, 2021

Fix Issue 20863 - Passing aggregate types by alias parameter drops qualifier #11320

Are you sure you want to change the base?

Fix Issue 20863 - Passing aggregate types by alias parameter drops qualifier #11320

Conversation

ntrel commented Jun 24, 2020

dlang-bot commented Jun 24, 2020

Bugzilla references

Testing this PR locally

andralex left a comment

Choose a reason for hiding this comment

andralex commented Jun 24, 2020

ntrel commented Jun 25, 2020

UplinkCoder commented Jul 8, 2020

ibuclaw left a comment

Choose a reason for hiding this comment

ntrel commented Jul 10, 2020

ibuclaw left a comment • edited by maxhaton Loading

Choose a reason for hiding this comment

ntrel commented Jul 10, 2020

UplinkCoder commented Jul 10, 2020

schveiguy commented Jul 10, 2020

schveiguy commented Jul 10, 2020 • edited Loading

WalterBright May 2, 2021

Choose a reason for hiding this comment

WalterBright May 2, 2021

Choose a reason for hiding this comment

BorisCarvajal May 2, 2021

Choose a reason for hiding this comment

WalterBright left a comment

Choose a reason for hiding this comment

RazvanN7 commented Sep 9, 2021

ibuclaw left a comment •

edited by maxhaton

Loading

schveiguy commented Jul 10, 2020 •

edited

Loading