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.

Sign up for GitHub

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

Jump to bottom

Always allow field access for objects defined inside a recover block #3606

Merged
merged 15 commits into from
Aug 25, 2020
Merged

Always allow field access for objects defined inside a recover block #3606

merged 15 commits into from
Aug 25, 2020

Conversation

jasoncarr0
Copy link
Contributor

@jasoncarr0 jasoncarr0 commented Aug 10, 2020 •
edited
Loading

I should have opened this ages ago, but this change always allows simple assignments with variables defined locally:
x.y = e, x.y.z.q = e
Soundness can be seen because this functionality is possible today using methods, it is just very cumbersome.

This also includes a change to enable field-writes to non-local variables when the destination is sendable (this is slightly less than the most general: which would include writing a sendable expression to any field; but that requires adding more check logic). The current checks had very strange logic, and don't seem to be necessary: We only need to maintain isolation of the recover region, so it's perfectly fine to move sendable items in or out without restriction, since they cannot violate the guarantees. See #1057 for context of those original changes.

Fixes #3069

@jasoncarr0 jasoncarr0 changed the title Always allow local fields to be assigned to in recover expressions Always allow fields of local objects to be assigned to in recover expressions Aug 10, 2020
@jasoncarr0 jasoncarr0 changed the title Always allow fields of local objects to be assigned to in recover expressions Always allow fields of local objects to be accessed in recover expressions Aug 10, 2020
@jasoncarr0
Copy link
Contributor Author

jasoncarr0 commented Aug 10, 2020 •
edited
Loading

That's why I never opened this, I didn't rigorously test deep field accesses: they won't work in all cases with this.

@jasoncarr0 jasoncarr0 marked this pull request as draft August 10, 2020 18:55
@jasoncarr0 jasoncarr0 changed the title Always allow fields of local objects to be accessed in recover expressions [WIP] Always allow fields of local objects to be accessed in recover expressions Aug 10, 2020
@jasoncarr0 jasoncarr0 marked this pull request as ready for review August 16, 2020 06:09
@jasoncarr0 jasoncarr0 changed the title [WIP] Always allow fields of local objects to be accessed in recover expressions [WIP] Aug 16, 2020
@jasoncarr0
Copy link
Contributor Author

May need to add/remove/merge unit tests still to ensure the right behavior is covered, but this is set to review.

@jasoncarr0 jasoncarr0 changed the title [WIP] Always allow fields of local objects to be accessed in recover expressions & allow sendable writes Aug 16, 2020
@jasoncarr0 jasoncarr0 changed the title Always allow fields of local objects to be accessed in recover expressions & allow sendable writes Allow more flexible field access in recover blocks: local fields and sendable writes Aug 16, 2020
@jasoncarr0
Copy link
Contributor Author

jasoncarr0 commented Aug 16, 2020 •
edited
Loading

Windows CI is failing because of one variable which is used in a single case branch, complaining that it's not initialized elsewhere, which is technically true, but missing the point. I'll fix it, but remain quite disgruntled about it as it makes the code harder to read for everyone & and the compiler, imo.

@SeanTAllen
Copy link
Member

I don't know this code at all. I'm leaving this in @jemc's hands as I consider him the "domain expert".

@SeanTAllen
Copy link
Member

@jasoncarr0 can you rebase this against master?

@jasoncarr0 jasoncarr0 force-pushed the local-field-in-recover branch from 56173ad to 96deb45 Compare August 19, 2020 19:33
@jasoncarr0 jasoncarr0 changed the title Allow more flexible field access in recover blocks: local fields and sendable writes Always allow field access for objects defined inside a recover block Aug 22, 2020
@jasoncarr0
Copy link
Contributor Author

jasoncarr0 commented Aug 22, 2020 •
edited
Loading

I've reverted the component about field accesses for another day (I still believe it's an important/sound addition) and likely an RFC. Accessing local fields is an important fix to get in as it's very cumbersome but always possible to work around (hence no soundness changes). I've also improved the log messages to be somewhat clearer.

@jasoncarr0
Allow field accesses on variables defined within the same recover block
583c035
@jasoncarr0
Add several tests for accessing local/nonlocal fields in recover blocks
87d050e
@jasoncarr0
Fix merge issue
8fde72a
@jasoncarr0
More merge fixes
1ed6beb
@jasoncarr0
Add test for deep local field access
00e8710
@jasoncarr0
Add expr_contained_in predicate for field access
ef229d5 
An expression all of whose variables are from the recover block
itself, is always valid. In this case this is only checking
deep field access by recursing to the left
@jasoncarr0
Allow field writes inside recover-blocks, so long as sendability is met.
648fba8 
So long as the thing to write is sendable, it can be safely removed
from the isolated region of the recover block, just like
for an iso object.
@jasoncarr0
Move & clean up tests
803e4e4
@jasoncarr0
Rename test
a994fc5
@jasoncarr0
Appease windows CI
dbb416f
@jasoncarr0
Revert "Allow field writes inside recover-blocks, so long as sendabil…
8d5b03a 
…ity is met."

This reverts commit dde7b9f.
@jasoncarr0
Correct condition for assignment revert
7a806dd
@jasoncarr0
Make field-access rejection log messages more descriptive
eb5e32a
@jasoncarr0 jasoncarr0 force-pushed the local-field-in-recover branch from 4e32d6a to eb5e32a Compare August 22, 2020 22:17
jemc
jemc reviewed Aug 24, 2020
View reviewed changes
Copy link
Member

@jemc jemc 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. Just one naming suggestion:

src/libponyc/expr/reference.c Outdated Show resolved Hide resolved
@jasoncarr0 @jemc
Apply naming suggestion
bff6210 
Co-authored-by: Joe Eli McIlvain <[email protected]>
jemc
jemc approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

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

Good to merge once the CI runs come back green.

@jasoncarr0
Copy link
Contributor Author

For a moment I was excited at how easy the suggestions feature made it and then I realized I still needed to change the callsite 😅

@SeanTAllen SeanTAllen added changelog - added Automatically add "Added" CHANGELOG entry on merge changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge and removed changelog - added Automatically add "Added" CHANGELOG entry on merge labels Aug 25, 2020
@SeanTAllen
Copy link
Member

@jasoncarr0 can you add public facing release notes to #3605 for this?

@SeanTAllen SeanTAllen merged commit 16a810e into ponylang:master Aug 25, 2020
github-actions bot pushed a commit that referenced this pull request Aug 25, 2020
@ponylang-main
Update CHANGELOG for PR #3606
b96ec55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jemc jemc jemc approved these changes

Assignees
No one assigned
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't access newly created object fields inside a recover expression
3 participants
@jasoncarr0 @SeanTAllen @jemc