Builtin methods shall not be publicly visible

[Akirathan]

The @Builtin_Method are "different" to regular methods. Originally we wanted to make them behave the same as regular methods, but there is other solution: hide builtin methods!

Let's restrict usage of @Builtin_Method:

• throw an error when @Builtin_Method annotation is used in non-private module
• throw error if @Builtin_Method annotation is used elsewhere than Standard.Base (or rather Standard.Builtins module)

The first rule hides the builtin methods from Enso users. The second rule makes sure the builtin methods are only used by experienced developers working on Enso.

Original Report

The following snippet

from Standard.Base import all

main =
    dur1 = Duration.new hours=1
    dur2 = Duration.new hours=2
    IO.println <| Meta.meta Duration . methods . contains "plus_builtin" == True
    IO.println <| ((dur1.plus_builtin dur2) == (Duration.new hours=3))
    Duration.plus_builtin dur1 dur2 # Results in an unexpected error

outputs:

True
True
Execution finished with an error: Method `plus_builtin` of Duration could not be found.
        at <enso> tmp.main(tmp.enso:16:5-35)

In other words, a builtin method can be called as an instance method (dur1.plus_builtin dur2), but cannot be called as a static method Duration.plus_builtin dur1 dur2. This works for all the other non-builtin methods.

Tasks

Preview Give feedback

1. Remove Array.sort_builtin (SortArrayNode), which is a temporary workaround because of this issue. See sort handles incomparable values #5998 (comment)

[jdunkerley]

Is this still true? If so push back down!

[Akirathan]

Is this still true? If so push back down!

Yes, I can still reproduce this issue.

[JaroslavTulach]

Rather than improving builtins as this issue suggests, we should make sure they are more hidden!

• there is tight connection between library with builtins and engine
• makes no sense to have builtins in a module not bundled with the engine
• no reason to expose @Builtin_Method to users of Enso

E.g. let's restrict usage of @Builtin_Method:

• throw an error when it is used in non-private module as introduced by Implement private modules #7840
• throw error if it is used elsewhere than Standard.Base (or rather Standard.Builtins module)

[Akirathan]

Rather than improving builtins as this issue suggests, we should make sure they are more hidden!

@JaroslavTulach That makes sense. Generally, it is very difficult to achieve the same dispatch mechanism for builtin methods as for standard methods. Making sure that builtin methods can be invoked only in either Standard.Base or Standard.Builtins module seems like a good idea.

E.g. let's restrict usage of @Builtin_Method:

• throw an error when it is used in non-private module as introduced by Implement private modules #7840
• throw error if it is used elsewhere than Standard.Base (or rather Standard.Builtins module)

I agree with both points.

@hubertp - As one of the original builtin methods mechanism creators, can you see something that could potentially go wrong with this idea? If not, let's do this.

[JaroslavTulach]

I've just investigated the current status and here are my findings.

Make all @Builtin_Methods Private

With this patch all @Builtin_Method instances are going to be considered project private:

enso$ git diff engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java
diff --git engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java
index f93932d807..7e2b3afbc6 100644
--- engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java
+++ engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java
@@ -96,7 +96,7 @@ public final class Function extends EnsoObject {
    */
   public static Function fromBuiltinRootNode(BuiltinRootNode node, ArgumentDefinition... args) {
     RootCallTarget callTarget = node.getCallTarget();
-    FunctionSchema schema = FunctionSchema.newBuilder().argumentDefinitions(args).build();
+    FunctionSchema schema = FunctionSchema.newBuilder().projectPrivate().argumentDefinitions(args).build();
     return new Function(callTarget, null, schema);
   }

With such a simple patch we get runtime checks which yields various errors like:

sbt:enso> runEngineDistribution --run test/Base_Tests
Execution finished with an error: Private access error: The project-private method 'Ref.new' in unknown project cannot be accessed from project 'Standard.Test'.
        at <enso> Group_Builder.Impl(Group.enso:18:64-85)
        at <enso> Suite_Builder.group(Suite.enso:34:25-47)
        at <enso> Any_Spec.add_specs(src/Semantic/Any_Spec.enso:8-13)
        at <enso> Main.main.suite(src/Main.enso:99:9-40)
        at <enso> Test.build(Test.enso:24:9-33)
        at <enso> Main.main(src/Main.enso:98-179)

Make a non-private Method a Compilation Error

Following patch emits compile time warning when there is a builtin method which is not private:

diff --git engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala
index 21b9df88ba..a872f62c7a 100644
--- engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala
+++ engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala
@@ -144,6 +144,11 @@ case object UnusedBindings extends IRPass {
           if (isBuiltin) args
           else args.map(lintFunctionArgument(_, context))
         val body1 = runExpression(body, context)
+        if (isBuiltin && !lam.isPrivate) {
+          body1.addDiagnostic(
+            Warning.NonPrivateBuiltinMethod(body.identifiedLocation())
+          )
+        }
         val lintedBody =
           if (isBuiltin)
             body match {
diff --git engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Warning.scala engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Warning.scala
index be4ff3e4ad..bf5c1eeb07 100644
--- engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Warning.scala
+++ engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Warning.scala
@@ -89,6 +89,19 @@ object Warning {
     override def diagnosticKeys(): Array[Any] = Array()
   }
 
+  /** A warning about a `@Builtin_Method` not being private.
+    *
+    * @param identifiedLocation the location of the annotated application
+    */
+  case class NonPrivateBuiltinMethod(
+    override val identifiedLocation: IdentifiedLocation
+  ) extends Warning {
+    override def message(source: (IdentifiedLocation => String)): String =
+      "A @Builtin_Method must be private."
+
+    override def diagnosticKeys(): Array[Any] = Array()
+  }
+
   /** A warning raised when a method is defined with a `self` parameter defined
     * not in the first position in the parameters' list.
     *

All Builtins are from Standard.Base

Currently builtins don't have an associated Package. We should:

• disallow builtins from any other project that Standard.Base
• make sure all builtins properly report their project

enso$ git diff engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java
index 0cd267597d..69bccd918a 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java
@@ -14,6 +14,7 @@ import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.nodes.NodeInfo;
 import com.oracle.truffle.api.source.SourceSection;
 import java.util.UUID;
+import org.enso.editions.LibraryName;
 import org.enso.interpreter.Constants;
 import org.enso.interpreter.node.BaseNode;
 import org.enso.interpreter.node.EnsoRootNode;
@@ -272,6 +273,13 @@ public abstract class InvokeFunctionNode extends BaseNode {
   private boolean isInSameProject(Function function) {
     var thisProj = getThisProject();
     var funcProj = getFunctionProject(function);
+    if (funcProj == null) {
+        // it is a builtin
+        var ctx = EnsoContext.get(this);
+        var lib = LibraryName.apply("Standard", "Base");
+        var pkg = ctx.getPackageRepository().getPackageForLibrary(lib);
+        funcProj = pkg.isDefined() ? pkg.get() : null;
+    }
     return thisProj == funcProj;
   }

this patch is unlikely the best one.

We should make sure builtins are from Standard.Base

Rewrite all methods to be private

Then we need to rewrite those 444 instances that violate the privacy enforcement.