Skip to content

Change assertion failure to unreachable statement #1205

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

Closed
wants to merge 4 commits into from
Closed

Change assertion failure to unreachable statement #1205

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ed746c
Change assertion failure to unreachable statement
DuncanUszkay1 Apr 1, 2020
842becc
whitespace
DuncanUszkay1 Apr 7, 2020
59f618b
Move unreachable into top level statement compilation
DuncanUszkay1 May 6, 2020
d2d2017
only set compiled flag upon successful compilation for globals
DuncanUszkay1 May 6, 2020
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
4 changes: 3 additions & 1 deletion src/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7915,7 +7915,9 @@ export class Compiler extends DiagnosticEmitter {
return module.unreachable();
}
let globalType = global.type;
assert(globalType != Type.void);
if(globalType == Type.void) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing a whitespace here, and wondering if the fix should instead make compileGlobal above return false. Might have unforeseen consequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't compile regardless- since a global resolving into a void is already an error:

if (resolvedType == Type.void) {
          this.error(
            DiagnosticCode.Type_expected,
            typeNode.range
          );
          return false;
        }

Given that, this is really just a question of what gets displayed when it does fail. I suppose the worst case here is that there is some edge case where this will append additional errors which don't make sense to the error which the dev needs to see, but that doesn't seem like huge problem.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that this requires a fix, just questioning the chosen location. If compileGlobal would return false if it failed to compile that same global earlier already, which gives us an unreachable as well, this would be much clearer than checking for Type.void.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compileGlobal does in fact return false for the first statement- and that false was being ignored by compileTopLevelStatement. If I move the unreachable call there it will also work:

if (!this.compileGlobal(<Global>element)) this.module.unreachable();

Most other calls to compileGlobal do the same, checking its result and then returning unreachable if it failed. Is this what you mean?

For this to work however I'd still need to delete the globalType != void assertion, otherwise none of these errors will ever print.

Copy link
Member

@dcodeIO dcodeIO Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Most other calls to compileGlobal do the same, checking its result and then returning unreachable if it failed. Is this what you mean?

Yeah, plus that it currently seems that only the first call to compileGlobal for the same global returns false if compilation fails currently, while subsequent calls see that it is already CommonFlags.COMPILED and return true. I think we should fix that, instead of adding checks for Type.void that are only necessary due to a more general problem.

Copy link
Contributor Author

@DuncanUszkay1 DuncanUszkay1 May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Updated to move the unreachable call- looking into a more general fix as well

return module.unreachable();
}
if (global.is(CommonFlags.INLINED)) {
return this.compileInlineConstant(global, contextualType, constraints);
}
Expand Down