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: allow access to Self and its public methods from actor initialisers #4719

Open
wants to merge 67 commits into
base: master
Choose a base branch
from

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Sep 30, 2024

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.


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.

TODO:

  • adapt renaming.ml? — nope, I don't have business in IR
  • test actor class too
  • test clashing Self — there is already self-shadow.mo
  • test/run/issue1464ok.mo fails — fixed snafu
  • deal with FIXMEs
  • shared method equality in the interpreters
  • shared call viability checking (in interpreters)
  • fix bug: actor self reference oddity #4731
  • write issue about the mixed (pair/closure) shared func comparison problem — here: interpreters have trouble comparing shared methods when mixed intern/extern #4732
  • resolve the Promise.is_fulfilled problem
  • check the "For actors, this may be too permissive" comment in the code

src/mo_frontend/definedness.ml Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Oct 2, 2024

Comparing from 15a4ac2 to fd53c84:
In terms of gas, no changes are observed in 5 tests.
In terms of size, no changes are observed in 5 tests.

to fetch actor and its method from the environment
update the right `env`
@ggreif ggreif changed the title feat: allow access to Self and its public methods from actor initialisers feat: allow access to Self and its public methods from actor initialisers Oct 11, 2024
when both sides are external view -> structurally compare tuples
when both internal -> physical equality (as before)
when mixed -> fail for now
ignore principalOfActor Self;
caller(Self.method);
caller(method);
//debugPrint (debug_show(principalOfActor Self));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bizarre: uncommenting this gives a forward declaration inference error... but we already use debugPrint above!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crusso any idea how to explain this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this resolved?

Copy link
Contributor

@crusso crusso Oct 15, 2024

Choose a reason for hiding this comment

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

I just played around with it. I expect it's because the call to debugPrint is the last declaration that is causing the issue...

This also produces the error:

import { debugPrint; principalOfActor } = "mo:⛔";

actor class C() = Self {
    debugPrint "So far so good!" 
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed an issue
#4733

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspected that it is unrelated. Thanks for checking!

src/mo_frontend/definedness.ml Show resolved Hide resolved
src/ir_interpreter/interpret_ir.ml Outdated Show resolved Hide resolved
(M.empty, S.singleton i.it) +++ delayify (
group msgs (dec_fields msgs dfs @ class_self d.at i') /// pat msgs p /// shared_pat msgs csp
group msgs ~extern (dec_fields msgs dfs @ class_self d.at i' s.it) /// pat msgs p /// shared_pat msgs csp
Copy link
Contributor

@crusso crusso Oct 15, 2024

Choose a reason for hiding this comment

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

Instead of adding the ~extern optional parameter, why not just add (class_self ... ) to beginning or end of the decs argument, depending on whether this is an actor (at front) or at other object (at end).

Copy link
Contributor

Choose a reason for hiding this comment

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

(and then rename class_self to obj_self...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that would break

$ cat test/run-drun/self-shadow.mo
actor foo {
  public func foo() {};

  flexible func go() : async () {
    let bar = actor bar { public func bar() {} }
  };
}

right? See also the discussion in #1104.

Comment on lines +201 to +204
(* Insert the externally defined binding in front if present, non-shadowing *)
let defWhen = match extern with
| Some b when M.find_opt b.it defWhen |> Option.is_none -> M.add b.it (-1) defWhen
| _ -> defWhen in
Copy link
Contributor

Choose a reason for hiding this comment

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

You might not need this if you just extend the decs appropriately,

src/mo_interpreter/interpret.ml Outdated Show resolved Hide resolved
ignore principalOfActor Self;
caller(Self.method);
caller(method);
//debugPrint (debug_show(principalOfActor Self));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this resolved?

let env' = adjoin_vals { env with self = self } ve_in in
interpret_dec_fields env' dec_fields ve_ex
let env'' = adjoin_vals { env' with self } ve_in in
interpret_dec_fields env'' dec_fields ve_ex
(fun obj ->
(env.actor_env := V.Env.add self obj !(env.actor_env);
Copy link
Contributor

Choose a reason for hiding this comment

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

Crazy idea: At the moment, the actor environment maps actor ids to Objects containing a map from field names to values.
Suppose we changed that, so an actor environment just contains a map from field names to promises (i.e. ve_ex).
Then we can update the actor map before we interpret the body. As the promises in ve_ex get defined, so do the ones accessible from the self-reference, a bit like your previous 'increments' but automatic.
I think that might let us implement the equality on mixtures of symbolic and function references (provided we also give the equality operator) access to the environment.

Best attempted in a separate PR I think. Might be a dead-end with too many pervasive changes.

src/mo_values/operator.ml Show resolved Hide resolved
Comment on lines +241 to +242
| Func _, Tup [Blob _; Text _]
| Tup [Blob _; Text _], Func _ -> assert false; (* mixed, cannot determine equality *)
Copy link
Contributor

Choose a reason for hiding this comment

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

If equality had access to the actor env then we could probably do better here and maybe even get it right (see above)

@crusso crusso mentioned this pull request Oct 16, 2024
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.

bug: actor self reference oddity
2 participants