Fix Issue 19415 - return non-copyable struct fails if member function has return attribute

[RazvanN7]

struct S
{
   int x;
   S foo() return { return S(x); }
   this(this) @disable;
}

S bar()
{
   S s;
   return s; // Error: struct `S` is not copyable because it is annotated with @disable
}

When struct S is semantically analyzed, the field StructDeclaration.postblit is null. Before actually building the postblit and setting StructDeclaration.postblit to point to it, all the members of struct S are semantically analyzed. This results in the fact that at the time when foo is analyzed, the struct does not appear to have a postblit. During the semantic analysis of foo, this line [1] is executed in order to delete the return attribute from the function. The code path is : TypeStruct.hasPointers() -> AggregateDeclaration.size() -> AggregateDeclaration.determineSize() -> StructDeclaration.finalizeSize(). At the end of finalizeSize this is done: [2]. The code path is: toArgTypes -> StructDeclaration.isPOD. Although the struct is not pod because it has a postblit, because the struct has not been semantically analyzed completely, it appears as it is POD, messing with the size of the struct and ultimately making it wrongfully not usable with NRVO.

The fix makes it so that AggregateDeclaration.size() is not called anymore to verify forward references. This should also improve performance since a lot of unnecessary checks are avoided.

This has truly been an adventure.

[1] https://github.com/dlang/dmd/pull/8346/files#diff-64d4abe08ba2355c2b95cb22e46b8572R876
[2] https://github.com/dlang/dmd/blob/master/src/dmd/dstruct.d#L429

[dlang-bot]

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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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
✓	19415	major	return non-copyable struct fails if member function has return attribute

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 "master + dmd#8983"

[schveiguy]

Wow, thanks for finding that so quickly!

[schveiguy]

Seems to fail for 64-bit builds, but when I look at the log, I can't figure out why. Looks like linker errors, but no actual message. Seems like it's std.container.util unittests.

[schveiguy]

You have the issue number messed up, should be 19415, not 19145.

[jacob-carlborg]

I would be nice to have the PR description i the commit message. Doing that from the beginning would automatically add the commit message as the PR description.

[RazvanN7]

@schveiguy

Seems to fail for 64-bit builds, but when I look at the log, I can't figure out why. Looks like linker errors, but no actual message. Seems like it's std.container.util unittests.

I found a better fix. It seems like StructDeclaration.size() was used abusively to check for forward references. I added the single check; this is correct and even more efficient.

You have the issue number messed up, should be 19415, not 19145.

Fixed.

@jacob-carlborg

I would be nice to have the PR description i the commit message. Doing that from the beginning would automatically add the commit message as the PR description.

I think it would be a bit too verbose.

[RazvanN7]

cc @WalterBright as it was your PR that introduced this bug : #8346