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

Move sentence and explain constructor rules better in Classes.md #214

Merged
merged 1 commit into from
Apr 26, 2016

Conversation

pdfernhout
Copy link
Contributor

The text did not make sense with the sentence about calling super() between a sentence that leads into an example (with a colon) and the example.

This issue was introduced in this commit: d0fa464

@@ -73,12 +73,12 @@ tom.move(34);
This example covers quite a few of the inheritance features in TypeScript that are common to other languages.
Here we see the `extends` keywords used to create a subclass. You can see this where `Horse` and `Snake` subclass the base class `Animal` and gain access to its features.

Derived classes that contain constructor functions must call `super()` which will execute the constructor function on the base class.
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this part of the preceding paragraph? I would add two sentences after this one, something like:

You have to call super() before accessing this in the derived constructor.
Both Horse and Snake technically do this since neither one actually references this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandersn I have added those two sentences. Since you seemed to want to clarify restrictions about constructors, I also added other sentences to explain another restriction from the TypeScript spec about when the 'super' call had to be the first statement in the constructor.

I moved all that into its own paragraph since it got a bit long. It might be too long now, so feel free to suggest further improvements.

I also added a blank line to split up the earlier paragraph, since the sentence about "This example covers quite a few of the inheritance features..." now leads into the three paragraphs that follow which each explain a different concern (extends, constructor restrictions, and overriding).

I left the changes as two separate commits -- the original one to fix an issue and the second one as an enhancement. Let me know if you would prefer them squashed together into one commit though.

@pdfernhout pdfernhout force-pushed the classes.md-sentence-order-fix branch 7 times, most recently from 8628bec to 1f0d716 Compare March 30, 2016 02:15
Here we see the `extends` keywords used to create a subclass. You can see this where `Horse` and `Snake` subclass the base class `Animal` and gain access to its features.

TypeScript has some restrictions about constructors intended to help ensure instances of a class have their properties correctly initialized before the properties are used. Derived classes that contain constructor functions must call `super()` which will execute the constructor function on the base class. You have to call `super()` before accessing `this` in the derived constructor. Both `Horse` and `Snake` technically do that since neither one actually references `this`. A 'super' call must also be the first statement in the constructor of a derived class when the derived class contains initialized properties or has parameter properties. Neither `Horse` or `Snake`, however, have those kind of properties right now. So, as long as those two classes stay that way, they could have console logging or other statements before the `super` call in their constructors without generating compile-time errors.
Copy link
Member

Choose a reason for hiding this comment

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

One sentence per line please. We do this for easy diff-ability.

Copy link
Member

Choose a reason for hiding this comment

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

Use backticks around "super" in "A 'super' call"

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 feedback. I made those improvements and also added parentheses for each super call reference to be consistent.

@pdfernhout pdfernhout force-pushed the classes.md-sentence-order-fix branch from 1f0d716 to 89a00ed Compare March 30, 2016 12:11
@pdfernhout pdfernhout changed the title Move sentence in Classes.md Move sentence and explain constructor rules better in Classes.md Mar 30, 2016
@pdfernhout pdfernhout force-pushed the classes.md-sentence-order-fix branch from 89a00ed to 14d87d8 Compare March 30, 2016 12:21
@sandersn
Copy link
Member

👍 I like the changes. We don't care much about commits, so just leave them as-is.

Here we see the `extends` keywords used to create a subclass. You can see this where `Horse` and `Snake` subclass the base class `Animal` and gain access to its features.

TypeScript has some restrictions about constructors intended to help ensure instances of a class have their properties correctly initialized before the properties are used.
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not really a TypeScript restriction. it is a JS one. and enforced by the fact that the super, and the this share the same object, and that the object is not well formed, until super call has returned.

Copy link
Contributor

Choose a reason for hiding this comment

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

how about?

Initialization of a Base class properties is done as part of calling the Base constructor in a derived class, i.e. `super()` call.
As a result, `this` can not be used in a constructor of a derived class before `super()` call.
A `super()` call must also be the first statement in the constructor of a derived class when the derived class contains initialized properties or has parameter properties.

Copy link
Member

Choose a reason for hiding this comment

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

@mhegazy's suggestion looks good, with the following applied:

Base -> base
can not -> cannot
before a super() call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, is that change intended to replace just that sentence or all the text in the paragraph? If it is just to change the first sentence, then it introduces redundancy that should at least be removed from the rest. But if it is to replace everything, it loses the specificity to the Animal example.

The purpose of the introductory sentence was to motivate the reader to understand the intent behind these rules. I'd agree it could be worded better.

There is also perhaps a subtle distinction here between different sorts of errors -- compile time, run time, and conceptual/algorithmic. Not to be too pedantic, but here is a JSFiddle with the code that the TypeScript playground output for the Animal example modified with a console.log statement added accessing this (which returns undefined at that point) before a super() call in a constructor. So, you can access a not-well-formed object in JavaScript. It is just usually not worth doing. So, in that sense, the TypeScript compiler is attempting to protect against a conceptual/algorithmic mistake by these rules. Now, it is possible writing code in such a problematical way will also mess up a JavaScript virtual machine's optimization processes (maybe?), but that is not quite the same as a language restriction.

And these constructor restrictions are slightly overzealous as discussed in TypeScript issue #945 -- because there is no reason, say, a logging statement of "got here" is going to hurt anything if you do it before a super call in a constructor in a derived class. The underlying issue seems to me to be that the compiler can't easily determine how safe such calls before super() are in order to protect the programmer from creating various unexpected inconsistencies that may be hard to debug. Java has restrictive rules like this with perhaps more justification given its complex memory model and multithreading, and it is often frustrating in Java that you can't easily call other functions in complex ways to prepare data to pass to a superclass in a constructor. I'm not saying the TypeScript constructor rules should change -- they are a reasonable tradeoff. I'm just trying to be clear about their intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a runtime error to access this before calling super in es6.

I think we could relax the rule that super has to be the first statement now, that we have the other rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am sorry i have lost track of this PR. so if all what is needed is moving the sentence around, why not just do that and do not change the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy The original pull request just moved that sentence. Then there was a request for more changes by @sandersn which I made, with some other additions I added. In hindsight, it probably would have been best to have suggested back then that we make a new issue for discussion of any additional changes to the text so the original pull request could go through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy As you requested, I created a new TypeScript issue on changing constructor restrictions (TypeScript issue #8277).

Copy link
Contributor

Choose a reason for hiding this comment

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

i would go back to moving the sentence then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy To make incremental progress, as suggested, I have brought the pull request back to its original form that just moves this sentence:

+-Derived classes that contain constructor functions must call super() which will execute the constructor function on the base class.

Because the latest push no longer has these changes on GitHub, below are the other changes in the second commit for future reference.

This was my addition (where we have debated whether the origin of the restrictions is more due to JavaScript/ES6 or TypeScript):

+TypeScript has some restrictions about constructors intended to help ensure instances of a class have their properties correctly initialized before the properties are used.

These were additions by @sandersn (which would always be accurate):

+You have to call super() before accessing this in the derived constructor.
+Both Horse and Snake technically do that since neither one actually references this.

This was my further additions (which would be obsolete if TypeScript relaxes the restriction):

+A super() call must also be the first statement in the constructor of a derived class when the derived class contains initialized properties or has parameter properties.
+Neither Horse or Snake, however, have those kind of properties right now.
+So, as long as those two classes stay that way, they could have console logging or other statements before the super() call in their constructors without generating compile-time errors.

The text did not make sense with the sentence about calling super() between a sentence that leads into an example (with a colon) and the example. 

This issue was introduced in this commit: microsoft@d0fa464
@pdfernhout pdfernhout force-pushed the classes.md-sentence-order-fix branch from bf4ccce to 9091b49 Compare April 26, 2016 11:37
@pdfernhout
Copy link
Contributor Author

@sandersn I've brought the pull request back to just moving the sentence based on discussion for a now outdated diff with @mhegazy. Perhaps we can move forward with integrating this less-controversial change which is clearly needed and then discuss other incremental enhancements to the constructor explanation in another issue?

@sandersn
Copy link
Member

Sure, sorry about the wild goose chase. Thanks for your patience on this one.

@sandersn sandersn merged commit 9277f03 into microsoft:master Apr 26, 2016
@pdfernhout
Copy link
Contributor Author

@sandersn Thanks for merging the commit. I learned more about TypeScript and ES6 in the process of working on this, so the chase was good exercise. :-)

Also, sometimes when things are challenging to document, that means part of a design could be improved. So, perhaps TypeScript itself will improve further from the issue opened on relaxing constructor rules, which in turn would make it easier for us to improve the constructor documentation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants