D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 20965 - Implicitly generated postblit overrides disabled copy ctor
Summary: Implicitly generated postblit overrides disabled copy ctor
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P1 major
Assignee: No Owner
URL:
Keywords: pull
Depends on:
Blocks:
 
Reported: 2020-06-21 11:00 UTC by Stanislav Blinov
Modified: 2020-11-11 10:49 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Stanislav Blinov 2020-06-21 11:00:31 UTC
There's a bit of a contradiction in the spec. On one hand, it says [1]:

"When a copy constructor is defined for a struct, all implicit blitting is disabled for that struct"

On the other [2]:

"WARNING: The postblit is considered legacy and is not recommended for new code. Code should use copy constructors defined in the previous section. For backward compatibility reasons, a struct that defines both a copy constructor and a postblit will only use the postblit for implicit copying."

and

"...the compiler may generate ... postblit functions"

[1] https://dlang.org/spec/struct.html#struct-copy-constructor
[2] https://dlang.org/spec/struct.html#struct-postblit

What that results in in practice is that a generated postblit overrides copy constructors. Consider:

struct C
{
    this(this) {}
}

struct S
{
    C c;
    @disable this(ref typeof(this));
}

void main()
{
    S s1;
    auto s2 = s1; // problem
}

The line 'problem' compiles. I posit that it shouldn't. Alas:
- a copy ctor is present (and disabled), therefore implicit blitting should be disabled, however
- C defines a postblit, therefore a postblit is implicitly generated for S, therefore S "defines" both a copy ctor and a postblit after all, therefore only the postblit is considered. Even though it is implicit and so should be disabled :)

Consider that type C comes from elsewhere. Programmers that write new code, ostensibly following advice from the WARNING cited above, shouldn't have to manually introspect dependencies to find out whether they have postblits (that is, if the whole point is to get rid of postblits). The language should make the right call here, i.e. adhere to the first rule from the copy ctors spec: disable all implicit blits (i.e. include generated postblits in that rule).
Comment 1 RazvanN 2020-06-25 05:03:25 UTC
You are correct, however, the initial idea was to favor the use of postblits until they are deprecated. If you want to make sure that all copying is disabled, simply do:

@disable this(this);
@disable this(ref typeof(this));

You don't have to reflect on any code this way.

The rule of thumb is: if you want to make sure that a posblit does not get in your way you must defensively disable the postblit and then the copy constructor will have priority.
Comment 2 Max Samukha 2020-06-25 05:50:09 UTC
Sorry, that's not a good design. If postblit is going to be deprecated, let people gradually prune their codebases of postblits instead of forcing them to add more!
Comment 3 Stanislav Blinov 2020-06-25 09:40:11 UTC
(In reply to RazvanN from comment #1)

> @disable this(this);
> @disable this(ref typeof(this));
> 
> You don't have to reflect on any code this way.

If I don't want to further infest the codebase with postblits, I have to.

> The rule of thumb is: if you want to make sure that a posblit does not get
> in your way you must defensively disable the postblit and then the copy
> constructor will have priority.

Postblits will never go away then, until they stop compiling.
Comment 4 RazvanN 2020-11-11 02:52:02 UTC
(In reply to Stanislav Blinov from comment #3)
> (In reply to RazvanN from comment #1)
> 
> > @disable this(this);
> > @disable this(ref typeof(this));
> > 
> > You don't have to reflect on any code this way.
> 
> If I don't want to further infest the codebase with postblits, I have to.
> 
> > The rule of thumb is: if you want to make sure that a posblit does not get
> > in your way you must defensively disable the postblit and then the copy
> > constructor will have priority.
> 
> Postblits will never go away then, until they stop compiling.

I agree, however, initially you start with tons of codebases that have postblit and 0 that have copy constructor. The DIP [1] explicitly says:

"In order to assure a smooth transition from postblit to copy constructor, the following strategy is employed: if a `struct` defines a postblit (user-defined or generated) all copy constructor definitions will be ignored for that particular `struct` and the postblit will be preferred. Existing code bases that do not use the postblit may start using the copy constructor without any problems, while codebases that rely on the postblit may start writing new code using the copy constructor and remove the deprecated postblit from their code."

This was Walter's request and I don't think that this will change. 

[1] https://github.com/dlang/DIPs/pull/129/files#diff-ecee0474c4314cd47dd8c2656b485c0cfd56e704a85de75839ec2850fb61f0ebR282
Comment 5 RazvanN 2020-11-11 02:52:59 UTC
I will make a PR to try and fix this and see what happens.
Comment 6 Dlang Bot 2020-11-11 04:08:18 UTC
@RazvanN7 created dlang/dmd pull request #11945 "Fix Issues 20714, 20965 - Postblit has priority over copy constructor" fixing this issue:

- Fix Issues 20714, 20965 - Postblit has priority over copy constructor

https://github.com/dlang/dmd/pull/11945
Comment 7 Dlang Bot 2020-11-11 04:56:35 UTC
dlang/dmd pull request #11945 "Fix Issues 20714, 20965 - Postblit has priority over copy constructor" was merged into master:

- 40d0661190ac132dbb5d61e3804dd22bbea26602 by RazvanN7:
  Fix Issues 20714, 20965 - Postblit has priority over copy constructor

https://github.com/dlang/dmd/pull/11945
Comment 8 Dlang Bot 2020-11-11 10:49:12 UTC
dlang/dmd pull request #11947 "Revert PR 11945 (Postblit has priority over copy constructor)" was merged into master:

- bc6976a4cc7abe799856689c077406a269dc0c51 by Geod24:
  Revert "Fix Issues 20714, 20965 - Postblit has priority over copy constructor"
  
  This reverts commit c35ace622492da4ca53c055c9af0fa0346aa178b.
  
  PR 11945 / this commit introduces a silent change of behavior,
  which leads to fields with postblit not having their postblit
  called anymore.
  A deprecation path was proposed, and would require the user to
  disable postblit when a copy constructor is present,
  or issue a deprecation message otherwise.
  Other options are likely available to avoid a silent behavior change.
  However, since the PR was merged within 4 minutes of being submitted,
  there were no time for such suggestions.
  Additionally, the PR was lacking both a spec PR and a changelog entry.

https://github.com/dlang/dmd/pull/11947