Change assertion failure to unreachable statement

[DuncanUszkay1]

Issue: #1165

Right now if you have a global which fails to resolve its own type, and is also used in another constant, you get an assertion failure which prevents any errors from being printed to the console.

This PR just returns unreachable instead of panicking, allowing error messages to be printed.

Example:

const INNER: fake = 1;
const OUTER: i32 = SOME;

Before: AssertionFailure with stack trace
After:

ERROR TS2304: Cannot find name 'fake'.

 const INNER: fake = 1;
              ~~~~
 in main.ts(5,14)

ERROR: 1 compile error(s)
    at Object.main (/Users/duncanuszkay/src/github.com/Shopify/as-fork/assemblyscript/cli/asc.js:614:21)
    at /Users/duncanuszkay/src/github.com/Shopify/as-fork/assemblyscript/bin/asc:21:47

I don't think I need to add an error to the line where the assertion fails- there's not much useful information here since they didn't set their type to void, it's really just an internal thing. AFAIK there's no way to make this global void without causing another error, so the dev should always get enough information to fix the bug

[DuncanUszkay1]

@dcodeIO any issues with this?

[dcodeIO]

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

[DuncanUszkay1]

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.

[dcodeIO]

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.

[DuncanUszkay1]

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.

[dcodeIO]

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.

[DuncanUszkay1]

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

[DuncanUszkay1]

Updated such that we don't return true on the second call to compileGlobal- the issue now is that if you do anything self referential:
var SELFREFERENCING: i32 = SELFREFERENCING then you get a stack overflow error on compilation. If we're worried about that, maybe we can add a COMPILING flag in addition to the COMPILED flag and fail whenever we try to compile a global that's already compiling?

[MaxGraey]

var SELFREFERENCING: i32 = SELFREFERENCING should print TS2448: Block-scoped variable 'SELFREFERENCING' used before its declaration.. If it's not true for now I guess better make another PR which will handle this

[dcodeIO]

What if we keep the COMPILED flag as-is, but add a Set holding all the globals that failed to compile earlier? Would that help?

  compileGlobal(global: Global): bool {
    if (global.is(CommonFlags.COMPILED)) return !this.failedGlobals.has(global);
    // whenever returning `false`, also add to `failedGlobals`