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

Comparable.from doesn't return Comparable #10355

Closed
JaroslavTulach opened this issue Jun 25, 2024 · 6 comments · Fixed by #10468
Closed

Comparable.from doesn't return Comparable #10355

JaroslavTulach opened this issue Jun 25, 2024 · 6 comments · Fixed by #10468
Assignees
Labels
-compiler -libs Libraries: New libraries to be implemented

Comments

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Jun 25, 2024

Alas, conversion methods are not checked right now, because Comparable.from typically violates the required contract - it doesn't return Comparable, but some unrelated type.

The goal of this issue is to change the way we deal with Comparable and turn on the return type check for conversion methods.

type Comparable
    private Both value:Any comparator:Any
    
    new value:Any comparator:Any -> Comparable = Comparable.Both value comparator

    // implement operations by using self.value and that.value and delegating to self.comparator
    <,>,>=,<=,=
  • Verify the extra abstraction doesn't influence benchmarks.
  • Turn return type check on: patch is here
@JaroslavTulach
Copy link
Member Author

This is the code that enables return type check for conversion methods:

diff --git engine/runtime-integration-tests/src/test/java/org/enso/compiler/ExecCompilerTest.java engine/runtime-integration-tests/src/test/java/org/enso/compiler/ExecCompilerTest.java
index c194132011..ad4ea01208 100644
--- engine/runtime-integration-tests/src/test/java/org/enso/compiler/ExecCompilerTest.java
+++ engine/runtime-integration-tests/src/test/java/org/enso/compiler/ExecCompilerTest.java
@@ -21,6 +21,7 @@ import org.graalvm.polyglot.Context;
 import org.graalvm.polyglot.PolyglotException;
 import org.graalvm.polyglot.Source;
 import org.graalvm.polyglot.io.IOAccess;
+import org.hamcrest.core.AllOf;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Ignore;
@@ -478,4 +479,30 @@ public class ExecCompilerTest {
     runMethod.execute(0);
     assertThat(out.toString(), containsString(expectedErrMsg));
   }
+
+  @Test
+  public void resultOfConversionIsTypeChecked() throws Exception {
+    var module =
+        ctx.eval(
+            LanguageInfo.ID,
+            """
+        type First_Type
+        type Other_Type
+
+        First_Type.from (that:Other_Type) = 42
+        run value -> First_Type = Other_Type
+        """);
+    var runMethod = module.invokeMember(Module.EVAL_EXPRESSION, "run");
+    try {
+      var r = runMethod.execute(0);
+      fail("We don't expect any result, but exception: " + r);
+    } catch (PolyglotException ex) {
+      assertThat(
+          ex.getMessage().toLowerCase(),
+          AllOf.allOf(containsString("type"), containsString("error")));
+      var typeError = ex.getGuestObject();
+      assertEquals("Expected type", "First_Type", typeError.getMember("expected").toString());
+      assertEquals("Got wrong value", 42, typeError.getMember("actual").asInt());
+    }
+  }
 }
diff --git engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala
index fc6ddf9f78..606c839864 100644
--- engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala
+++ engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala
@@ -504,6 +504,7 @@ class IrToTruffle(
                             m.getFunction.getName,
                             fn.arguments,
                             fn.body,
+                            null,
                             effectContext,
                             true
                           )
@@ -536,6 +537,7 @@ class IrToTruffle(
                     fullMethodDefName,
                     fn.arguments,
                     fn.body,
+                    null,
                     effectContext,
                     true
                   )
@@ -712,6 +714,7 @@ class IrToTruffle(
                 methodDef.methodName.name,
                 fn.arguments,
                 fn.body,
+                ReadArgumentCheckNode.build(context, "conversion", toType),
                 None,
                 true
               )
@@ -1921,6 +1924,7 @@ class IrToTruffle(
       val initialName: String,
       val arguments: List[DefinitionArgument],
       val body: Expression,
+      val typeCheck: ReadArgumentCheckNode,
       val effectContext: Option[String],
       val subjectToInstrumentation: Boolean
     ) {
@@ -1949,7 +1953,13 @@ class IrToTruffle(
           case _ =>
             ExpressionProcessor.this.run(body, false, subjectToInstrumentation)
         }
-        (argExpressions.toArray, bodyExpr)
+
+        if (typeCheck == null) {
+          (argExpressions.toArray, bodyExpr)
+        } else {
+          val bodyWithCheck = ReadArgumentCheckNode.wrap(bodyExpr, typeCheck)
+          (argExpressions.toArray, bodyWithCheck)
+        }
       }
 
       private def computeSlots(): (
@@ -2039,7 +2049,7 @@ class IrToTruffle(
       binding: Boolean = false
     ): CreateFunctionNode = {
       val bodyBuilder =
-        new BuildFunctionBody(scopeName, arguments, body, None, false)
+        new BuildFunctionBody(scopeName, arguments, body, null, None, false)
       val fnRootNode = ClosureRootNode.build(
         language,
         scope,

@enso-bot
Copy link

enso-bot bot commented Jul 9, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-07-08):

Progress: - Fixing #10355 with #10468
- down to 33 failures: #10468 (comment)
- reported ensoup issue: #10476
- review of autoscoped First/Last: #10467 (review)
- review of shapeless: #10462 (review)
- closed eta-conversion: #10281 (comment)
- demo meetings
- standup
- meeting Dmitry It should be finished by 2024-07-12.

@JaroslavTulach JaroslavTulach moved this from ⚙️ Design to 👁️ Code review in Issues Board Jul 9, 2024
@enso-bot
Copy link

enso-bot bot commented Jul 10, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-07-09):

Progress: - analyzing benchmarks: #10468 (comment)

@enso-bot
Copy link

enso-bot bot commented Jul 11, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-07-10):

Progress: - checking benchmarks results: #10468 (comment)

@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Jul 11, 2024
@enso-bot
Copy link

enso-bot bot commented Jul 12, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-07-11):

Progress: - Merged: #10468

Discord
Discord is great for playing games and chilling with friends, or even building a worldwide community. Customize your own space to talk, play, and hang out.

@enso-bot
Copy link

enso-bot bot commented Jul 13, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-07-12):

Progress: - reported benchmarks regression issue: #10539

Discord
Discord is great for playing games and chilling with friends, or even building a worldwide community. Customize your own space to talk, play, and hang out.
Discord
Discord is great for playing games and chilling with friends, or even building a worldwide community. Customize your own space to talk, play, and hang out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler -libs Libraries: New libraries to be implemented
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants