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

feat: identify invalid captures (and other improvements) #1743

Merged
merged 8 commits into from
Mar 15, 2023

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Mar 7, 2023

Following up on #1682 to identify cases where we cannot infer which operations are performed on a captured resource.

See positive/negative tests for examples.

Rewrite the algorithm which analyzes the expressions captured by inflight methods so that it is able to identify more cases and emit errors when captures cannot be qualified (i.e. a resource is captured but we cannot determine which operations are performed on it without static analysis).

For each method, we identify all expressions that start with this.xxx and break them down into parts (using nested references). Then, we traverse the list of parts and split the expression into preflight and inflight. The preflight part is what we are capturing and the first inflight component qualifies which operations are performed on the captured object.

Reorganized capture tests into resource_captures (both under valid and invalid).

This does not address #76 but it explicitly identifies these cases. We will follow up at some point with a way to allow users to explicitly qualify the reference.

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

@eladb eladb requested a review from a team as a code owner March 7, 2023 16:35
@eladb eladb requested review from yoav-steinberg and Chriscbr March 7, 2023 16:35
@eladb eladb changed the title feat: errors when cannot infer resource operations during capture feat: emit an error when cannot infer resource operations during capture Mar 7, 2023
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.

Looks good to me (added a few non-blocking questions)

libs/wingsdk/src/core/resource.ts Outdated Show resolved Hide resolved
libs/wingc/src/jsify.rs Outdated Show resolved Hide resolved
examples/tests/invalid/inflight_ref_resource_sub_method.w Outdated Show resolved Hide resolved
examples/tests/valid/inflight_ref_resource_collection.w Outdated Show resolved Hide resolved
examples/tests/valid/inflight_ref_resource_sub_field.w Outdated Show resolved Hide resolved
libs/wingc/src/jsify.rs Outdated Show resolved Hide resolved
libs/wingc/src/jsify.rs Outdated Show resolved Hide resolved
libs/wingc/src/jsify.rs Outdated Show resolved Hide resolved
libs/wingc/src/jsify.rs Outdated Show resolved Hide resolved
libs/wingc/src/jsify.rs Outdated Show resolved Hide resolved
@ekeren ekeren mentioned this pull request Mar 9, 2023
@eladb eladb marked this pull request as draft March 9, 2023 13:54
@eladb eladb requested a review from Chriscbr March 9, 2023 13:54
libs/wingc/src/diagnostic.rs Outdated Show resolved Hide resolved
libs/wingc/src/type_check.rs Outdated Show resolved Hide resolved
libs/wingc/src/diagnostic.rs Outdated Show resolved Hide resolved
libs/wingc/src/diagnostic.rs Outdated Show resolved Hide resolved
libs/wingc/src/jsify.rs Outdated Show resolved Hide resolved
examples/tests/valid/inflight_ref_resource_collection.w Outdated Show resolved Hide resolved
Rewrite the algorithm which analyzes the expressions captured by inflight methods so that it
is able to identify more cases and emit errors when captures cannot be qualified (i.e. a resource
is captured but we cannot determine which operations are performed on it without static analysis).

Merge all `inflight_ref_resource_*` valid tests into `resource_captures` and all negative tests as well.
@eladb eladb force-pushed the eladb/fail-non-infer branch from 8c2f74c to 220f5ed Compare March 15, 2023 21:14
@eladb eladb changed the title feat: emit an error when cannot infer resource operations during capture feat: identify invalid captures (and other improvements) Mar 15, 2023
@eladb eladb marked this pull request as ready for review March 15, 2023 21:19
@eladb eladb requested a review from Chriscbr March 15, 2023 21:36
@eladb
Copy link
Contributor Author

eladb commented Mar 15, 2023

@yoav-steinberg @Chriscbr can you take another look here? I think this is done.

libs/wingc/src/jsify.rs Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Mar 15, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit c217ea1 into main Mar 15, 2023
@mergify mergify bot deleted the eladb/fail-non-infer branch March 15, 2023 22:30
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.5.134.

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.

5 participants