oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
71.78k stars 2.55k forks source link

TypeScript generated super call not passing arguments #2060

Open DavidPesta opened 1 year ago

DavidPesta commented 1 year ago

What version of Bun is running?

0.5.1

What platform is your computer?

Linux 5.15.0-60-generic x86_64 x86_64

What steps can reproduce the bug?

class Base {
  constructor(data: any) {
    Object.assign(this, data);
  }
}

class Outer extends Base {
  stuff: string;
  things: number;
  extra: Inner;
}

class Inner extends Base {
  more: string;
  greatness: boolean;
}

let outer = new Outer({
  stuff: "Hello World",
  things: 42,
  extra: new Inner({
    more: "Bun is becoming great!",
    greatness: true
  })
});

console.log(outer);

What is the expected behavior?

{
  stuff: "Hello World",
  things: 42,
  extra: {
    more: "Bun is becoming great!",
    greatness: true
  }
}

What do you see instead?

{
  stuff: undefined,
  things: undefined,
  extra: undefined
}

Additional information

It works when you run the on an online REPL, click Run on this page: https://www.typescriptlang.org/play?#code/MYGwhgzhAEBCkFNoG8BQBIYB7AdhALgE4Cuw+WhAFACZj5gBc0YOAngJQoboDyARgCsEZAHSQIASwDmOSvgAWEiABpoteuwDcGAL6o9qUOOg9i+BIWgIAHuZzUY8CEjToCxAGYemBQhJxS2ugK-lIQTDjEALZ8FkE2RIzQAJI4OHH6qIbgUClpFla2CPaOiFzoURQIPkShQVKECHTpUEx8WFggTTjaBl340FhmBQC80OkA7ibDVK7uXkwARAASCCAgWNAA6hQg1IvKGCEB4dAALABMh+gJhEmTeemz3JWNS7DEONBK0LHYUaFoA0mvgAIQHbjA5oIVrQIjEBC6dj6LRZbB4ToIEQbKSUIbmQiooA

It works in Bun when the constructors are not inherited:

class Outer {
  stuff: string;
  things: number;
  extra: Inner;

  constructor(data: any) {
    Object.assign(this, data);
  }
}

class Inner {
  more: string;
  greatness: boolean;

  constructor(data: any) {
    Object.assign(this, data);
  }
}

let outer = new Outer({
  stuff: "Hello World",
  things: 42,
  extra: new Inner({
    more: "Bun is becoming great!",
    greatness: true
  })
});

console.log(outer);

The above gives me:

{
  stuff: "Hello World",
  things: 42,
  extra: {
    more: "Bun is becoming great!",
    greatness: true
  }
}

So it just doesn't work when the constructor is inherited.

ThatOneBro commented 1 year ago

I believe this was fixed in https://github.com/oven-sh/bun/pull/1883 which made it into version 0.5.2, can you try running bun upgrade in order to get the latest version and try again?

DavidPesta commented 1 year ago

I did as you said and the problem persists.

I performed bun upgrade and I watched the upgrade. Then I performed bun -v and I get 0.5.6 as a result. Then I run the script again and I get this:

{
  stuff: undefined,
  things: undefined,
  extra: undefined
}
ThatOneBro commented 1 year ago

Sorry about that, you're right. This is a different issue.

However, I looked into it and it looks like Chrome / Node both have the same behavior for this piece of code.

I need to look into why it happens this way, I'm guessing it has to do with the subclass prop declaration shadowing the values of the super?

Here's what happens in Chrome:

Screen Shot 2023-02-13 at 9 35 36 AM

And if you move the prop declaration to the parent:

Screen Shot 2023-02-13 at 9 41 01 AM

It works.

And if you console.log() in the constructor of Base, you'll see what is happening. The values are set properly, but in the outer scope, after the super returns and I'm guessing Outer is finalized, the values are erased -- which would correspond to the prop declarations in the Outer class signature, which default to undefined.

Screen Shot 2023-02-13 at 9 47 26 AM

I think this is more of an ECMAScript issue than a Bun issue, but if you think I made a mistake let me know

DavidPesta commented 1 year ago

That is unfortunate. At least someone's implementation of TypeScript accounts for this and causes this to work. Try "Run" in this online REPL and you'll see it works there:

https://www.typescriptlang.org/play?#code/MYGwhgzhAEBCkFNoG8BQBIYB7AdhALgE4Cuw+WhAFACZj5gBc0YOAngJQoboDyARgCsEZAHSQIASwDmOSvgAWEiABpoteuwDcGAL6o9qUOOg9i+BIWgIAHuZzUY8CEjToCxAGYemBQhJxS2ugK-lIQTDjEALZ8FkE2RIzQAJI4OHH6qIbgUClpFla2CPaOiFzoURQIPkShQVKECHTpUEx8WFggTTjaBl340FhmBQC80OkA7ibDVK7uXkwARAASCCAgWNAA6hQg1IvKGCEB4dAALABMh+gJhEmTeemz3JWNS7DEONBK0LHYUaFoA0mvgAIQHbjA5oIVrQIjEBC6dj6LRZbB4ToIEQbKSUIbmQiooA

DavidPesta commented 1 year ago

If that came across as abrasive, many apologies. No abrasiveness intended.

ThatOneBro commented 1 year ago

No not in the slightest! I totally get it, this is kinda frustrating.

I think this has to do with transpilation targets. I initially transpiled the source with esbuild with esnext as the target, which gives a very rudimentary translation to ECMAScript classes. Running the same code through tsc (which defaults to es3 target) gave this:

Screen Shot 2023-02-13 at 11 25 59 AM

which works as intended:

Screen Shot 2023-02-13 at 11 22 54 AM

By default I think in Bun we are transpiling to esnext internally at runtime, but I could be wrong.

This is a bit out of my field so I'll see what @Jarred-Sumner or @dylan-conway has to say about this

DavidPesta commented 1 year ago

Golly! I hope this is something that could be supported. I am hoping to be able to implement a pattern of simple model classes (as shown) that inherit from a Persistence layer, which provides constructor and other supporting methods that remain out of sight, so that those simple model classes can be instantiated with data and passed around a system and seamlessly get saved to disk, etc.

Worst case I find myself writing a language that transpiles to a language that transpiles to a language. :sob:

ThatOneBro commented 1 year ago

As a temporary workaround, you could transpile to es3 or a low-enough transpilation target yourself and pipe it into bun, kinda like how ts-node works.

Edit: Actually I'm not sure if we can pipe code directly through stdin yet, I need to verify that. But you could emit JS and then run the output file directly in bun for now.

DavidPesta commented 1 year ago

As a temporary workaround that would be great. Although, whether this gets a permanent fix at some level would affect my approach. I'm not sure I want to do what never becomes a standard. I just don't have enough insight into behind the scenes to predict how all this will likely play out.

For example...

Would ES see this as a problem that they definitely need to fix for TypeScript to be able to then be fixed? Is there some different critical issue that recent ES needed to fix that logically prevents ES from being able to fix this?

What is your sense on this?

ThatOneBro commented 1 year ago

Would ES see this as a problem that they definitely need to fix for TypeScript to be able to then be fixed? Is there some different critical issue that recent ES needed to fix that logically prevents ES from being able to fix this?

I'm not sure if this is necessarily a bug from the perspective of ES, I can see it being part of the philosophy that each subclass should be responsible for their own members. You see that with class private methods, you can't call a subclass private method from a superclass, as to make them truly private. Though I'm not 100% on that.

It does suck that it deviates from what is expected from the perspective of a TS user though. We'll have to think about how to address this

ThatOneBro commented 1 year ago

We could do some smart hacks where we automatically move prop declarations in the transpiled JS to the superclass whenever no constructors are present on the subclasses. That actually might work... curious to see what others think about that

Jarred-Sumner commented 1 year ago

We will make it follow TypeScript's behavior for TypeScript files and not modify JavaScript's behavior for JavaScript files

I suspect this is related to differences between how TypeScript transpiles classes versus JavaScript. There is a setting in tsconfig.json that is default false in tsc which Bun pretends is default true and that is likely the cause of this bug.

Bun's behavior is correct for modern JavaScript, this bug has to do with legacy default behavior in TypeScript which Bun should implement

DavidPesta commented 1 year ago

Many thanks for the great responses to address this. You all are giving me a lot of hope! :grinning:

paperdave commented 3 weeks ago

tsconfig.json's target compiler ooption defaults to ES5 or ES3, which makes this emit function declarations by the default configuration.

Outer is compiled this way by TSC, which completely removes the uninitialized fields.

// tsc index.ts
var Outer = /** @class */ (function (_super) {
    __extends(Outer, _super);
    function Outer() {
        return _super !== null && _super.apply(this, arguments) || this;
    }
    return Outer;
}(Base));

When using modern tsc settings, mainly setting target to anything past ES5, this compiles to a class without types:

class Outer extends Base {
    stuff;
    things;
    extra;
}

This looks like these fields do not do anything by default, but since a field without an initializer is sugar for stuff = undefined; these have different behavior. to illustrate what is happening, when calling new Outer, the following happens in this order

step 2 of that is not done in typescript's ES5 class lowering, which i believe predated JavaScript class fields, hence an implementation difference.

Bun does not do this lowering yet. Until Bun implements this syntax transform, there is one alternative: add declare to your TS Class fields to prevent a compiler from actually initializing them to anything.

 class Outer extends Base {
-  stuff: string;
+  declare stuff: string;
-  things: number;
+  declare things: number;
-  extra: Inner;
+  declare extra: Inner;
}

This makes tsc --target ESNext, bun, esbuild, and other tools transform this to just class Outer extends Base {}