Always allow code before super call when it does not use "this" #8277
Comments
mhegazy
added
Suggestion
|
Yeah, this an interesting issue because I personally have had to write special cases where you have to pass closures to the super to ensure they get executed before other blocks of code. On the other side of the coin, this is also an issue in C# as well because calling :base() you need to follow a similar pattern and have an onConstruct override or something like that. Or... all your properties need to be lazy.. :/ |
|
I also have many cases where I check / initialise local variables before calling super. It's great that TypeScript uses the best from other well-typed languages, but the way it is right now is simply out of line with common sense. Like per @jbaron example: constructor(id:String) {
var label = I18N.translate(id);
var icon = IconMap.get(id);
super(label, icon);
this.setBackground(this.color);
}
// vs…
constructor(id:String) {
super(I18N.translate(id), IconMap.get(id));
this.setBackground(this.color);
}That limitation doesn't bring in any value. There was an argument on complexity of the checks – doing check for |
mhegazy
added
Help Wanted
|
It pretty much looks like this is fixed for 2.0 beta. Can you give it a try @pdfernhout? |
|
@DanielRosenwasser It's probably all right now. The error message explains what went wrong precisely I think. This is fine: This is not: |
|
Could someone tell me why this code is not permitted ? `export class MatterAccessRevokedForUser extends MatterUserDomainEvent { }` |
|
This example clearly shows the need to allow code before super call when it does not use "this" to apply transformations to the parameters of super() Error: constructor(opts: ReadableOptions) {
opts.objectMode = true
super(opts)
}My solution constructor(opts: ReadableOptions) {
super((() => {
opts.objectMode = true
return opts
})())
} |
|
Is there any reason why having variable initiators on private variables in the sub class disables the above functionality? Moving the variable initialisation so it's after the super call fixes it - but I'm curious as to the reasoning as by definition private variables should not have any side effects on the base class? |
|
@davidglezz this is exactly what I've had to do. |
|
@DanielRosenwasser I confirmed things works better under TypeScript 2.6. I did not test earlier 2.x versions. The Handbook page on Classes also reflects this improvement. Thanks to everyone who worked on those improvements. There are still edge cases that could be improved further like @marlon-tucker and @captainjono raised -- but as far as I am concerned the fix so far is good enough. === More details Below is the test example I used -- where The third commented code statement contains a direct access to The first two commented code items are for an initialized property and a parameter property. They are both an indirect access to An edge case involving conditional code with a presumably analyzable bifurcation in the code path to call Further improvements in those directions might be nice -- but such work is at risk of producing diminishing returns after the previous improvement. So, it is OK with me to close this issue and suggest people open a new issue if they want to continue discussing relaxing constraints further in such cases. |
|
@mhegazy hi |
Fixes microsoft#8277. It feels wrong to put a new `forEachChild` loop in the checker, though in the vast majority of user files this will be a very quick one. Is there a better way to check for a reference to `super` or `this`?
|
The implementation details in #29374 are starting to reach into the transformer realm, so a summary of the findings (thanks @ajafff for pointing much of these out!):
The second point means this now requires some more changing to the checker and transformer. I think it's still doable if the original "you must call super immediately" requirement becomes "you must call a super before the end of the function or references to this or super" using control flow analysis. In other words, >=1 class Test extends Base {
prop = 1;
// Not acceptable, as the `super(...)` might not be reached
constructor() {
if (Math.random() > .5) super(1);
}
}class Test extends Base {
prop = 1;
// Acceptable, as there's no way for a `super(...)` not to be reached
constructor() {
if (Math.random() > .5) { super(1); }
else { super(0); }
}
}On top of that, Related but not the same: #23422 on the subject of multiple |
|
I think I found a bug, but since it’s directly related to this I’m not sure if it needs a separate issue. In the comments above (#8277 (comment), #8277 (comment)), people are talking about passing anon functions / immediately-invoked-function-expressions (IIFEs) as arguments to
The bug I’ve found is that this IIFE can reference constructor (opts: ReadableOptions) {
super((() => {
console.log(this) // oh no!
opts.objectMode = true
return opts
})())
}TypeScript in its current version (v3.9.2) does not emit an error, but when attempting to run this code, the JavaScript runtime will throw this error:
You can see a full playground here. Since Design Goal 1 is “Statically identify constructs that are likely to be errors”, I would propose adding to this issue by emitting an error for this use case. TLDR: An IIFE passed as an argument to |
|
I just encountered error ts(2376). As far as I could see my use case hasn't been mentioned in this thread, so I'll add it as another motivation why it's desirable to allow other code before In my case, the super constructor has side effects that must only run if the object instantiation is successful; and the derived class must validate its constructor arguments and throw on invalid values. It looks like this: abstract class Node {
#parent: Node | null;
#children = new Set<Node>();
constructor(parent: Node | null) {
this.#parent = parent;
if (parent) parent.#children.add(this);
}
destroy() {
this.#parent?.children.delete(this);
}
}
class TextNode extends Node {
#text: string;
constructor(parent: Node, text: string) {
if (!text) throw new Error("`text` is required");
super(parent);
this.#text = text;
}
}Moving the line |
|
I think I found another bug: Calling class Foo {
constructor (n: number) {
if (n < 0) throw new RangeError()
}
}
class Bar extends Foo {
constructor (n: number) {
try {
super(n)
} catch {
// Expected a TS error here:
// > Constructors for derived classes must contain a 'super' call.
}
}
}
new Bar(-1) // runtime error (not predicted by TS):
// > must call super constructor before using 'this' in derived class constructor
// > Must call super constructor in derived class before accessing 'this' or returning from derived constructor |
|
I would like to add to this an argument why allowing code before the super call might be a good idea. My argument is the following: not allowing to call super after regular code will entice young programmers to call super with dummy values just to get it initialized. This will lead to existing objects that are in an dangerous internal state. Add lazy loading to the mix and you have essentially a race-condition generator. In other words, if there can be no regular code before calling super, people are enticed to do that after super which will leave the object in a dangerous state for a short time. You don't see that since every member is initialized! Here is a very long example (please only read that if you have time for an coffee break): This is loosely based on a bug I just found in our code-base. Please note, we started with pure JavaScript and have converted this code to TypeScript. In JavaScript you can call the super later, therefore this code is written this way (I assume, I didn't write it). The problem is that you can't write this since you needs to call super first. So what we could have done: There are two problems her: the code is way too long (this is our fault since we made this mess) and we can not use the same configBuilder object! The problem is, if that object has an internal state you basically have to handle this object via an parameter even if that doesn't make sense. Alright, someone came up with a easy but pretty stupid solution: And it worked. Until someone did basically this: And this was the point where all objects figuratively exploded if the internet connection was too fast or your device was too fast or the sun was felling that way. And since you work in a nice office with good internet connection this bug isn't too obvious. TLDR; If we can't run regular code before super it will entice the usage of dummy values for the super call. This can lead to really ugly bugs. |
|
Here's a toy example: declare class T {
constructor(x: number, y: number);
}
class H extends T {
x = 0;
constructor() {
const rand = Math.random();
super(rand, rand);
}
}this cannot be represented in TS. Actually, it can, like this: declare class T {
constructor(x: number, y: number);
}
class H extends T {
x = 0;
static rand = NaN;
constructor() {
super(H.rand = Math.random(), H.rand);
}
}that is ugly, but TS allows it. ¯\_(ツ)_/¯ Otherwise, I strongly believe that TS2376 is obsolete with the introduction of TS17009. |
TypeScript should not do this, an arrow function can be passed to As an example, this is a simplified version of something I'm already doing today (and works): class MathView {
readonly #render: () => Element;
constructor(render: () => Element | Promise<Element>) {
this.#render = render;
}
async render(): Promise<Element> {
const element = await this.#render();
return wrapAndSetMetadata(element, this);
}
}
class MathPlusView extends MathView {
readonly addend: View;
readonly summand: View;
constructor(addend: View, summand: View) {
super(() => this.#render()); // This works fine, when the public
// .render() is called, "this" will be available
this.addend = addend;
this.summand = summand;
}
#render = async () => {
const [addendElement, summandElement] = await Promise.all([
this.addend.render(),
this.summand.render(),
]);
return fromTemplate`
<mrow>${ base }<mo>+</mo>${ superScript }</mrow>
`;
}
}Now TypeScript could detect "this" within IIFEs within |
|
@Jamesernator An IIFE (immediately-invoked function expression) is a function that is called immediately after it is defined. In your example, you have a regular arrow function, but it’s not called right away — as you correctly state, it doesn’t get called until the public In my example, an IIFE is sent as an argument to super( (() => {
console.log(this) // ReferenceError!
})() )
// ^ notice the call here — the function expression is invoked immediatelyYour code poses no danger, as the argument sent to |
|
ECMAScript class fields have arrived at Stage 4. In the standard, we can write expressions that don't contain |
|
Not using Typescript syntax cuz i hate compiling, but i use checkJS class A {}
class B extends A {
#x = ''
constructor (x, y, z) {
if (arguments.length < 3) throw new TypeError('Need more arguments')
super()
}
}A 'super' call must be the first statement in the constructor when a class contains |
|
Typescript is only a compilation tool ..I expect to be able to make assertions during runtime, as a protection against unexpected inputs! Here is my take on the problem class AssertionError extends Error {}
function assert<Value=unknown>(value: Value, assertion: string): asserts value {
if (!value) {
throw new AssertionError(assertion);
}
}
function assertNonNull<Value=unknown>(value: Value, valueName?: string, extra?: unknown): asserts value is NonNullable<Value> {
assert(value !== undefined && value !== null, 'Expecting a defined and non-null value');
}
class Base<Value extends unknown> {
protected _value: Value;
constructor(value: Value) {
this._value = value;
}
}
// IMPLEMENTATION 1 : desired usage
class Implementation<Value extends string> extends Base<Value> {
constructor(value: Value) {
assertNonNull(value); // typescript compiler-god is striking with great lighting
super(value);
//
// instance-related initialization...
//
}
}
const instance1 = new Implementation(null as any);
// IMPLEMENTATION 2 : workaround with factory pattern
class Implementation<Value extends string> extends Base<Value> {
private _safelyConstructed: boolean = false;
// generic type can't be passed to static, but could be in a `type` declaration above if DRY is required
static construct<Value extends string>(value: Value): Implementation<Value> {
assertNonNull(value);
const instance = new Implementation<Value>(value);
instance._safelyConstructed = true;
return instance;
}
constructor(value: Value) {
super(); // typescript compiler-god is appeased and laughing at the sweating developer
assert(this._safelyConstructed, 'Expecting instance to be safely constructed');
//
// instance-related initialization...
//
}
}
const instance2 = Implementation.construct(null as any);
// const instance2 = Implementation.new(null as any); // alternative name, it is valid
// const instance2 = new Implementation(null as any); // this is now unsafe...And yes, assertion could be done in the base class in some cases, but not if some inputs are narrowing the base class types... This pattern can be used for all the above cases too, essentially the constructor is wrapped with before and after logic. Although the construction function is Also, all classes extending an implementation with the factory-style workaround (actually the whole implementation tree extending it) will have to use the same pattern. So this may be cumbersome on the long run. |
|
Here is another approach using a hook pattern. This is one is more complex, but allows the syntax and interfaces to stay the same. class AssertionError extends Error {}
function assert<Value=unknown>(value: Value, assertion: string): asserts value {
if (!value) {
throw new AssertionError(assertion);
}
}
function assertNonNull<Value=unknown>(value: Value, valueName?: string, extra?: unknown): asserts value is NonNullable<Value> {
assert(value !== undefined && value !== null, 'Expecting a defined and non-null value');
}
// IMPLEMENTATION 1 : desired usage
class Base1<Value extends unknown> {
protected _value: Value;
constructor(value: Value) {
this._value = value;
}
}
class Implementation1<Value extends string> extends Base1<Value> {
constructor(value: Value) {
assertNonNull(value); // typescript compiler-god is striking with great lighting
super(value);
//
// instance-related initialization...
//
}
}
const instance1 = new Implementation1(null as any);
// IMPLEMENTATION 2 : workaround with internal hook pattern
class Base2<Value extends unknown> {
protected _value: Value;
constructor(value: Value) {
this._value = value; // assignment to appease the typescript compiler-god
this._construct(value);
this._init();
this._start();
}
// these abstract could be a base implementation instead, that can be overriden
abstract protected _construct(value: Value): void; // could use `this._value` since assignment was required by type
abstract protected _init(): void;
private _start() {
// base implementation
};
}
class Implementation2<Value extends string> extends Base2<Value> {
constructor(value: Value) {
super(value); // typescript compiler-god is appeased and laughing at the sweating developer
//
// instance-related initialization AFTER all the construtor hooks (construct, init, start) ...
//
}
protected _construct(value: Value) {
assertNonNull(value);
}
protected _init() {
// specific logic for this construction step, required since it is `abstract` in base class
};
// lets suppose `start()` is fine as it is already base-implemented
}
const instance2 = new Implementation2(null as any); // yay, this is safe and with the expected syntaxUnlike my previous attempt, the instanciation logic and interface stays inside the class encapsulation. It also allows the base class to expose a specific sequence/strategy of construction, which may help readability and decoupling. Essentially, use Unfortunately, if omitting assignments in If this is used, the If Like my previous workaround, all the derived sub-classes would need to call the construction implementation when overriding an existing hook, which moves responsibility of sub-classes to know what to do (generally calling _construct(value: Value) {
// logic before, requires base class knowledge
super._construct(value); // in theory optional, but only if the overriden logic IS optional, requires base class knowledge
// logic after
} |
This comment has been minimized.
This comment has been minimized.
|
@crimsoncodes0 actually, Im the one emitting assertions using my functions. My workarounds are to be able to use them in a implementation class before the You are right indeed, perhaps I should also open another ticket about the super call in combination with function with a In my app, I'm trying to validate critical input with assertions (again not automatic compiler directives) to make sure my algorithms run well. I think you mean that I expect TSC to throw error automatically. After clarifying, would you say it is again TS goals? if yes, please explain. [edit] const a = b as number;So to be clear, when I mention assertion, I mean to check a critical condition that should throw or be handled if condition is not met. |
Probably it's very easy that we add the condition "the target version is older than ES2022." |
Ah, my apologies, I had severely misunderstood what you were asking for. Yes, the problem that we all share in common here is the idea of having any code executing before |
|
Honestly, I don’t see the point in forcing people to have a wrapper function around constructors, or to have long hardly readable expressions as parameter to Do the ECMAScript standard really disallows it? It seems not … |
|
Is there any chance this error can be disabled for ES5? To my knowledge, this only applies to ES6 classes. The following example works in ES5 but not in ES6: My project has a fair amount of legacy code using ES5 standards and we see many of these errors from the compiler, even though the compiled code works as expected. It would be a significant and error-prone lift to fix all the issues. Our legacy code uses Backbone.js which requires some properties to be set before a |
|
We don't turn off semantic rules based on the target, since that creates a giant upgrade trap without people realizing it (as you yourself are noticing). I'm curious: How did you get into this state in the first place? This has been an error in TS approximately (literally?) forever. |
|
Appreciate the response and I understand the dilemma. Here's ours: My team adopted TypeScript fairly early and we upgraded as much as we could until we started seeing this error. We're currently stuck on 1.6. We have a fair amount of legacy code still in use and we lack the resources to make the upgrades necessary to be compliant with newer TypScript versions. We're using Visual Studio as our IDE, which stopped supporting TS 1.x a few major versions ago. The developer experience in our legacy code is about as bad as it gets: broken Intellisense and our MsBuild targets fail to find the resources to even output error messages. We get the same error in the post below but the recommended solution does not work for us. https://forum.ncrunch.net/Default.aspx?g=posts&m=5355 However, our TS code still compiles valid JS with configuration settings of Our legacy code is written in Backbone/Marionette, which uses ES5 style classes so the "this before super()" isn't a problem, though I wouldn't say it's encouraged. There's a long thread about it below. We've been using In the end, I just want to make our development process less painful. We don't really need to upgrade our TS version but it seemed like the path of least resistance so long as we could figure out how to mute this specific error. My initial solution was a script to place Currently, I'm trying to make use of loose-ts-check to mute this specific error. I need to tinker with it a bit more to properly integrate it into our build and development process, but I believe it will provide a better path forward. If you have any suggestions on how to move forward, I gratefully welcome them. |
|
I'm not super familiar with Backbone's API - but if you're willing to, I would create an intermediate non-ES6 class that performs the constructor initialization in a deferred way. So Here's a proof-of-concept in the playground. This wouldn't work in ES2015 output since you'd need to use Anyway, for any classes that need to be refactored, you can use that trick and write a lint rule to enforce that the helper method is actually called. For any classes where you need to access prototype methods before the super helper, you'd need to do some manual tweaking. |
|
Correction: TS was in the wrong. More recently it was me |
|
@dcporter I think you may have misunderstood. The issue is that I do need/want to access @DanielRosenwasser Thank you for your response and code sample. |
|
You're right, I thought your issue was the same one. Cheers, glad you're all set! Has TS changed its policy towards pre-super code in general? Hoping to be wrong twice in one post 😄🤞🏻 |
|
I'd have to read through this thread and the changelog to get a precise idea of the current status, but, yes, you can execute "this-less" code in the |
|
Fantastic news that I should have gotten previously and on my own. Thanks! Updated my OP. |
and others
The TypeScript specification currently reads:
It is reasonable in TypeScript to not permit
thisto be referenced in a constructor before callingsuperwhen there are initialized properties or constructor parameter properties becausethisis not fully initialized until aftersuperis called. But broader restrictions on calling other code beforesuperthat is not directly usingthisdon't seem that helpful and can be worked around anyway. So why keep them?A common use case for having code before a call to
superis to transform constructor parameters in the subclass constructor before passing them to the superclass constructor. If such transformations are complex, a programmer might want to do the transformation step-by-step on multiple lines for increased readability and easier debugging.An example of bypassing the compiler's restriction of no code before
superis just making function calls wrapping arguments to asupercall such assuper(logThisName(name))where the called function refers tothis.As show by an example in the Handbook discussion linked below on improving the explanation for TypeScript constructor restrictions, ES6 permits other code in a constructor before a
supercall (although accessingthisin called code would generate a runtime error before super was called). TypeScript is being more strict than what ES6 permits, and sometimes that is a good thing. But, is there any real value in this case by differing from what ES6 allows overall -- compared to just getting in the way? Why not always always allow code before asupercall when it does not usethis? Does the benefit of not allowing code before asupersometimes really benefit anyone compared to the confusion caused by requiring programmers to use awkward workarounds and to learn a more complex rule for writing constructors than "Don't usethisbefore callingsuper"?This idea was originally brought up in issue #945 (closed in October 2014). I am creating a new issue for that as discussed with @mhegazy here: microsoft/TypeScript-Handbook#214. There is a code example in that Handbook issue which can be used for testing the current behavior for TypeScript, Babel, and ES6.
The text was updated successfully, but these errors were encountered: