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.

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

Strict field initialization #1155

Closed
wants to merge 17 commits into from
Closed

Strict field initialization #1155

wants to merge 17 commits into from

Conversation

saulecabrera
Copy link
Contributor

@saulecabrera saulecabrera commented Mar 10, 2020

This PR addresses #482

It introduces a new kind of flags: FieldFlags in order to track the state of each field per flow. Under the hood, field flags are represented as Map<string, FieldFlags> in which each field is uniquely identified by its internal name. Field flags are only initialized when the current flow belongs to a constructor, in every other case they are null

A general overview of the changes:

  • Dry up error reporting for this case, the code is almost identical when exporting a class and when instantiating one (Ensure constructors of exported classes #1154 helps on this)
  • Handle conditional field assignment in the constructor
  • Handle switch statement
  • Fix self compilation errors
  • Fix test errors regarding strict initialization
  • Constructor inlining

This commit introduces strict field initialization. All fields must be definitely assigned through an initializer or within the constructor else, a compilation error will be reported.
If the field is intended to be initialized later through dependency injection or a custom initialiazer, the field must be post-fixed with !
This commit add field initialization at the flow level. It also refactors the error reporting for uninitialized properties.
@saulecabrera
Copy link
Contributor Author

Flows was an interesting journey 😅

I've added support for conditional flows in the constructor in my last commits, in the test files you can find the use cases (https://github.com/AssemblyScript/assemblyscript/pull/1155/files#diff-5fa1688a93849f802b2ae2b7f3cbffe6R26)

I realized that we need the same behaviour for switch, I'm tackling that as part of my next commit.

src/flow.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
- Use `actualFunction` to check for constructor flows
- Fix compilation errors regarding boolean comparisons
 - Fix self compilation
 - Add strict property initialization flags for TS
 - Handle constructor inlining
@saulecabrera saulecabrera marked this pull request as ready for review March 29, 2020 23:49
@saulecabrera
Copy link
Contributor Author

In 743ef7e I fixed self compilation and tests all around.

Self compilation seems to be OK on CI, anyhow I'm getting an index out of range exception on npm run astest, I've tracked this down and I believe it's here, but not sure what is causing it.

@MaxGraey
Copy link
Member

It seems main reason on failure happened in setPedantic:
https://github.com/AssemblyScript/assemblyscript/runs/544024426#step:6:476

@dcodeIO
Copy link
Member

dcodeIO commented Mar 30, 2020

Not necessarily, the error begins a few lines prior. Wondering how it is even able to print during console.log - as if inspecting would call something.

@saulecabrera
Copy link
Contributor Author

I spent some time today on @MaxGraey's hint (debugging setPedantic) but couldn't find anything meaningful, then I started backtracking on all the changes in this PR with no luck, I was mostly trying to prove that all the fields that I marked with ! have an equivalent effect as prior to this change and that they have nothing to do with the error.

src/flow.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
saulecabrera and others added 3 commits March 31, 2020 09:17
Don not skip property initialization even if expected to be initialized
at the constructor level
Co-Authored-By: Max Graey <maxgraey@gmail.com>
Co-Authored-By: Max Graey <maxgraey@gmail.com>
@saulecabrera saulecabrera changed the title [WIP] Strict field initialization Strict field initialization Mar 31, 2020
Co-Authored-By: Max Graey <maxgraey@gmail.com>
@saulecabrera
Copy link
Contributor Author

saulecabrera commented Apr 1, 2020

So I was not terribly convinced that the fix that I introduced in 8c6a554 was the right way to go without knowing why. I've found the answer, it turns out that we're not compiling exported generic classes unless that this is done in a different code path or in a different way that I'm not aware of; that means that with the code that I had prior to 8c6a554 for a class like:

export class C<T> {
  prop: i32
}

the compiler won't report any compilation errors and even worse it will skip the initialization of prop: i32, this will eventually result in an out of range error.

@dcodeIO:

  • Any insight on why we skip the compilation of exported generic classes?

@MaxGraey
Copy link
Member

MaxGraey commented Apr 1, 2020

It's well known issue. And we even have PR for this: #981

@dcodeIO
Copy link
Member

dcodeIO commented Apr 1, 2020

What should happen currently is, if one has

export class C<T> {
  prop: i32
}

there is nothing to export because we don't know what T will be, so there is no code to export. However, as soon as one does

export class C<T> {
  prop: i32
}
new C<i32>(); // here

we know that there is indeed a class C<i32> and we will create the exports for these, but not as C#xy but as C<i32>#xy, for each concrete class. Might be that this doesn't work a 100% currently, but that's the intended mechanism, with the issue linked above asking for an error if something is exported that is never instantiated.

@MaxGraey
Copy link
Member

MaxGraey commented Apr 1, 2020

export class C<T> {
  prop: i32
}
new C<i32>(); // here

I guess even with this case we shouldn't export generics for host and throw the error / warning

@dcodeIO
Copy link
Member

dcodeIO commented Apr 26, 2020

Hesitating a bit here due to all the someField! changes to the compiler sources. I think that since we need proper type checking anyway, and can reasonably default non-reference fields to zero, perhaps definite assignment should be the default, with the annotation being optional where necessary due to type checking issues with tsc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants