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

fix(compiler): no error when trying to reassign to a let variable #1196

Merged
merged 13 commits into from
Jan 22, 2023

Conversation

yoav-steinberg
Copy link
Contributor

@yoav-steinberg yoav-steinberg commented Jan 19, 2023

now this:

let x = 9;
x = 10;

produces something like: Error at ../../examples/tests/invalid/reassing_to_nonreassignable.w:2:1 | Variable x is not reassignable

Also correctly import reassignability (let/var) from JSII class properties.

see #1186

By submitting this pull request, I confirm that my contribution is made under the terms of the
Monada Contribution License
.

Also correctly import reassignability (let/var) from JSII class properties.
@yoav-steinberg yoav-steinberg changed the title fix(compiler): No error when trying to assing to a let variable. fix(compiler): no error when trying to assing to a let variable. Jan 19, 2023
@yoav-steinberg yoav-steinberg marked this pull request as ready for review January 19, 2023 00:04
@yoav-steinberg yoav-steinberg requested a review from a team as a code owner January 19, 2023 00:04
@yoav-steinberg yoav-steinberg changed the title fix(compiler): no error when trying to assing to a let variable. fix(compiler): no error when trying to assing to a let variable Jan 19, 2023
@yoav-steinberg yoav-steinberg linked an issue Jan 19, 2023 that may be closed by this pull request
@yoav-steinberg
Copy link
Contributor Author

yoav-steinberg commented Jan 19, 2023

Note that this is currently failing because one of our tests attempt to write to an argument passed to a function, but there's currently no way of specifying if an argument is reassignable:

let f = (x: num) => {
  x = x + 1; // This write fails because `x` is non reassignable
};

Should we suppor the following syntax to fix this? @eladb @Chriscbr WDYT?

let f = (var x: num) => {
  x = x + 1;
};

See related TypeScript and Kotlin discussions

@Chriscbr
Copy link
Contributor

@yoav-steinberg I'd be okay adding that syntax later if there is demand for it, though I feel like it's generally safer for parameters to not be reassignable by default.

Copy link
Contributor

@Chriscbr Chriscbr left a comment

Choose a reason for hiding this comment

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

One suggestion to reduce some verbosity - otherwise LGTM. I like the var argument idea as well 👍

libs/wingc/src/type_check.rs Outdated Show resolved Hide resolved
@Chriscbr
Copy link
Contributor

Also, can you update the spec to mention this feature as well? We should keep it in sync 👍

yoav-steinberg and others added 3 commits January 21, 2023 21:16
* Replaced `VariableKind` with a `reassignable` bool for clarity.
* Support JSII imported mutable fields via "reassignable".
* Allow assigning to non-reassingbale fields in `init` for class initialization.
Signed-off-by: github-actions <[email protected]>
@yoav-steinberg yoav-steinberg added the 🚧 pr/do-not-merge PRs with this label will not be automatically merged by mergify. label Jan 21, 2023
@yoav-steinberg
Copy link
Contributor Author

Update: fixed everything related to to function arguments not being var.
Also added support for modifying non-var class fields in init so we can initialize classes (this also caused test failures).
Also added support (and tests) for checking reassignablity of class fields.
Additionally added support for mutable JSII class fields by marking them as reassignable on import.

@Chriscbr Chriscbr changed the title fix(compiler): no error when trying to assing to a let variable fix(compiler): no error when trying to assign to a let variable Jan 22, 2023
@Chriscbr Chriscbr changed the title fix(compiler): no error when trying to assign to a let variable fix(compiler): no error when trying to reassign to a let variable Jan 22, 2023
examples/tests/valid/reassignment.w Outdated Show resolved Hide resolved
@eladb
Copy link
Contributor

eladb commented Jan 22, 2023

@yoav-steinberg is this ready to be merged?

@yoav-steinberg
Copy link
Contributor Author

@yoav-steinberg is this ready to be merged?

Not yet, I woke up this morning having a feeling this might break:

resource R {
  f: num;
  init() {
    if true {
      this.f = 1; // Access a non reassignable field from constructor but not in upper most scope. Need to make sure this works!
    }
  }
}

@yoav-steinberg yoav-steinberg merged commit 6c0026f into main Jan 22, 2023
@yoav-steinberg yoav-steinberg deleted the yoav/reassing_to_let branch January 22, 2023 18:10
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.4.121.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧 pr/do-not-merge PRs with this label will not be automatically merged by mergify.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler should error if let variable is reassigned to
4 participants