Skip to content

Refactor compiler sources to adhere to TS strict mode #1283

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 5 commits into from
May 25, 2020
Merged

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented May 18, 2020

As the title says, this PR enables TypeScript's --strict mode (strict null checks, strict checking of function types, strict checking of property initialization in classes) in compiler sources by means of refactoring quite a bit of code. Also found a few potentially problematic places where ranges were missing.

Helps #1155 in particular in that the compiler sources don't need a heap of definitive assignment assertions anymore, where my expectation is that these will yield runtime checks ultimately, which we now avoid.

Technically this is a breaking change for internal compiler API users due to changed parameter orders and similar, but not for compiler users, so I'm not sure if we should make this a new major release just for that, or if it's ok to assume that internal API compatibility is something we can't guarantee between minor versions just yet?

@dcodeIO
Copy link
Member Author

dcodeIO commented May 19, 2020

The next thing I wanted to look into was the same for stdlib, but the Map and Set constructors that reuse this.clear() for initialization leave me puzzled. Reusing this part of code makes total sense as it avoids duplication, but expressing this in an AOT-safe way isn't possible, unless using a definitive assignment assertion, leaving us with:

  • Accept the runtime check on every access of a DA property (quite a bit of overhead)
  • Make it an unsafe feature with an implicit null-store instead (some overhead)
  • Duplicate code (meh)
  • changetype the hell out of it (unrealistic in user code)
  • Diverge from TypeScript and traverse into functions within the constructor (¯\(ツ)/¯, new set of problems)

@dcodeIO
Copy link
Member Author

dcodeIO commented May 19, 2020

One alternative I see is to allow uninitialized fields if their default value is a zero anyway and do a memory.fill upfront to guarantee that this is correct Edit: makeFieldInitializationInConstructor already takes care otherwise. For Map this means that

export class Map<K,V> {
  private buckets: ArrayBuffer | null;
  private bucketsMask: u32;
  private entries: ArrayBuffer | null;
  private entriesCapacity: i32;
  private entriesOffset: i32;
  private entriesCount: i32;

is fine since every field can use a default value of zero. In particular this works for Map because this.buckets is accessed as changetype<usize>(this.buckets) exclusively anyway, and we know it is not null, so there is at least a straight-forward workaround in cases like these. Very close to the 4th option above, with a bit of the 5th.

@dcodeIO
Copy link
Member Author

dcodeIO commented May 25, 2020

Merging to get it out of the way, as it is likely that future work will conflict significantly otherwise.

@dcodeIO dcodeIO merged commit 7702f5d into master May 25, 2020
@dcodeIO dcodeIO deleted the strict branch June 11, 2020 17:17
@dcodeIO dcodeIO mentioned this pull request Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant