Fix Issue 19902 - hasElaborateCopyConstructor doesn't know about copy… #2709

edi33416 · 2019-07-31T12:08:58Z

… constructors

dlang-bot · 2019-07-31T12:09:01Z

Thanks for your pull request and interest in making D better, @edi33416! 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
✓	19902	regression	hasElaborateCopyConstructor doesn't know about copy constructors

Testing this PR locally

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

dub fetch digger
dub run digger -- build "stable + druntime#2709"

src/core/internal/traits.d

thewilsonator · 2019-07-31T14:30:16Z

Please target stable as this is a critical issue.

edi33416 · 2019-07-31T15:35:44Z

As you can see, this patch is quite meaty.
This is because I needed ParameterDefaults from std.traits which depends on ParameterIdentifierTuple, which depends on isFunctionPointer and isDelegate.

After this PR is merged, I'll open subsequent PRs in Phobos to use the traits from core.internal.traits to avoid code duplication for the moved functions.

… constructors

edi33416 · 2019-07-31T16:02:46Z

Please target stable as this is a critical issue.

Did that. Can you please double check that I didn't mess things up?
Don't know why, but my local history turned into a mess after I first did the rebase with origin/stable

TurkeyMan · 2019-07-31T16:29:58Z

Thanks for doing this, bit woah, I feel like this is kinda wild. I wonder if we should use __traits if this is so hard?

edi33416 · 2019-07-31T19:19:51Z

Thanks for doing this, bit woah, I feel like this is kinda wild. I wonder if we should use __traits if this is so hard?

There isn’t a __traits for this. Are you suggesting we create a new one, something like __traits(hasCopyConstructor, S) ?

aliak00 · 2019-07-31T19:55:42Z

I feel the same as @TurkeyMan about maybe making a __trait if that's cleaner. Though, I managed to implement this without using std.traits, but it's not pretty:

template hasNonTrivialCopyConstructor(T) {
    bool isCopyConstructorFor(alias ctor, T)() {
        // Specialized function that checks specifically for "ref" in a string
        bool containsRef(string str) {
            foreach (i, c; str) {
                if (c == 'r') {
                    if (i + 2 < str.length) {
                        return str[i + 1] == 'e' && str[i + 2] == 'f';
                    }
                }
            }
            return false;
        }
        static if (is(typeof(ctor) Params == __parameters) && Params.length >= 1) {
            // Check that Params 0 is correct type for a copy constructro
            enum isCorrectType = is(Params[0] == T);

            // If more than one parameter, check that they are all defaulted
            static if (Params.length >= 1) {
                bool hasCorrectDefaults() {
                    bool result = true;
                    static foreach (I; 1 .. Params.length) {{
                        auto getDefault(Params[I..I+1] args) { return args[0]; }
                        result &= is(typeof(getDefault()));
                    }}
                    return result;
                } 
            } else {
                enum hasCorrectDefaults = true;
            }

            return isCorrectType
                && containsRef(Params[0..1].stringof)
                && hasCorrectDefaults;
        } else {
            return false;
        }
    }

    bool impl() {
        static if (__traits(hasMember, T, "__ctor")) {
            foreach (f; __traits(getOverloads, T, "__ctor")) {
                auto yes = isCopyConstructorFor!(f, T);
                if (yes) {
                    return yes;
                }
            }
        }
        return false;
    }

    enum hasNonTrivialCopyConstructor = impl;
}

wilzbach · 2019-07-31T21:06:38Z

+1 for adding a compiler trait. They are cheap and don't require massive CTFE evaluations.

TurkeyMan · 2019-07-31T22:16:45Z

I feel like this is a fact that the compiler must already just know, because the compiler decides to emit calls to copy constructors or just blit .init. the logic that drives the compilers decision should be reflected here verbatim, with no room for difference of behaviour in weird edge cases.

TurkeyMan · 2019-08-02T08:31:59Z

This is most important issue ;)

edi33416 · 2019-08-02T10:23:31Z

I'm currently working on making this into a __traits

TurkeyMan · 2019-08-02T17:33:55Z

Oh awesome, thanks!
My feeling is that whatever the logic looks like inside the compiler where it decides to blit or copy; that block of code should be pulled into a function, and then the __traits should route to that precise code.
This issue wouldn't have happen if it were like that the first time round ;)

edi33416 · 2019-08-05T12:09:06Z

src/core/internal/traits.d

+        this(int x, int y) {}
+    }
+
+    static assert(hasElaborateCopyConstructor!S);


Making S non-static will make the assert fail, and I can't figure out why. It's unclear to me if the behaviour is ok.

edi33416 · 2019-08-05T12:12:23Z

This is based on the observation that you can't pass a rvalue to a ref parameter.
This greatly simplifies the implementation.

edi33416 · 2019-08-05T12:13:48Z

The AppVeyor build seems to fail because it's not using the dmd/stable build.

CC @wilzbach

aliak00 · 2019-08-05T13:03:18Z

src/core/internal/traits.d

+                                auto p = &f;
+
+                                // Check if ctor is callable with lval or rval
+                                S s;


This won't work with function-local structs and probably with @disabled structs. S s = S.init should fix your static assert issues.

Thanks for the help.

Now I'm also taking into account @disabled structs.

TurkeyMan · 2019-08-06T01:26:04Z

src/core/internal/traits.d

+                                (*p)(s);
+
+                                // Check that ctor is't callable with rval
+                                static assert (!__traits(compiles, (*p)(S()) ));


Why is this important?

Given an rvalue, I feel like there are 2 possibilities here:

compiler would construct via a move, and the copy constructor is irrelevant

copy constructor can be invoke with an rvalue... and why is that bad? it's copied either way.

For the purposes of knowing whether there exists a copy constructor, i'm not sure it's interesting to know that it can be called with an rvalue, what we need to know is that it can be called with an lvalue...?

But back to my point in the other PR, I basically don't trust this code in general, and for the same reason we're fixing this issue in the first place.
The compiler deploys internal logic to determine if it will perform elaborate copying in assignment or just do a default blit, and library code needs to know which thing the compiler will do... we need to know exactly what the compiler will do, not just a good approximation which fails in some edge cases... and ideally not something that's brittle which must be maintained against ongoing changes to the compiler.

The compiler has a piece of logic used internally to determine this fact... I feel we should expose that EXACT piece of logic here (via __traits).

First off, this is a matter in which reasonable people may disagree.

So we have (a) implement in the language or (b) implement in library. In either case we don't want an approximation but something precise.

It's obvious that at a level appealing to a language/compiler change is easier. But that ignores larger trends. The main argument in favor of the library implementation is: If we (as we have repeatedly done in the past) run to the big brother to get any introspection done, I think that's a damaging pattern in the long run. We do not develop the understanding, tools, and patterns that allow us to do advanced introspection (the kind that would be difficult or impractical to refer to the compiler), but instead we develop a culture "if we can't do it as a trait we don't know how to do it".

BTW the code size in compiler vs library is comparable, and obviously the audience of the library code is much larger. The library implementation is complicated, the meaning is simpler:

auto p = &f; // Does this ctor accept lvalue but not rvalue? if (__traits(compiles, (S s) => (*p)(s)) && !__traits(compiles, (*p)(S()))) return true; // Cool, then it's a copy constructor

BTW the best illustration of the kind of idioms that could have remained undiscovered if we just said, "hey we need a new trait". @edi33416 and me had no idea how to do this. We experimented until we figured you can take the address of a ctor and then see how you can call it. That little idiom would have remained undiscovered.

I understand your argument, and I agree in principle for many cases of this category, but:

So we have (a) implement in the language [...]

It is already implemented in language, that's my point; it's absolutely implemented in the language, and that semantic applies to the code when the compiler chooses to blit, or call a function. We just can't react to that piece of language at the library level, or rather, we can indirectly, but not without some noise introduced into the system.

For instance, the test here wonders about rvalues; what's that about? Why is that even a factor? The traits implementation is one line exposing the compilers internal test, and it certainly doesn't ask questions about rvalue/lvalue-ness... that complexity appeared out of nowhere; it's just dealing with noise added to the signal because trying to infer the condition that the compiler performs indirectly.

If a precise in-library solution is possible, that's fine, but my confidence in a library solution being accurate decreases according to the number-of-lines² (squared). Every additional line required to express it in library doubles my suspicion that it's not accurate, and that we've missed something if it should be so hard.

In this case, the compiler evaluates a small expression to determine this fact; it seems simpler to make that expression a function, and make a traits call that function, then we capture the identical piece of logic. The rules that define elaborate copying are very low-level internal compiler implementation detail, and repeating that logic in the library, or in this case, attempting to extract it indirectly and then filter out the nouse, can only lead to bad-ness... DRY...

Again, I don't disagree with your principle, but I feel like this particular case should be exceptional, because it is a piece of internal compiler logic that we want to evaluate.

That little idiom would have remained undiscovered.

That's a cute anecdote, but probably not strictly true. It's not undiscovered knowledge, it's just that you needed to think of some trick to get there. I'm not really impressed by unsanitary and unnecessary additional work being performed to UN-DO work that was performed on top of the original value that you're trying to sample.

Situation, you have x, and I want x.
We have decided that I can't ask you "What is x?", instead, I can ask you "What is y where y = b*f(t)?", where t = x + 2, f() is a thing that we are kinda confident we understand and I think I can define the proper inverse, and b is an unrelated detail.
I now have to calculate x = invF(y/b)-2 to try and recover x. I hope that I defined infF() correctly, and all of that was completely unnecessary work. We have problems with compile times, and there is noise in the signal that I may not have filtered correctly.

Maybe this case is okay, but I'm concerned that there's mention of rvalues; that's an alarm bell. But either way, I want to temper your position; there's absolutely nothing wrong with asking the compiler to supply the precise evaluation of languages semantic detail which are lower level than the language has expressions to describe. It's more performant, and more precise. If the library solution is sufficiently indirect (additional work to reverse significant work), or requires filtering noise from the signal, then I think it's proper to consider a traits.

As I said, reasonable people may disagree about this. And in such instances, the person with most dedication to arguing will win. So, congratulations.

I will note the irony of "I feel like this particular case should be exceptional". We said that quite a few times...

I will note the irony of "I feel like this particular case should be exceptional". We said that quite a few times...

Well, we're all low-level ecosystem developers here, and we're the most likely to expose these gaps.
I feel like there's particular criteria that may be used to weight in one direction or the other in cases like this though; when it comes to std.traits, there's a whole lot which can be reasonably constructed with primitives we have in language; combinations of is() expressions or reflection for presence of detail, but this isn't a request for information in quite the same way as a lot of traits, it's a request for a piece of internal language semantic which has no expression in-language. So to solve it in library, we can only try and infer the original value, but expressing that in-language is tricky without injecting other unrelated elements of compiler logic into the equation. I'm sure we can do it, but it'll be long-winded and brittle, as evidenced by the fact we have this regression in the first place.

I actually agree with your core argument, it's just 'in this case' specifically...

wilzbach

It's still something that should be handled by the compiler to avoid weird edge cases

aliak00 · 2019-08-07T23:07:44Z

Additionally, and ideally, if it had already been implemented as manu suggest, this wouldn’t have even cropped up as a needed fix no? Duplicating compiler semantics in the standard library does sound a bit iffy sometimes.

…

On Wed, 7 Aug 2019 at 22:51, Manu Evans ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/core/internal/traits.d <#2709 (comment)>: > + else + { + enum hasElaborateCopyConstructor = () { + static if (__traits(hasMember, S, "__ctor")) + { + static foreach (f; __traits(getOverloads, S, "__ctor")) + {{ + bool r = __traits(compiles, { + auto p = &f; + + // Check if ctor is callable with lval or rval + S s = S.init; + (*p)(s); + + // Check that ctor is't callable with rval + static assert (!__traits(compiles, (*p)(S()) )); I understand your argument, and I agree in principle for many cases of this category, but: So we have (a) implement in the language [...] It is already implemented in language, that's my point; it's absolutely implemented in the language, and that semantic applies to the code when the compiler chooses to blit, or call a function. We just can't react to that piece of language at the library level, or rather, we can indirectly, but not without some noise introduced into the system. For instance, the test here wonders about rvalues; what's that about? Why is that even a factor? The traits implementation is one line exposing the compilers internal test, and it certainly doesn't ask questions about rvalue/lvalue-ness... that complexity appeared out of nowhere; it's just dealing with noise added to the signal because trying to infer the condition that the compiler performs indirectly. If a precise in-library solution is possible, that's fine, but my confidence in a library solution being accurate decreases according to the number-of-lines² (squared). Every additional line required to express it in library doubles my suspicion that it's not accurate, and that we've missed something if it should be so hard. In this case, the compiler evaluates a small expression to determine this fact; it seems simpler to make that expression a function, and make a traits call that function, then we capture the identical piece of logic. The rules that define elaborate copying are very low-level internal compiler implementation detail, and repeating that logic in the library, or in this case, attempting to extract it indirectly and then filter out the nouse, can only lead to bad-ness... DRY... Again, I don't disagree with your principle, but I feel like this particular case should be exceptional, because it is a piece of internal compiler logic that we want to evaluate. That little idiom would have remained undiscovered. That's a cute anecdote, but probably not strictly true. It's not undiscovered knowledge, it's just that you needed to think of some trick to get there. I'm not really impressed by unsanitary and unnecessary additional work being performed to UN-DO work that was performed on top of the original value that you're trying to sample. Situation, you have x, and I want x. We have decided that I can't ask you "What is x?", instead, I can ask you "What is y where y = b*f(t)?", where t = x + 2, f() is a thing that we are kinda confident we understand, and I think I can define the proper inverse, and b is an unrelated detail. I now have to calculate x = invF(y/b)-2 to try and recover x, but and I hope that I defined infF() correctly, and all of that was completely unnecessary work. We have problems with compile times, and there is noise in the signal that I may not have filtered correctly. Maybe this case is okay, but I'm concerned that there's mention of rvalues; that's an alarm bell. But either way, I want to temper your position; there's absolutely nothing wrong with asking the compiler to supply the precise evaluation of languages semantic detail which are lower level than the language has expressions to describe. It's more performant, and more precise. If the library solution is sufficiently indirect (additional work to reverse significant work), or requires filtering noise from the signal, then I think it's proper to consider a traits. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2709?email_source=notifications&email_token=AAUEDTHC7SC42WU3SJB2JA3QDMYTLA5CNFSM4IIF6AYKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCA44POQ#discussion_r311757322>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUEDTE7NYZXHJMDUSIMSIDQDMYTLANCNFSM4IIF6AYA> .

TurkeyMan · 2019-08-18T21:02:18Z

What's happening here? This has been blocking me for almost a month, and sadly, 2.088 just got forked >_< ... which mean's we're out by another whole release cycle!

edi33416 · 2019-08-19T12:51:42Z

@TurkeyMan I guess, that based on the discussion here, the dmd PR is the one way to go. That should get in first and then we can update this to use the internal trait.

TurkeyMan · 2019-09-05T05:47:41Z

This is blocking #2780

n8sh · 2019-09-06T01:14:34Z

I think the way forward is to complete this PR without waiting for DMD. If/when extra __traits are added the definition here can be simplified to use them.

n8sh · 2019-09-06T03:38:02Z

src/core/internal/traits.d

+        @disable this(this);
+    }
+
+    static assert(!hasElaborateCopyConstructor!S4);


This is in contradiction of the current std.traits definition of hasElaborateCopyConstructor:
https://run.dlang.io/is/r3wT27

import std.traits; struct S { @disable this(this); } void main() { assert(hasElaborateCopyConstructor!S); }

n8sh

From the description of the PR it seems that if hasElaborateCopyConstructor!T was already true it should not stop being true. That currently does not hold (see #2709 (review)).

edi33416 · 2020-01-20T09:02:57Z

Closing as this has been resolved by #2911

edi33416 requested review from MartinNowak and schveiguy as code owners July 31, 2019 12:08

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jul 31, 2019

RazvanN7 reviewed Jul 31, 2019

View reviewed changes

src/core/internal/traits.d Outdated Show resolved Hide resolved

edi33416 force-pushed the issue19902 branch from 81f176e to 0897027 Compare July 31, 2019 15:41

edi33416 changed the base branch from master to stable July 31, 2019 15:41

edi33416 force-pushed the issue19902 branch from 0897027 to 146037c Compare July 31, 2019 15:58

Fix Issue 19902 - hasElaborateCopyConstructor doesn't know about copy…

6e23b3e

… constructors

edi33416 force-pushed the issue19902 branch from 146037c to 6e23b3e Compare July 31, 2019 16:01

edi33416 mentioned this pull request Aug 2, 2019

Add hasElaborateCopyConstructor trait dlang/dmd#10265

Closed

Refactor

55bcfb8

edi33416 commented Aug 5, 2019

View reviewed changes

aliak00 reviewed Aug 5, 2019

View reviewed changes

edi33416 added 2 commits August 5, 2019 16:20

Fix static struct issue

57f0667

Disabled postblit means no elaborate copy constructor

75d28ea

TurkeyMan reviewed Aug 6, 2019

View reviewed changes

wilzbach suggested changes Aug 7, 2019

View reviewed changes

n8sh reviewed Sep 6, 2019

View reviewed changes

n8sh suggested changes Sep 6, 2019

View reviewed changes

pbackus mentioned this pull request Jan 19, 2020

Fix issue 19902 - hasElaborateCopyConstructor doesn't know about copy… #2911

Merged

edi33416 closed this Jan 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 19902 - hasElaborateCopyConstructor doesn't know about copy… #2709

Fix Issue 19902 - hasElaborateCopyConstructor doesn't know about copy… #2709

edi33416 commented Jul 31, 2019

dlang-bot commented Jul 31, 2019 •

edited

Loading

thewilsonator commented Jul 31, 2019

edi33416 commented Jul 31, 2019

edi33416 commented Jul 31, 2019

TurkeyMan commented Jul 31, 2019

edi33416 commented Jul 31, 2019

aliak00 commented Jul 31, 2019

wilzbach commented Jul 31, 2019

TurkeyMan commented Jul 31, 2019

TurkeyMan commented Aug 2, 2019

edi33416 commented Aug 2, 2019

TurkeyMan commented Aug 2, 2019

edi33416 Aug 5, 2019

edi33416 commented Aug 5, 2019

edi33416 commented Aug 5, 2019

aliak00 Aug 5, 2019

edi33416 Aug 5, 2019

TurkeyMan Aug 6, 2019 •

edited

Loading

andralex Aug 7, 2019

andralex Aug 7, 2019

andralex Aug 7, 2019

TurkeyMan Aug 7, 2019 •

edited

Loading

andralex Aug 8, 2019

TurkeyMan Aug 8, 2019

wilzbach left a comment

aliak00 commented Aug 7, 2019 via email

TurkeyMan commented Aug 18, 2019

edi33416 commented Aug 19, 2019

TurkeyMan commented Sep 5, 2019

n8sh commented Sep 6, 2019

n8sh Sep 6, 2019

n8sh left a comment

edi33416 commented Jan 20, 2020

Fix Issue 19902 - hasElaborateCopyConstructor doesn't know about copy… #2709

Fix Issue 19902 - hasElaborateCopyConstructor doesn't know about copy… #2709

Conversation

edi33416 commented Jul 31, 2019

dlang-bot commented Jul 31, 2019 • edited Loading

Bugzilla references

Testing this PR locally

thewilsonator commented Jul 31, 2019

edi33416 commented Jul 31, 2019

edi33416 commented Jul 31, 2019

TurkeyMan commented Jul 31, 2019

edi33416 commented Jul 31, 2019

aliak00 commented Jul 31, 2019

wilzbach commented Jul 31, 2019

TurkeyMan commented Jul 31, 2019

TurkeyMan commented Aug 2, 2019

edi33416 commented Aug 2, 2019

TurkeyMan commented Aug 2, 2019

Choose a reason for hiding this comment

edi33416 commented Aug 5, 2019

edi33416 commented Aug 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMan Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMan Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

aliak00 commented Aug 7, 2019 via email

TurkeyMan commented Aug 18, 2019

edi33416 commented Aug 19, 2019

TurkeyMan commented Sep 5, 2019

n8sh commented Sep 6, 2019

Choose a reason for hiding this comment

n8sh left a comment

Choose a reason for hiding this comment

edi33416 commented Jan 20, 2020

dlang-bot commented Jul 31, 2019 •

edited

Loading

TurkeyMan Aug 6, 2019 •

edited

Loading

TurkeyMan Aug 7, 2019 •

edited

Loading