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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Fix compiler crash when certain operators used on types #2 #2293

Closed
wants to merge 4 commits into from
Closed

Fix compiler crash when certain operators used on types #2 #2293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad87290
fix
romdotdog May 12, 2022
44e6842
disable eslint for test
romdotdog May 12, 2022
037121d
fix error when `contextualType` is not `void`
romdotdog May 12, 2022
5943e06
make the test look better :shipit:
romdotdog May 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions src/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9237,13 +9237,10 @@ export class Compiler extends DiagnosticEmitter {
// make a getter for the expression (also obtains the type)
var getValue = this.compileExpression( // reports
expression.operand,
contextualType.exceptVoid,
contextualType,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@dcodeIO dcodeIO May 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@romdotdog romdotdog May 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@romdotdog romdotdog May 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Constraints.NONE
);

// shortcut if compiling the getter already failed
if (getExpressionId(getValue) == ExpressionId.Unreachable) return getValue;

// if the value isn't dropped, a temp. local is required to remember the original value,
// except if a static overload is found, which reverses the use of a temp. (see below)
var tempLocal: Local | null = null;
Expand Down Expand Up @@ -9450,6 +9447,17 @@ export class Compiler extends DiagnosticEmitter {
return module.unreachable();
}

// check assignability
var targetType = resolver.getTypeOfElement(target);
if (!targetType) targetType = Type.void;
if (!this.currentType.isStrictlyAssignableTo(targetType)) {
this.error(
DiagnosticCode.Type_0_is_not_assignable_to_type_1,
expression.range, this.currentType.toString(), targetType.toString()
);
return module.unreachable();
}

// simplify if dropped anyway
if (!tempLocal) {
return this.makeAssignment(
Expand Down Expand Up @@ -9597,7 +9605,7 @@ export class Compiler extends DiagnosticEmitter {
compound = true;
expr = this.compileExpression(
expression.operand,
contextualType.exceptVoid,
contextualType,
Constraints.NONE
);

Expand Down Expand Up @@ -9668,7 +9676,7 @@ export class Compiler extends DiagnosticEmitter {
compound = true;
expr = this.compileExpression(
expression.operand,
contextualType.exceptVoid,
contextualType,
Constraints.NONE
);

Expand Down Expand Up @@ -9837,6 +9845,18 @@ export class Compiler extends DiagnosticEmitter {
var resolver = this.resolver;
var target = resolver.lookupExpression(expression.operand, this.currentFlow);
if (!target) return module.unreachable();

// check assignability
var targetType = resolver.getTypeOfElement(target);
if (!targetType) targetType = Type.void;
if (!this.currentType.isStrictlyAssignableTo(targetType)) {
this.error(
DiagnosticCode.Type_0_is_not_assignable_to_type_1,
expression.range, this.currentType.toString(), targetType.toString()
);
return module.unreachable();
}

return this.makeAssignment(
target,
expr,
Expand Down
53 changes: 53 additions & 0 deletions tests/compiler/increment-error.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
{
"asc_flags": [
],
"stderr": [
"AS234: Expression does not compile to a value at runtime.", "++Foo;",
"TS2469: The '++' operator cannot be applied to type 'void'.", "++Foo;",
"AS234: Expression does not compile to a value at runtime.", "Foo += 1;",
"TS2469: The '+' operator cannot be applied to type 'void'.", "Foo += 1;",
"AS234: Expression does not compile to a value at runtime.", "Foo--;",
"TS2469: The '--' operator cannot be applied to type 'void'.", "Foo--;",
"AS234: Expression does not compile to a value at runtime.", "--Foo;",
"TS2469: The '--' operator cannot be applied to type 'void'.", "--Foo;",
"AS234: Expression does not compile to a value at runtime.", "Foo -= 1;",
"TS2469: The '-' operator cannot be applied to type 'void'.", "Foo -= 1;",
"AS234: Expression does not compile to a value at runtime.", "Array++;",
"TS2469: The '++' operator cannot be applied to type 'void'.", "Array++;",
"AS234: Expression does not compile to a value at runtime.", "++Array;",
"TS2469: The '++' operator cannot be applied to type 'void'.", "++Array;",
"AS234: Expression does not compile to a value at runtime.", "Array += 1;",
"TS2469: The '+' operator cannot be applied to type 'void'.", "Array += 1;",
"AS234: Expression does not compile to a value at runtime.", "Array--;",
"TS2469: The '--' operator cannot be applied to type 'void'.", "Array--;",
"AS234: Expression does not compile to a value at runtime.", "--Array;",
"TS2469: The '--' operator cannot be applied to type 'void'.", "--Array;",
"AS234: Expression does not compile to a value at runtime.", "Array -= 1;",
"TS2469: The '-' operator cannot be applied to type 'void'.", "Array -= 1;",
"AS234: Expression does not compile to a value at runtime.", "const a = (Foo++);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const a = (Foo++);",
"AS234: Expression does not compile to a value at runtime.", "const b = (++Foo);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const b = (++Foo);",
"AS234: Expression does not compile to a value at runtime.", "const c = (Foo += 1);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const c = (Foo += 1);",
"AS234: Expression does not compile to a value at runtime.", "const d = (Foo--);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const d = (Foo--);",
"AS234: Expression does not compile to a value at runtime.", "const e = (--Foo);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const e = (--Foo);",
"AS234: Expression does not compile to a value at runtime.", "const f = (Foo -= 1);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const f = (Foo -= 1);",
"AS234: Expression does not compile to a value at runtime.", "const g = (Array++);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const g = (Array++);",
"AS234: Expression does not compile to a value at runtime.", "const h = (++Array);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const h = (++Array);",
"AS234: Expression does not compile to a value at runtime.", "const i = (Array += 1);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const i = (Array += 1);",
"AS234: Expression does not compile to a value at runtime.", "const j = (Array--);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const j = (Array--);",
"AS234: Expression does not compile to a value at runtime.", "const k = (--Array);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const k = (--Array);",
"AS234: Expression does not compile to a value at runtime.", "const l = (Array -= 1);",
"TS2322: Type 'i32' is not assignable to type 'void'.", "const l = (Array -= 1);",
"EOF"
]
}
39 changes: 39 additions & 0 deletions tests/compiler/increment-error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/* eslint-disable no-class-assign */
/* eslint-disable no-global-assign */

class Foo {}

Foo++;
++Foo;
Foo += 1;

Foo--;
--Foo;
Foo -= 1;

Array++;
++Array;
Array += 1;

Array--;
--Array;
Array -= 1;

const a = (Foo++);
const b = (++Foo);
const c = (Foo += 1);

const d = (Foo--);
const e = (--Foo);
const f = (Foo -= 1);


const g = (Array++);
const h = (++Array);
const i = (Array += 1);

const j = (Array--);
const k = (--Array);
const l = (Array -= 1);

ERROR("EOF");