fix: Fix assert when calling a setter property

[saulecabrera]

Emits an error diagnostic when calling a setter property. This case should be treated in the same way getters are treated. The issue is reproducible with the following code:

class B {
  set prop(a: i32) {}
}

export function add(): void {
  // Actual: assert
  // Expected: error diagnostic specifying that this expression is not callable. 
  (new B()).prop(1);
}

I emitted the diagnostic early, I'm assuming that there's no point in compiling the expression when this case is met as the setter expression is compiled when an assignment expression is found (might be wrong about this).

• I've read the contributing guidelines

[MaxGraey]

I guess need to do the same for getter as well?

[dcodeIO]

Iirc it is allowed to call a getter as a function, and the error seen here is rather because of the property not having a getter (which is unexpected), not because it has a setter.

[MaxGraey]

I mean this

class B {
  set prop(a: i32) {}
  get prop(): i32 { return 1 }
}

export function add(): void {
  (new B()).prop(1); // should be error
  (new B()).prop();  // should be error as well
}

[saulecabrera]

I mean this

class B {
  set prop(a: i32) {}
  get prop(): i32 { return 1 }
}

export function add(): void {
  (new B()).prop(1); // should be error
  (new B()).prop();  // should be error as well
}

@MaxGraey It already does.

[saulecabrera]

@dcodeIO

because of the property not having a getter (which is unexpected), not because it has a setter.

Having only either or is valid AFAIK. But I see your point, this is that part that I was a bit unsure about, this should error only if there's a setter and no getter.

[saulecabrera]

Just verified, and you were right:

• When both set and get are present, the diagnostics are emitted correctly
• When only the getter is present, the diagnostic is emitted correctly
• When only the setter is present, the crash occurs.

I've amended the change to fix only the last condition.

[dcodeIO]

Seems like only the presence of a getter is relevant here (would not call a setter anyway), so this can perhaps be simplified to

let getterInstance = propertyInstance.getterInstance;
if (!getterInstance) {
  this.error(
    DiagnosticCode.Cannot_invoke_an_expression_whose_type_lacks_a_call_signature_Type_0_has_no_compatible_call_signatures,
    expression.range, this.currentType.toString()
  );
  return module.unreachable();
}

i.e. replacing the assert with an if.

[dcodeIO]

Also not sure about this.currentType here. Seems like a case for getTypeOfElement on target rather, which may return null, so if it does better printing a "is not represented by a type" kind of error.

[saulecabrera]

Yeah, this works too.

[dcodeIO]

I think my comment on this.currentType may have got lost somehow (no problem), or is this not applicable?

[saulecabrera]

I think my comment on this.currentType may have got lost somehow (no problem), or is this not applicable?

After looking at compileExpression indeed, it sets this.currentType to the contextual type passed in, which ensures that the diagnostic reported after (around line 6359) has the accurate type.

So yeah I think that calling resolver.getTypeOfElement(target) is the safest route.

if it does better printing a "is not represented by a type" kind of error.

Just to make sure that we are on the same page, with this, do you mean using DiagnosticCode.Expression_cannot_be_represented_by_a_type ?

[saulecabrera]

Yeah, good point, FWIW it was introduced here https://github.com/AssemblyScript/assemblyscript/pull/864/files, and removing the assert and re-running the tests locally doesn't break anything.

[saulecabrera]

After digging deeper into this, I believe I finally found the root of the void assertion.

First of all, I couldn't track down a case in which the type here would be void, effectively all typedElements which can be any of: EnumValue, Property, Global, Field, Local, Index Signature, Type definition, Function, Class and Enum are properly resolved when this method is called; with the linked PR above, there was an exception with lazy globals, in which if the global wasn't typed (for whatever reason) the expression wouldn't be resolved, potentially leaving the type property of the TypedElement class as void, but that case was correctly fixed by the PR above. I'm my latest change I've removed the void assertion and returned to use resolver#getTypeOfElement.

---

@MaxGraey do you have any kind of context in this part of the compiler so that you can review?

[MaxGraey]

@MaxGraey do you have any kind of context in this part of the compiler so that you can review?

No, so I'll need time to investigate of related parts

[saulecabrera]

Makes sense, thanks!

[MaxGraey]

@saulecabrera So it fix issue (assert) occurring only when the setter is present and add more proper diagnostic error. It looks legit for me.