DIP 1004: Inherited Constructors #42

AndrejMitrovic · 2016-09-04T14:03:16Z

Related issue: https://issues.dlang.org/show_bug.cgi?id=9066

mihails-strasuns · 2016-09-04T16:15:04Z

Thanks! I will be unavailable for two weeks starting tomorrow but will check it right after.

Bolpat · 2016-09-04T20:02:22Z

You only mention alias super.this this;. How about alias this = super.this;? The spec features the latter. I don't like the first syntax as it collides with alias … this; which means something completely different, but it should be supported.

AndrejMitrovic · 2016-09-04T20:28:48Z

You only mention alias super.this this;. How about alias this = super.this;?

I think both could be supported. At some point there was talk about allowing alias this = type; for subtyping as well. Anyway I don't see why we can't support either or both syntaxes, super.this should be apparently different to subtyping.

Alternatively if there really is a visibility issue we could do something like: alias super.this() this(); and alias this() = super.this().

AndrejMitrovic · 2016-09-04T20:29:28Z

I think technically we have access to the __ctor symbol already, but that would look a bit ugly in user code.

PetarKirov · 2016-09-05T07:08:34Z

IIRC, it was proposed that alias member this, should be changed to alias this : member (instead of alias this = member) in order to emphasize the subtyping relationship and to avoid conflicts with alias this = super, which incidentally this DIP proposes. That proposal went nowhere, so alias this() = super.this() sounds like a good solution in the meantime.

mihails-strasuns

Looks good overall, just need to polish the details

mihails-strasuns · 2016-09-19T07:06:42Z

DIPs/DIP1004.md

+The process of writing forwarding constructors is tedious and may even be
+error-prone. It's possible to use existing language features such as
+template mixins to generate forwarding constructors, however this increases
+compilation times and also generates more object code.


afair the main issue wasn't compilation time but the fact it creates reference loop for semantic pass (can't reliably reflect on an aggregate while trying to mix in is method)

mihails-strasuns · 2016-09-19T07:10:54Z

DIPs/DIP1004.md

+
+The rationale for this current limitation: If the `Derived` object was constructible
+by calling only the `Base` class constructor then the `Derived` object itself
+could be left in an *unexpected* state. This is because the `Derived` constructors could


I'd suggest also mentioning also the code example when derived class has no own constructors (thus us rationale is not applicable) but it still doesn't work. Probably separate this section in two blocks as per https://github.com/dlang/DIPs/pull/42/files#diff-3830fabd6166499fcffc5e7cb94d0c69R32

mihails-strasuns · 2016-09-19T07:21:27Z

DIPs/DIP1004.md

+    }
+
+    /// inherit base class constructors
+    alias super.this this;


I wonder if alias super this makes more sense - there is no way to refer to constructor method by this name in language and at the same time semantically by forwarding constructors you are saying derived is a strict superset of base.

It's sort of odd to use alias super this; because it looks like subtyping, but on the other hand the derived object is a base class object already (implicitly converts to base).

I think I'll simply list a couple of proposed syntaxes and then let the community decide on the best choice.

mihails-strasuns · 2016-09-19T07:37:33Z

DIPs/DIP1004.md

+
+## Breaking changes
+
+There are no expected breaking changes with this proposal.


Not true, same as for adding any new feature there is a risk that some code that didn't compile in is(typeof()) will start doing so :) It is very unlikely to make difference for existing code bases but DIPs need to mention all possible risks.

Ah true. :)

mihails-strasuns · 2016-09-19T07:38:28Z

DIPs/DIP1004.md

+
+The feature syntax shares a similarity with object subtyping,
+however `alias this.super this` would not conflict with the subtyping feature
+since `super` is a reserved word.


Have you checked if existing grammar supports it already (I suppose alias this grammar is generic but need to make sure)?

Hmm I haven't, I did assume the compiler would currently reject alias this.super this.

MartinNowak · 2016-09-19T11:54:01Z

Just bringing in previous discussions about this.
Issue 9066 – Add constructor inheritance feature
In particularly the one proposal mentioned in Comment 13.

AndrejMitrovic · 2016-09-20T21:23:58Z

the one proposal mentioned in Comment 13.

You mean the disabling specific base class ctors? Yeah I must have skipped that part. I'll amend the DIP soon.

mihails-strasuns · 2016-09-26T16:25:18Z

Ping @AndrejMitrovic ;)

AndrejMitrovic · 2016-09-27T22:00:55Z

Ok updated with the proposed changes. I think with regards to syntax proposals this can still be discussed in the community. I'm sure we can come up with a reasonable syntax most people are happy with. But the basic idea for the feature is there.

mihails-strasuns

Thanks! I like the content, now it is only matter of formalizing and improving structure. I made some inline comments but checking existing state against https://github.com/dlang/DIPs/blob/master/README.md#advices-for-writing-great-dips can be a good idea too.

You may also want to re-generate table of contents now that more sections have been added.

mihails-strasuns · 2016-09-28T12:04:46Z

DIPs/DIP1004.md

+
+The rationale for this current limitation: If the `Derived` object was constructible
+by calling only the `Base` class constructor then the `Derived` object itself
+could be left in an *unexpected* state. This is because the `Derived` constructors could


mihails-strasuns · 2016-09-28T12:07:20Z

DIPs/DIP1004.md

+    new Derived(1, 2);  // error: constructor is disabled
+}
+```
+


Based on Andrei latest comment for DIP1002, would be a good idea to add here a section showing existing constructor boilerplate in public projects and how it would be simplified.

AndrejMitrovic · 2016-10-01T16:17:53Z

I'd suggest also mentioning also the code example when derived class has no own constructors

This was added at the start of the examples in the previous push.

Based on Andrei latest comment for DIP1002, would be a good idea to add here a section showing existing constructor boilerplate in public projects and how it would be simplified.

Good idea. I've added examples from an open source project, from Phobos, and I've also included the forwarding constructors mixin template as an example of what's already possible in library code (but which has its shortcomings).

AndrejMitrovic · 2016-10-01T16:20:13Z

Changed title in PR / code and commit message to Inherited Constructors rather than Inheriting Constructors, I think it sounds better this way.

Changes reading flow to first explain the problem and show existing solutions first and only to proceed with proposed changes after.

@disable

- Proposed changes are phrased as directives for compiler - Proposed changes are explained with straighforward before/after samples - @disable of constructors is split onto own section

Shows how common it is to define wrong exception class constructors with existing language semantics.

mihails-strasuns · 2016-10-24T08:59:36Z

I have created a pull request against tour branch with various proposed changes : AndrejMitrovic#1

Proposed editorial changes

AndrejMitrovic · 2016-11-06T18:49:20Z

@Dicebot let me know if you need this squashed (perhaps 1 origin commit and 1 fixup commit by you), or we could just keep the history as is.

mihails-strasuns · 2016-11-06T20:46:08Z

Don't bother, it will be all squashed into one commit on merge anyway. I'll wait for Andrei to prepare his final judgement of existing DIPs in the queue and merge this one if it doesn't fail any of possibly mentioned requirements.

mihails-strasuns · 2016-11-17T11:38:23Z

Andrei's judgement of last merged DIPs:

https://github.com/dlang/DIPs/blob/master/DIPs/DIP1001.md#review
https://github.com/dlang/DIPs/blob/master/DIPs/DIP1002.md#review

AndrejMitrovic · 2016-11-17T22:04:01Z

Yup, saw it. I'm not planning on throwing a tantrum if the DIP is rejected. :)

JohanEngelen · 2016-11-20T10:43:07Z

DIPs/DIP1004.md

+{
+    // Error: class Derived cannot implicitly generate a default ctor when
+    // base class Base is missing a default ctor
+    auto d = new Derived(10, 10);


(10,10): Is there some code missing elsewhere, or should this be () ?

Yes, this is the wrong snippet and error message, fix: AndrejMitrovic#2

DIP1004: Fix snippet for shadowing base default ctor

deadalnix · 2016-11-28T07:44:35Z

Ok 2 quick things.

1/ I love the general idea.
2/ I don't think the alias super.this this is necessary. Why not provide a mixin template to do this ? i would prefer a library solution here, as the proposed solution add to language complexity for a very specific use case and looks arcane and probably isn't necessary. If there is a major obstacle to this being done as a library, I'd like to know what it is.

AndrejMitrovic · 2016-11-28T20:51:17Z

Why not provide a mixin template to do this ?

We could certainly try. The question is if it will end up being a half-solution like the current Typedef situation.

deadalnix · 2016-11-28T22:56:06Z

I would consider that having a half working solution here to be a problem with out mixin mechanism, and that we should fix that rather than baking a workaround for a specific use case in the language itself.

mihails-strasuns · 2016-11-29T12:51:45Z

My 5 cents.

There are several proposed changes:

https://github.com/dlang/DIPs/blob/master/DIPs/DIP1004.md#implicit-constructor-inheritance-for-derived-classes-with-no-constructors

This can't truly be replaced by any library solution because they all have a problem with one of stated goals - make correct approach the default by making class MyException : Exception { } "just work".

https://github.com/dlang/DIPs/blob/master/DIPs/DIP1004.md#syntax-for-inheriting-base-class-constructors-for-derived-classes and https://github.com/dlang/DIPs/blob/master/DIPs/DIP1004.md#selective-disabling-of-inherited-constructors

Those certainly can be done a library utility (I remember having issues with semantic analysis cycles in the past but that was definitely a compiler bug). One can argue though that those are not new features and already work for any other class method - omitting constructors makes an artificial exception. I am less opinionated on this part though.

Anyway, before it gets to review stage, we should collect all proposed library implementations from NG and evaluate them as part of the DIP.

deadalnix · 2016-11-29T19:42:55Z

@Dicebot It was never question of making 1 as a library solution, so I'm not sure where you are going with that.

For 2, it is about copying a set a function from place to another. If we can't use reflection and mixin to do this, I'd argue this is the problem that need to be solved.

jmdavis · 2016-11-30T22:07:02Z

For exceptions, we do now have http://dlang.org/phobos/std_exception.html#.basicExceptionCtors, which at least makes mixing in the constructors easy, though it's not automatic. You have to explicitly mix them in. But thanks to improvements in the compiler, it actually works with ddoc, which wasn't the case in the past when such solutions were brought up.

mdparker · 2017-04-22T17:06:43Z

@andrej-mitrovic-sociomantic Please email me. Thanks!

mihails-strasuns added the dip-addition label Sep 4, 2016

mihails-strasuns suggested changes Sep 19, 2016

View reviewed changes

AndrejMitrovic force-pushed the inherited-constructors branch from edba45e to 4ec6c61 Compare September 27, 2016 21:58

mihails-strasuns suggested changes Sep 28, 2016

View reviewed changes

AndrejMitrovic force-pushed the inherited-constructors branch 3 times, most recently from 75674cf to e8b7b70 Compare October 1, 2016 16:16

DIP 1004: Inherited Constructors

1ce020a

AndrejMitrovic force-pushed the inherited-constructors branch from e8b7b70 to 1ce020a Compare October 1, 2016 16:19

AndrejMitrovic changed the title ~~DIP 1004: Inheriting Constructors~~ DIP 1004: Inherited Constructors Oct 1, 2016

Dicebot added 5 commits October 24, 2016 08:50

Reformat paragraphs to mostly fit width 80

a80b149

Restructure DIP sections

c19e600

Changes reading flow to first explain the problem and show existing solutions first and only to proceed with proposed changes after.

Restructure proposed change section internals

c49b59e

- Proposed changes are phrased as directives for compiler - Proposed changes are explained with straighforward before/after samples - @disable of constructors is split onto own section

Enhance real-world example section

f2b79fa

Shows how common it is to define wrong exception class constructors with existing language semantics.

Regenerate TOC

21e5fcc

Merge pull request #1 from Dicebot/inherited-constructors

456f540

Proposed editorial changes

JohanEngelen reviewed Nov 20, 2016

View reviewed changes

Dicebot and others added 2 commits November 20, 2016 14:43

Fix snippet for shadowing base default ctor

324665b

Merge pull request #2 from Dicebot/inherited-constructors

93b0bfd

DIP1004: Fix snippet for shadowing base default ctor

mihails-strasuns approved these changes Nov 28, 2016

View reviewed changes

mihails-strasuns merged commit 68bb203 into dlang:master Nov 28, 2016

AndrejMitrovic deleted the inherited-constructors branch January 28, 2017 00:11


		## Breaking changes

		There are no expected breaking changes with this proposal.

DIP 1004: Inherited Constructors #42

DIP 1004: Inherited Constructors #42

Conversation

AndrejMitrovic commented Sep 4, 2016 • edited

mihails-strasuns commented Sep 4, 2016

Bolpat commented Sep 4, 2016

AndrejMitrovic commented Sep 4, 2016

AndrejMitrovic commented Sep 4, 2016

PetarKirov commented Sep 5, 2016

mihails-strasuns left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinNowak commented Sep 19, 2016

AndrejMitrovic commented Sep 20, 2016

mihails-strasuns commented Sep 26, 2016

AndrejMitrovic commented Sep 27, 2016

mihails-strasuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrejMitrovic commented Oct 1, 2016

AndrejMitrovic commented Oct 1, 2016

mihails-strasuns commented Oct 24, 2016

AndrejMitrovic commented Nov 6, 2016

mihails-strasuns commented Nov 6, 2016

mihails-strasuns commented Nov 17, 2016

AndrejMitrovic commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadalnix commented Nov 28, 2016

AndrejMitrovic commented Nov 28, 2016

deadalnix commented Nov 28, 2016

mihails-strasuns commented Nov 29, 2016

deadalnix commented Nov 29, 2016

jmdavis commented Nov 30, 2016

mdparker commented Apr 22, 2017

AndrejMitrovic commented Sep 4, 2016 •

edited