State rework & IO Contexts #3828

kustosz · 2022-10-25T11:20:18Z

Pull Request Description

Changes how we do monadic state – rather than a haskelly solution, we now have an implicit env with mutable data inside. It's better for the JVM. It also opens the possibility to have state ratained on exceptions (previously not possible) – both can now be implemented.
Introduces permission check system for IO actions.

Important Notes

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

JaroslavTulach

Very nice simplification.

JaroslavTulach · 2022-10-25T13:15:42Z

engine/runtime/src/main/java/org/enso/interpreter/node/ClosureRootNode.java

@@ -87,9 +86,7 @@ public Object execute(VirtualFrame frame) {
    }
    Object state = Function.ArgumentsHelper.getState(frame.getArguments());
    frame.setObject(this.getStateFrameSlot(), state);
-    Object result = body.executeGeneric(frame);
-    state = FrameUtil.getObjectSafe(frame, this.getStateFrameSlot());
-    return new Stateful(state, result);


Yes, this is simpler.

JaroslavTulach · 2022-10-25T13:17:44Z

engine/runtime/src/main/java/org/enso/interpreter/node/callable/ApplicationNode.java

-            this.callable.executeGeneric(frame), frame, state, evaluateArguments(frame));
-    frame.setObject(getStateFrameSlot(), result.getState());
-    return result.getValue();
+    State state = (State) FrameUtil.getObjectSafe(frame, getStateFrameSlot());


State is a class. It is in a getStateFromSlot().

JaroslavTulach · 2022-10-25T13:28:41Z

engine/runtime/src/main/java/org/enso/interpreter/node/callable/thunk/ForceNode.java

-    Stateful result = thunkExecutorNode.executeThunk(thunk, state, getTailStatus());
-    frame.setObject(getStateFrameSlot(), result.getState());
-    return result.getValue();
+    State state = (State) FrameUtil.getObjectSafe(frame, getStateFrameSlot());


I suggest to introduce:

static State.getState(Frame frame, BaseNode where) { ... }

which could encapsulate this code into a nicer API.

JaroslavTulach · 2022-10-25T13:31:54Z

...time/src/main/java/org/enso/interpreter/node/controlflow/permission/PermissionGuardNode.java

+
+  @Override
+  public Object executeGeneric(VirtualFrame frame) {
+    State state = (State) FrameUtil.getObjectSafe(frame, getStateFrameSlot());


While evaluating I found :

protected FrameSlot getStateFrameSlot() { if (stateFrameSlot == null) { CompilerDirectives.transferToInterpreterAndInvalidate(); stateFrameSlot = ((EnsoRootNode) getRootNode()).getStateFrameSlot(); } return stateFrameSlot; }

I'd like to ask? Does it make sense to have per node local stateFrameSlot? It might be enough to:

protected FrameSlot getStateFrameSlot() { return((EnsoRootNode) getRootNode()).getStateFrameSlot(); }

Should have the same performance (on fast path) because getRootNode() is a compilation constant.

JaroslavTulach · 2022-10-25T13:39:14Z

...src/main/java/org/enso/interpreter/node/expression/builtin/function/ApplicationOperator.java

-  Stateful execute(
-      VirtualFrame frame, @MonadicState Object state, Object self, @Suspend Object argument) {
+  Object execute(
+      VirtualFrame frame, @MonadicState State state, Object self, @Suspend Object argument) {


Do we need the @MonadicState annotation? Isn't it enough to check for State and treat each State as a carrier for the @MonadicState?

JaroslavTulach · 2022-10-25T13:55:41Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/IOPermissions.java

@@ -0,0 +1,31 @@
+package org.enso.interpreter.runtime.state;
+
+public record IOPermissions(String name, boolean isInputAllowed, boolean isOutputAllowed) {


Using record as enum! You don't want this class to have public constructor. Try:

private IOPermissions { }

JaroslavTulach · 2022-10-25T13:56:59Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/State.java

+import com.oracle.truffle.api.object.DynamicObject;
+import com.oracle.truffle.api.object.Shape;
+
+public record State(Container container, IOPermissions ioPermissions) {


Probably you also want a private constructor with a factory method to get the default State.

radeusgd · 2022-10-26T11:41:24Z

test/micro-distribution/lib/Standard/Base/0.0.0-dev/src/Main.enso

+allow_input_in env = @Builtin_Method "Runtime.allow_input_in"
+allow_output_in env = @Builtin_Method "Runtime.allow_output_in"


It feels like this should be placed in a file Runtime.enso and re-exported here, no?

And btw. shouldn't these definitions be placed also in the main distribution too? Otherwise, they are unreachable in the normal operations outside of the tests that replace the distribution with micro-distribution.

Because of that - I'd also suggest to add a copy of at least one tests from IOContextTest.scala to test/Tests/src/Runtime/IO_Context_Spec.enso so that we can ensure that they work outside of the micro-distribution framework properly (as I assume currently they wouldn't).

Or is that planned for some separate future PR?

radeusgd · 2022-10-26T12:30:42Z

test/Tests/src/Data/Text/Utils_Spec.enso

-        Text_Utils.take_prefix (kshi+kshi+kshi) 2 . should_equal kshi+kshi
-        Text_Utils.take_suffix (kshi+kshi+'a'+kshi) 2 . should_equal 'a'+kshi
-        Text_Utils.take_suffix (kshi+kshi+'a'+kshi) 1 . should_equal kshi
+        Test.specify "should correctly take prefix and suffix of a string" <|


Thanks for catching this!

radeusgd · 2022-10-26T12:39:23Z

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/IOContextTest.scala

+  override def contextModifiers: Option[Context#Builder => Context#Builder] =
+    Some(_.option("enso.IOEnvironment", "development"))


Is this the reason why these tests need to be in Scala and cannot be just pure Enso tests? Can we get a comment explaining that?

kustosz added 4 commits October 12, 2022 23:10

take state apart

3f20ebd

implement new state

35a8822

make test retained on panics

28f1488

final touches

30d02fa

kustosz requested review from jdunkerley, radeusgd, 4e6, JaroslavTulach and hubertp as code owners October 25, 2022 11:20

kustosz added 3 commits October 25, 2022 13:35

Merge branch 'develop' into wip/mk/state-refactor

45cdfeb

fix changelog

7616d2e

fmt

6282e6a

JaroslavTulach approved these changes Oct 25, 2022

View reviewed changes

JaroslavTulach reviewed Oct 25, 2022

View reviewed changes

radeusgd reviewed Oct 26, 2022

View reviewed changes

kustosz added 4 commits October 26, 2022 15:14

CR feedback

070d78c

Merge branch 'develop' into wip/mk/state-refactor

f1168a5

CI fight

414e8f4

export from stdlib

5786006

radeusgd approved these changes Oct 26, 2022

View reviewed changes

kustosz added the CI: Ready to merge This PR is eligible for automatic merge label Oct 26, 2022

mergify bot merged commit 9017608 into develop Oct 26, 2022

mergify bot deleted the wip/mk/state-refactor branch October 26, 2022 16:22

wdanilo mentioned this pull request Feb 6, 2023

Replace existing AST with Rust based AST #4691

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State rework & IO Contexts #3828

State rework & IO Contexts #3828

kustosz commented Oct 25, 2022

JaroslavTulach left a comment

JaroslavTulach Oct 25, 2022

JaroslavTulach Oct 25, 2022

JaroslavTulach Oct 25, 2022

JaroslavTulach Oct 25, 2022

JaroslavTulach Oct 25, 2022

JaroslavTulach Oct 25, 2022

JaroslavTulach Oct 25, 2022

radeusgd Oct 26, 2022

radeusgd Oct 26, 2022

radeusgd Oct 26, 2022

radeusgd Oct 26, 2022

		@@ -0,0 +1,31 @@
		package org.enso.interpreter.runtime.state;

		public record IOPermissions(String name, boolean isInputAllowed, boolean isOutputAllowed) {

		allow_input_in env = @Builtin_Method "Runtime.allow_input_in"
		allow_output_in env = @Builtin_Method "Runtime.allow_output_in"

		override def contextModifiers: Option[Context#Builder => Context#Builder] =
		Some(_.option("enso.IOEnvironment", "development"))

State rework & IO Contexts #3828

State rework & IO Contexts #3828

Conversation

kustosz commented Oct 25, 2022

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment