Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

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

Closed
wants to merge 4 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e23b3e
Fix Issue 19902 - hasElaborateCopyConstructor doesn't know about copy…
edi33416 Jul 31, 2019
55bcfb8
Refactor
edi33416 Aug 5, 2019
57f0667
Fix static struct issue
edi33416 Aug 5, 2019
75d28ea
Disabled postblit means no elaborate copy constructor
edi33416 Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion src/core/internal/traits.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,63 @@ template hasElaborateCopyConstructor(S)
}
else static if (is(S == struct))
{
enum hasElaborateCopyConstructor = __traits(hasMember, S, "__xpostblit");
enum hasElaborateCopyConstructor = __traits(hasMember, S, "__xpostblit") ||
() {
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the help.

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

(*p)(s);

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

@TurkeyMan TurkeyMan Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this important?

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

  1. compiler would construct via a move, and the copy constructor is irrelevant
  2. 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).

Copy link
Member

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@TurkeyMan TurkeyMan Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

});
if (r) return true;
}}
}
return false;
}();
}
else
{
enum bool hasElaborateCopyConstructor = false;
}
}

@safe unittest
{
static struct S
{
int x;
this(return scope ref typeof(this) rhs) { }
this(int x, int y) {}
}

static assert(hasElaborateCopyConstructor!S);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


static struct S2
{
int x;
this(int x, int y) {}
}

static assert(!hasElaborateCopyConstructor!S2);

static struct S3
{
int x;
this(return scope ref typeof(this) rhs, int x = 42) { }
this(int x, int y) {}
}

static assert(hasElaborateCopyConstructor!S3);
}

template hasElaborateAssign(S)
{
static if (__traits(isStaticArray, S) && S.length)
Expand Down