Fix compiler crash when certain operators used on types #2

[romdotdog]

The issue

When the prefix compound unary operators (++x and --x) are used on types, the compiler crashes
Example

▌ Whoops, the AssemblyScript compiler has crashed during compile :-(
▌ 
▌ Here is the stack trace hinting at the problem, perhaps it's useful?
▌ 
▌ /data/assemblyscript/std/portable/index.js:200
▌     throw new AssertionError(message);
▌           ^
▌ 
▌ AssertionError: assertion failed
▌     at null.q.assert (/data/assemblyscript/std/portable/index.js:200:11)
▌     at oi.makeAssignment (/data/assemblyscript/src/compiler.ts:6057:5)
▌     at oi.compileUnaryPrefixExpression (/data/assemblyscript/src/compiler.ts:9840:17)
▌     at oi.compileExpression (/data/assemblyscript/src/compiler.ts:3465:21)
▌     at oi.compileExpressionStatement (/data/assemblyscript/src/compiler.ts:2447:17)
▌     at oi.compileStatement (/data/assemblyscript/src/compiler.ts:2135:21)
▌     at oi.compileTopLevelStatement (/data/assemblyscript/src/compiler.ts:2097:25)
▌     at oi.compileFile (/data/assemblyscript/src/compiler.ts:1013:12)
▌     at oi.compile (/data/assemblyscript/src/compiler.ts:497:14)
▌     at Module.jS (/data/assemblyscript/src/index-wasm.ts:325:32)
▌     at Object.Ue (/data/assemblyscript/cli/index.js:715:31)
▌     at async runTest (file:///data/assemblyscript/tests/compiler.js:194:23)
▌     at async file:///data/assemblyscript/tests/compiler.js:565:31
▌ 
▌ If you see where the error is, feel free to send us a pull request. If not,
▌ please let us know: https://github.com/AssemblyScript/assemblyscript/issues
▌ 
▌ Thank you!

Solution

Purely removing .exceptVoid seems to work, though I'm not sure if it's the right way to go. I haven't been able to completely parse what .exceptVoid even is, but it seems to have no place in the functionality of an operator that cannot work on the void type anyway.

• I've read the contributing guidelines
• I've added my name and email to the NOTICE file

[dcodeIO]

Iirc, .exceptVoid makes sure that if contextualType is somehow void (could be a result of a previous error), that compileExpression here doesn't produce a drop, an expression that is never, say, assignable to a local, which would lead to validation errors in Binaryen. A way to test whether this leads to problems with the change could be a test for ++someFunctionReturningVoid().

[romdotdog]

ERROR TS2357: The operand of an increment or decrement operator must be a variable or a property access.

   ++foo();
     ~~~~~
   in increment-error.ts(7,3)

  FAILURE 1 parse error(s)

[dcodeIO]

Ok, that's already good. Then I wonder if there is still a way to produce a void type using a variable or property access, perhaps via a previous error. The general idea is that we don't want Binaryen to blow up but to diagnose every possible error ourselves. Do you know why exactly this is blowing up with .exceptVoid in place?

[romdotdog]

Yes, because there is a missing check. I am adding a new commit right now.

[dcodeIO]

Taking a step back, am I assuming correctly that the use of .exceptVoid prevents a fix in makeAssignment? If so, what is target.kind in makeAssignment with .exceptVoid in place and does commenting out the assert being hit still produce the expected diagnostic?

[dcodeIO]

Hmm, what about removing the last assert in makeAssignment and add a default case?

+      default: {
+        this.error(
+          DiagnosticCode.The_target_of_an_assignment_must_be_a_variable_or_a_property_access,
+          valueExpression.range
+        );
+      }
    }
-   assert(false);
    return module.unreachable();

  ERROR AS234: Expression does not compile to a value at runtime.

   ++String;
     ~~~~~~

  ERROR TS2541: The target of an assignment must be a variable or a property access.

   ++String;
     ~~~~~~

Essentially I am not sure about the .exceptVoid change as it seems unrelated. If something goes wrong when compiling with .exceptVoid, this would already produce an error before the resulting compiled expression is returned (here the case: AS234, just not diagnosed because an assert is hit before diagnostics are printed), with the compiled expression not really mattering anymore, except that it cannot be a drop, which .exceptVoid prevents. The alternative is ofc to add a bunch of late checks, but that seems good to avoid.

Unfortunately, though, my modification only works for ++String from the initial example, but not ++i32, but the latter is probably a separate bug related to i32(...) being a builtin function.

[romdotdog]

Why doesn't Foo += 1 use .exceptVoid? My changes directly reflect the former's implementation; they are, in essence, the same thing. Do all of those compound operators need changing?

[dcodeIO]

My concern is mostly that .exceptVoid seems unrelated in this PR, so that the fix can be simpler (no additional late checks). It might be the case that .exceptVoid can be removed, or it might not be the case, but I'm hesitant to make this call here.

[romdotdog]

Do you insist on this asymmetry? If Foo can be void, then what happens when there is Foo += 1 instead of Foo++? I lean towards either removing .exceptVoid or changing the implementation of all compound operators. I'm fine with either of the aforementioned.

[dcodeIO]

I'd be fine to thoroughly investigate the asymmetry separately I think. If Foo +=1 does not use .exceptVoid, it seems that it should to be sure. Other than that it doesn't seem necessary to remove .exceptVoid as it serves a purpose, even if the issue it prevents is untypical.

[romdotdog]

Are the errors in this test satisfactory? The primary aim of this PR is to unite the behaviors of Foo++ and Foo += 1 without changing the latter.