Skip to content

Fix variable initialization checks / revise flow logic #2578

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.

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

Merged
merged 14 commits into from
Dec 5, 2022
Merged

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Nov 23, 2022

Fixes #899. This has been a long-standing issue I apparently forgot about, and as it turns out there was quite a bit more attached to it. Specifically:

  • Switch statements now need to maintain proper flows (see also Use proper flows when compiling switch statements #2573),
  • Logical expressions need to record the respective true-ish alternative,
  • Flow logic was missing some cases (in particular in do statements), and
  • There is another source of unsoundness in TS that had to be addressed.

The last one isn't great, in that in the absence of full program analysis, we either need to enforce initializers on globals or introduce runtime checks where TS would not diagnose a problem. Decided to implement both, but limited it to non-nullable references since for other types there is a sound zero. POC in TS is:

let foo: string;
function bar() {
  foo.length; // no error even though undefined
}
bar();

Definitive assignment can be used as a fallback, then at the cost of runtime non-null checks when such a global is accessed:

let foo!: string;
function bar(): void {
  foo.length; // Aborts with "Unexpected 'null'" at runtime
}
bar();

Making a draft for now as it seems that there are more places to look at, closing #2573 which is included here, and perhaps there are thoughts about the changes to global initialization as explained above?

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

@dcodeIO
Copy link
Member Author

dcodeIO commented Nov 24, 2022

Alright, this is the most annoying stuff I've seen in a while. Wraith of flows having grown somewhat organically. Good news is that I now know about why, bad news is that oh my god.

@dcodeIO
Copy link
Member Author

dcodeIO commented Dec 1, 2022

While this PR supports definite assignments on globals (and static class fields, which are also globals in AS) now, there are two discrepancies here.

One with a Wasm GC future: In Wasm, globals imply an initializer, so if we have something like

var foo!: string;

there is nothing we can initialize the respective Wasm global with in Wasm

 (global $index/foo (mut (ref string)) (???))

Two options: Either we represent the global off-type in the binary, as nullable (ref null string) and insert casts whenever the global is accessed, or we don't support definitive assignment in this case closely adhere to what Wasm supports.

Another is with TypeScript: In TS, static class fields cannot have definitive assignment

class Foo {
  static bar!: string; // TS1255
}

though these become globals in AS. As such, in AS, this distinction doesn't make sense.

That's all somewhat unfortunate, because one or another property must be sacrificed anyway. I tend to drop support for definitive assignment either now or later and instead focus on almost 1:1 roundtripping from source to Wasm, which then basically means that we'd mandate that variables are always initialized with some value, e.g. using a nullable type and assigning null, i.e.

var foo!: string;

is not supported and instead we require

var foo: string | null = null;

Wdyt? cc @MaxGraey

@dcodeIO
Copy link
Member Author

dcodeIO commented Dec 2, 2022

Fine side-effect here is that now that flows cover all relevant termination cases the prior fixup code to insert unreachables in previously uncertain cases became unnecessary.

@dcodeIO dcodeIO marked this pull request as ready for review December 3, 2022 15:54
@dcodeIO
Copy link
Member Author

dcodeIO commented Dec 3, 2022

Postponing the decision on definitive assignment for now.

@dcodeIO dcodeIO changed the title Fix local/global variable initialization checks Fix variable initialization checks / revise flow logic Dec 4, 2022
@dcodeIO dcodeIO merged commit 6717de0 into main Dec 5, 2022
@iamdustan
Copy link

I think this broke as-variant at https://github.com/MaxGraey/as-variant/blob/main/assembly/index.ts#L15.

I started looking into making a repro test case on as-variant to support fixing that as well, however, it seemed like that would also involve either moving as-variant off of as-pect or updating as-pect to this version as well. ...that was more than I was ready to sign up for tonight, so just wanted to surface that here.

@MaxGraey
Copy link
Member

MaxGraey commented Feb 8, 2023

It would be great to make a rule exception for instanceof usage, like in this case:

function write<T>(variant: T) {
  if (isArrayLike<T>()) {
    let item: valueof<T>;
    if (item instanceof Foo) {
      // do something
    }
  }
}

The problem is that if you try init item like item: valueof<T> = changetype<valueof<T>>(0) it will works for references but not for integers especially small types. So it all turns into a very complicated challenge

@HerrCai0907 HerrCai0907 deleted the issue-899 branch October 17, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants