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

experiment: simpler version of claudio/self-pass #4735

Merged
merged 12 commits into from
Oct 17, 2024

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Oct 17, 2024

There's no need to add self_id to ActorE after all, thus avoiding much complexity.

The internal self declaration, if present, is enough.

The real culprit was some bad lowering that wasn't passing down the self-binding in the
ObjBlockE case (now fixed).

@crusso crusso changed the base branch from master to gabor/pass-self October 17, 2024 01:38
@@ -90,8 +90,8 @@ and exp' at note = function
(breakE "!" (nullE()))
(* case ? v : *)
(varP v) (varE v) ty).it
| S.ObjBlockE (s, _t, dfs) ->
obj_block at s None dfs note.Note.typ
| S.ObjBlockE (s, (self_id_opt,_), dfs) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the bug, which was failing to introduce a the self declaration for some cases.

@crusso crusso mentioned this pull request Oct 17, 2024
@crusso crusso marked this pull request as ready for review October 17, 2024 02:49
@crusso crusso requested a review from ggreif October 17, 2024 02:50
Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

LGTM! Nice improvement.

Copy link

Comparing from b6d40bd to 6997d0d:
The produced WebAssembly code seems to be completely unchanged.

@ggreif ggreif merged commit b957ce9 into gabor/pass-self Oct 17, 2024
6 checks passed
@ggreif ggreif deleted the claudio/pass-self-alt branch October 17, 2024 16:30
mergify bot pushed a commit that referenced this pull request Oct 17, 2024
…ialisers (#4719)

This PR accomplishes following improvements:
- allow `actor Self { ... Self ... }` in the actor initialiser (similarly for `actor class`)
- ditto for accessing all public methods `Self.method` from the actor initialiser
- accessing these from under lambdas without making the respective methods undefined when using them

The latter should allow setting up timers: `addTimer(b, period, Self.method)` from actor initialiser.

Also fixes the hole in definedness analysis (#4731).

-------------
_NOTES_:
- calling `Self.method` from actor initialiser is prohibited by the capability checker
- accessing `Self.method` from actor initialiser is currently allowed when `method` is not defined yet, and results in a descriptor (pair)
- comparison (in terms of `shared`) of the internal methods with external methods is trapping in the interpreter, but should work in the deployed actor.

There is an idea inside to fix these problems.

----
TODO:
- adapt `renaming.ml`? — nope, I don't have business in IR
- [x] test `actor class` too
- [x] test clashing `Self` — there is already `self-shadow.mo`
- [x] `test/run/issue1464ok.mo` fails — fixed snafu
- [x] deal with `FIXME`s
- [x] `shared` method equality in the interpreters
- [x] `shared` call viability checking (in interpreters)
- [x] fix #4731
- [x] write issue about the mixed (pair/closure) `shared func` comparison problem — here: #4732
- [x] resolve the `Promise.is_fulfilled` problem — #4735
- [x] check the "For actors, this may be too permissive" comment in the code — irrelevant, removed!
- [ ] try out the "crazy idea" mentioned in the feedback
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.

2 participants