Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot create optional constructor argument with non-nullable type #1735

Closed
bbugh opened this issue Mar 17, 2021 · 8 comments
Closed

Cannot create optional constructor argument with non-nullable type #1735

bbugh opened this issue Mar 17, 2021 · 8 comments
Labels

Comments

@bbugh
Copy link

bbugh commented Mar 17, 2021

Hi! 馃憢 This project is AWESOME. I love what you're doing here, it's nice to not have to jump over to Rust to build some stuff for TS/JS.

It appears that AssemblyScript cannot support an optional constructor argument that is a non-nullable built-in type. We are replacing some performance-critical code and would like this to be a drop-in replacement, and in order to do so, the constructor must be able to be called either as new Object() (null default value) or new Object(value: number/f64) (i.e. optional first argument). I cannot seem to find a way to make this work. I have looked through the documentation and issues and couldn't find anything about it.

I'm not sure if this is a bug, expected behavior, or a documentation improvement.

Here are the attempts:

ERROR AS215: Optional parameter must have an initializer.

   constructor(value?: f64) {
               ~~~~~~~~~~~

That's unexpected. Ok, let's try with an initializer.

ERROR TS1015: Parameter cannot have question mark and initializer.

   constructor(value?: f64 = null) {
               ~~~~~

It seems like optional parameters are not supported? Ok, I will try setting just the initializer:

WARNING AS226: Expression resolves to unusual type 'usize'.

   constructor(value: f64 = null) {
                            ~~~~

Ok, how about a union with null?

ERROR AS204: Type 'f64' cannot be nullable.

   constructor(value: f64 | null = null) {
                      ~~~
 in assembly/index.ts(11,22)

WARNING AS226: Expression resolves to unusual type 'usize'.

   constructor(value: f64 | null = null) {
                                   ~~~~

It can't be an optional parameter without an initializer, but it can't be optional if it has an initializer, and it can't be nullable. 馃お

Maybe related to #746?

@saulecabrera
Copy link
Contributor

saulecabrera commented Mar 17, 2021

From my experience this stems from the fact that f64 can't be nullable, that said, I've used the following pattern as a workaround in the past:

export class Foo {
  field: f64;

  constructor(theField: f64 = 0) {
    this.field = theField;
  }
}

export const foo1 = new Foo();
export const foo2 = new Foo(1);

assert(foo1.field == 0);
assert(foo2.field == 1);

The main disadvantage here is that the optional representation is not accurate as 0 might be a valid, non-optional value depending on your use case.


On the other hand, if your optional field can be null, then you have better possibilities:

export class Foo {
  field: string | null; // or field?: string

  constructor(theField: string | null = null) {
    this.field = theField;
  }
}

export const foo1 = new Foo();
export const foo2 = new Foo("foo");

assert(foo1.field == null);
assert(foo2.field == "foo");

@MaxGraey
Copy link
Member

You could use NaN at least for floating points for indicate this value is nullable:

export class Foo {
  constructor(public value: f64 = NaN) {}
}

let foo = new Foo;
let is_null = isNaN(foo.value);

@saulecabrera
Copy link
Contributor

NaN is an option too depending on what you're trying to achieve. I wouldn't recommend it overall, as it is weird to have it as a default, and depending on the use case it might not be semantically correct. For example, if you're exposing this as a library to be consumed by others. To bridge this gap I've also been using Box<T>. Then you can wrap primitives in this class and treat them as nullable:

export class Box<T> {
  value: T;

  constructor(value: T) {
    this.value = value;
  }
}
class Foo {
  field: Box<f64> | null;
}

@saulecabrera
Copy link
Contributor

documentation improvement.

This can definitely be clarified with more snippets, there's a list of some of them here AssemblyScript/website#23 which includes optional calls.

@bbugh
Copy link
Author

bbugh commented Mar 17, 2021

Thanks for all of the ideas! NaN is probably the most useful one in our case, as 0 is a commonly used value (and we can't reasonably predict any placeholder values).

Unfortunately to make this a drop-in, the constructor also has to receive strings, which I haven't really tried yet.

I also tried using a generic like

export class Thing<T = f64> {
  constructor(value: T | null) {
    if (isInteger(value) || isFloat(value)) {
      this.fromNumber(value);
    } else if (isString(value)) {
      this.fromString(value)
    }
  }
}

Which I had hoped would work fine, but that exhibits the same problem as "f64 cannot be null". Oof.

It seems like maybe this isn't possible as a drop-in with wasm and we'll need to add some static factory functions to the class to read it from numbers, strings, etc.

@MaxGraey
Copy link
Member

Just remove value: T | null and use value: T

@bbugh
Copy link
Author

bbugh commented Mar 17, 2021

@MaxGraey thanks, but that would require an argument to be passed, and we need Thing() to work.

I think we're going to go ahead with separating the string input into a fromString static function on the class, and use f64 as the default value. It will take some refactoring but it's probably better not to overload the constructor anyway.

export class Thing {
  private _value: f64 = 0;

  public get value(): f64 {
    return this._value
  }

  constructor(value: f64 = 0) {
    this._value = value
  }

  // simplified example
  public static fromString(value: string): Thing {
    const parsed = parseFloat(value)
    if (isNaN(parsed)) {
      return new Thing()
    }

    return new Thing(parsed)
  }
}

I think this will be fine until such time when/if #816 or #1548 drops

@github-actions
Copy link

github-actions bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jun 2, 2021
@github-actions github-actions bot closed this as completed Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants