JS Constructor Attr Support

[ambiguousname]

Fixes #718.

Current TODOs:

• Add consistent functions that can always be called (fromFields, fromValue, etc.) for when the default constructor is overridden.
• Hide private functions from .d.ts files
• Add test for overriding constructor for Enums (and calling fromValue)
• Ensure passing tests
• Update demo_gen to match
• Fix whitespacing
• See if usage is removable (I think this can be avoided by just returning a new constructed object)
• Avoid using symbol parameters (and ...args) in constructor functions so those inspecting source can see what the default constructor takes for params

[Manishearth]

Overall this seems good but I really don't think we should ever be sharing codegen between constructors and regular JS methods: a codegenned method should be one or the other.

The reason is that constructor has some subtle behavior: in a constructor, you either mutate this, or you return an object: If what you return is an Object (any object), that is the construction result.

We need to go through a constructor eventually since JS does not provide a good way to make objects with the right typeof without using a constructor.

What we should have is:

• internalConstructor: A non-static method that takes this and returns an object. This method should mutate this if necessary, and always return this or another object of the right type:
  • For opaques: set up lifetimes, and set up this.ptr. Return this
  • For structs: set subfields. Return this.
  • For enums: either return a cached object, the provided object, or (internal-only) set this.value and return this
• constructor: A thin wrapper that returns this.internalConstructor() (when no default ctor), or something calls the default ctor / the internal ctor depending on the value of the first argument.
• fromFields/fromValue: Thin wrappers around new Foo(exposeConstructor?, ...)
• fromFfi: We don't have this anymore, but this is just new Foo(internalConstructor, ...) and callsites already do that.

I don't think returning this is strictly always necessary: for example the struct setup works fine without it, but it might be better from a hygeine point of view? Open to other ideas. The nice thing about this design is that the codegenned constructor can also be identical in that case.

Thoughts?

[ambiguousname]

I think all of your suggestions sound good to avoid future headaches.

I believe what you're describing for #internalConstructor ONLY mutating this is very similar to what constructors looked like when I needed a usage : Option<SpecialMethod> parameter in gen_c_to_js_for_type to determine whether we were interested in modifying this or returning a new object (based on whether or not usage was Some(SpecialMethod::Constructor) or not)

If you're curious as to what that looked like, the commit hash was a70e7005d2b3f295926b4f641efb96a9bdbf3f7a

When I have some time, I'll go about changing all #internalConstructor methods to only modify this, instead of returning a new object.

[ambiguousname]

On further reading though, I think I may be missing your point. My understanding now is that you'd like to completely remove any form of sharing generation logic for any method that is meant to be a default constructor. So we might move all that logic from generate_method to an entirely new function, like say generate_constructor_method? Do I have that right?

[Manishearth]

My understanding now is that you'd like to completely remove any form of sharing generation logic for any method that is meant to be a default constructor

No, that part is okay. What I don't want is for us to ever have the situation we have now where a piece of the codegen template generates either a regular function or a constructor, as it does now: we shouldn't have {% if foo %} constructor {% else %} methodName {% endif %}.

We should instead generate #internalConstructor, optionally #defaultConstructor (which will internally call constructor with the first argument being diplomatRuntime.internalConstructor), and constructor which dispatches to one of the two based on what the first argument is.

When I have some time, I'll go about changing all #internalConstructor methods to only modify this, instead of returning a new object

They will still have to return an object at least for the enum case, because we cache enum objects. I don't believe this = foo works.

They have to both mutate this and return either this or an object of the same type, and the constructor() should call return this.#internalConstructor().

[ambiguousname]

Ah, I believe I understand your point now. I'll start by moving the generation for constructor into one place (Maybe js_class.js.jinja?) and work on building out logic for calling #internalConstructor and #defaultConstructor from there.

[ambiguousname]

@Manishearth I moved logic for generating constructors to js_class.js.jinja (and as a consequence of that, removed all the logic from special_methods.js.jinja to put all common generation between templates in one place). I also stripped out a lot of the conditionals for making constructor functions from each of the derived files, so as to make js_class.js.jinja the one source for generating constructor.

Let me know if this matches what you had in mind!

[Manishearth]

praise: nice work!!

[Manishearth]

thought (followup): we should probably move shared types into a shared ImplTemplate type, and have specific StructImplTemplate/etc