From c4b0ca8f6901c2daebcb832de523192223507fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Wa=C5=9Bko?= Date: Tue, 11 Jun 2024 19:11:03 +0200 Subject: [PATCH] Ensure type ascriptions are correctly transformed to facilitate checking of more complex return types (#10229) - Fixes #9980 - Adds some tests to ensure types like `|` or `&` (in addition to `!` from the ticket) correctly work in return type check. - Fixes a weird behaviour where we used to avoid processing type related IR transformations inside of type ascriptions. - Adds parentheses to type representations if they are more complex: `A | B & C` is unclear as it can either mean `A | (B & C)` or `(A | B) & C` which have different meanings. If we now have an operation with such nesting, the sub expressions are wrapped in parentheses to disambiguate. --- .../0.0.0-dev/src/Data/Text/Encoding.enso | 6 +- .../pass/desugar/OperatorToFunction.scala | 4 +- .../compiler/pass/resolve/TypeFunctions.scala | 18 +-- .../enso/interpreter/test/SignatureTest.java | 148 ++++++++++++++++++ .../pass/resolve/TypeSignaturesTest.scala | 21 ++- .../argument/ReadArgumentCheckNode.java | 48 +++++- .../interpreter/runtime/IrToTruffle.scala | 6 + 7 files changed, 224 insertions(+), 27 deletions(-) diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Encoding.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Encoding.enso index 60c0f28248..edeb5df713 100644 --- a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Encoding.enso +++ b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Encoding.enso @@ -89,7 +89,8 @@ type Encoding Convert an Encoding to it's corresponding Java Charset, if applicable. This method should be used in places not aware of special logic for the Default encoding. In such places, usage of Default encoding will be forbidden. - to_java_charset self -> Charset ! Illegal_Argument = + to_java_charset : Charset ! Illegal_Argument + to_java_charset self = self.to_java_charset_or_null.if_nothing <| warning = Illegal_Argument.Error "The Default encoding has been used in an unexpected place (e.g. Write operation). It will be replaced with UTF-8. Please specify the desired encoding explicitly)" Warning.attach warning (Encoding.utf_8.to_java_charset) @@ -97,7 +98,8 @@ type Encoding ## PRIVATE Convert an Encoding to it's corresponding Java Charset or null if it is the Default encoding. This method should only be used in places where a null Charset is expected - i.e. places aware of the Default encoding. - to_java_charset_or_null self -> Charset ! Illegal_Argument = case self of + to_java_charset_or_null : Charset | Nothing ! Illegal_Argument + to_java_charset_or_null self = case self of Encoding.Value charset_name -> Panic.catch UnsupportedCharsetException (Charset.forName charset_name) _-> Error.throw (Illegal_Argument.Error ("Unknown Character Set: " + charset_name)) diff --git a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala index 5b2e5a6d40..79ea2c1410 100644 --- a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala +++ b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala @@ -2,7 +2,7 @@ package org.enso.compiler.pass.desugar import org.enso.compiler.context.{InlineContext, ModuleContext} import org.enso.compiler.core.ir.expression.{Application, Operator} -import org.enso.compiler.core.ir.{Expression, Module, Type} +import org.enso.compiler.core.ir.{Expression, Module} import org.enso.compiler.pass.IRPass import org.enso.compiler.pass.analyse.{ AliasAnalysis, @@ -71,8 +71,6 @@ case object OperatorToFunction extends IRPass { inlineContext: InlineContext ): Expression = ir.transformExpressions { - case asc: Type.Ascription => - asc.copy(typed = runExpression(asc.typed, inlineContext)) case Operator.Binary(l, op, r, loc, passData, diag) => Application.Prefix( op, diff --git a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala index 45c40a4587..f6ac31cf07 100644 --- a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala +++ b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala @@ -106,16 +106,14 @@ case object TypeFunctions extends IRPass { * @return `expr`, with any typing functions resolved */ def resolveExpression(expr: Expression): Expression = { - expr.transformExpressions { - case asc: Type.Ascription => asc - case app: Application => - val result = resolveApplication(app) - app - .getMetadata(DocumentationComments) - .map(doc => - result.updateMetadata(new MetadataPair(DocumentationComments, doc)) - ) - .getOrElse(result) + expr.transformExpressions { case app: Application => + val result = resolveApplication(app) + app + .getMetadata(DocumentationComments) + .map(doc => + result.updateMetadata(new MetadataPair(DocumentationComments, doc)) + ) + .getOrElse(result) } } diff --git a/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/SignatureTest.java b/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/SignatureTest.java index d0ca155a6a..3eb6ce5f89 100644 --- a/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/SignatureTest.java +++ b/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/SignatureTest.java @@ -897,6 +897,33 @@ public class SignatureTest { assertFalse("false & false", compute.execute(false, false).asBoolean()); } + @Test + public void unionInArgument() throws Exception { + var uri = new URI("memory://binary.enso"); + var src = + Source.newBuilder( + "enso", + """ + from Standard.Base import all + foo (arg : Integer | Text) = arg + """, + uri.getAuthority()) + .uri(uri) + .buildLiteral(); + var module = ctx.eval(src); + + var ok1 = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo 42"); + assertEquals(42, ok1.asInt()); + var ok2 = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo 'Hi'"); + assertEquals("Hi", ok2.asString()); + try { + var v = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo []"); + fail("Expecting an error, not " + v); + } catch (PolyglotException ex) { + assertTypeError("`arg`", "Integer | Text", "Vector", ex.getMessage()); + } + } + @Test public void unresolvedReturnTypeSignature() throws Exception { final URI uri = new URI("memory://neg.enso"); @@ -1209,6 +1236,127 @@ public class SignatureTest { } } + @Test + public void returnTypeCheckErrorSignature() throws Exception { + final URI uri = new URI("memory://rts.enso"); + final Source src = + Source.newBuilder( + "enso", + """ + from Standard.Base import Integer + import Standard.Base.Errors.Illegal_State.Illegal_State + foo a -> Integer ! Illegal_State = + a+a + """, + uri.getAuthority()) + .uri(uri) + .buildLiteral(); + + var module = ctx.eval(src); + var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo"); + assertEquals(20, foo.execute(10).asInt()); + try { + var res = foo.execute("."); + fail("Expecting an exception, not: " + res); + } catch (PolyglotException e) { + assertContains("expected the result of `foo` to be Integer, but got Text", e.getMessage()); + } + } + + /** + * The `!` part in the type signature is currently only for documentation purposes - it is not + * checked. Other kinds of dataflow errors may be returned as well and this is not an error. In + * particular, we don't distinguish errors raised by the function from errors propagate from its + * inputs. + */ + @Test + public void returnTypeCheckErrorSignatureAllowsAllErrors() throws Exception { + final URI uri = new URI("memory://rts.enso"); + final Source src = + Source.newBuilder( + "enso", + """ + from Standard.Base import Integer, Error + import Standard.Base.Errors.Illegal_Argument.Illegal_Argument + import Standard.Base.Errors.Illegal_State.Illegal_State + + foo a -> Integer ! Illegal_State = + Error.throw (Illegal_Argument.Error "foo: "+a.to_text) + """, + uri.getAuthority()) + .uri(uri) + .buildLiteral(); + + var module = ctx.eval(src); + var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo"); + var result = foo.execute(10); + assertTrue(result.isException()); + assertContains("Illegal_Argument.Error 'foo: 10'", result.toString()); + } + + @Test + public void returnTypeCheckSum() throws Exception { + final URI uri = new URI("memory://rts.enso"); + final Source src = + Source.newBuilder( + "enso", + """ + from Standard.Base import Integer, Text + foo a -> Integer | Text = + a+a + """, + uri.getAuthority()) + .uri(uri) + .buildLiteral(); + + var module = ctx.eval(src); + var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo"); + assertEquals(20, foo.execute(10).asInt()); + assertEquals("..", foo.execute(".").asString()); + + try { + Object vec = new Integer[] {1, 2, 3}; + var res = foo.execute(vec); + fail("Expecting an exception, not: " + res); + } catch (PolyglotException e) { + assertContains( + "expected the result of `foo` to be Integer | Text, but got Vector", e.getMessage()); + } + } + + @Test + public void returnTypeCheckProduct() throws Exception { + final URI uri = new URI("memory://rts.enso"); + final Source src = + Source.newBuilder( + "enso", + """ + from Standard.Base import Integer, Text + type Clazz + Value a + Clazz.from (that : Integer) = Clazz.Value that + + foo a -> (Integer | Text) & Clazz = + a+a + """, + uri.getAuthority()) + .uri(uri) + .buildLiteral(); + + var module = ctx.eval(src); + var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo"); + assertEquals(20, foo.execute(10).asInt()); + + try { + var res = foo.execute("."); + fail("Expecting an exception, not: " + res); + } catch (PolyglotException e) { + assertContains( + "expected the result of `foo` to be (Integer | Text) & Clazz, but got Text", + e.getMessage()); + } + } + static void assertTypeError(String expArg, String expType, String realType, String msg) { assertEquals( "Type error: expected " + expArg + " to be " + expType + ", but got " + realType + ".", diff --git a/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/pass/resolve/TypeSignaturesTest.scala b/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/pass/resolve/TypeSignaturesTest.scala index 273e3a1725..7bff48d902 100644 --- a/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/pass/resolve/TypeSignaturesTest.scala +++ b/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/pass/resolve/TypeSignaturesTest.scala @@ -6,7 +6,6 @@ import org.enso.compiler.core.Implicits.AsMetadata import org.enso.compiler.core.ir.Expression import org.enso.compiler.core.ir.Function import org.enso.compiler.core.ir.Module -import org.enso.compiler.core.ir.Literal import org.enso.compiler.core.ir.expression.Application import org.enso.compiler.core.ir.expression.errors import org.enso.compiler.core.ir.module.scope.Definition @@ -265,14 +264,24 @@ class TypeSignaturesTest extends CompilerTest { "associate the signature with the typed expression" in { ir shouldBe an[Application.Prefix] - ir.getMetadata(TypeSignatures) shouldBe defined + val outerSignature = ir.getMetadata(TypeSignatures) + outerSignature shouldBe defined + outerSignature.get.signature.showCode() shouldEqual "Double" } "work recursively" in { - val arg2Value = ir.asInstanceOf[Application.Prefix].arguments(1).value - arg2Value shouldBe an[Application.Prefix] - val snd = arg2Value.asInstanceOf[Application.Prefix] - snd.arguments(0).value shouldBe an[Literal.Number] + val arg2Value = ir.asInstanceOf[Application.Prefix].arguments(1).value + val arg2Signature = arg2Value.getMetadata(TypeSignatures) + arg2Signature shouldBe defined + arg2Signature.get.signature.showCode() shouldEqual "Int" + + // But arg1 has no signature: + val arg1Signature = ir + .asInstanceOf[Application.Prefix] + .arguments(0) + .value + .getMetadata(TypeSignatures) + arg1Signature shouldBe empty } } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/argument/ReadArgumentCheckNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/argument/ReadArgumentCheckNode.java index d7e0911837..5813b1436d 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/argument/ReadArgumentCheckNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/argument/ReadArgumentCheckNode.java @@ -82,6 +82,38 @@ public abstract class ReadArgumentCheckNode extends Node { abstract String expectedTypeMessage(); + protected final String joinTypeParts(List parts, String separator) { + assert !parts.isEmpty(); + if (parts.size() == 1) { + return parts.get(0); + } + + var separatorWithSpace = " " + separator + " "; + var builder = new StringBuilder(); + boolean isFirst = true; + for (String part : parts) { + if (isFirst) { + isFirst = false; + } else { + builder.append(separatorWithSpace); + } + + // If the part contains a space, it means it is not a single type but already a more complex + // expression with a separator. + // So to ensure we don't mess up the expression layers, we need to add parentheses around it. + boolean needsParentheses = part.contains(" "); + if (needsParentheses) { + builder.append("("); + } + builder.append(part); + if (needsParentheses) { + builder.append(")"); + } + } + + return builder.toString(); + } + final PanicException panicAtTheEnd(Object v) { if (expectedTypeMessage == null) { CompilerDirectives.transferToInterpreterAndInvalidate(); @@ -195,9 +227,11 @@ public abstract class ReadArgumentCheckNode extends Node { @Override String expectedTypeMessage() { - return Arrays.stream(checks) - .map(n -> n.expectedTypeMessage()) - .collect(Collectors.joining(" & ")); + var parts = + Arrays.stream(checks) + .map(ReadArgumentCheckNode::expectedTypeMessage) + .collect(Collectors.toList()); + return joinTypeParts(parts, "&"); } } @@ -239,9 +273,11 @@ public abstract class ReadArgumentCheckNode extends Node { @Override String expectedTypeMessage() { - return Arrays.stream(checks) - .map(n -> n.expectedTypeMessage()) - .collect(Collectors.joining(" | ")); + var parts = + Arrays.stream(checks) + .map(ReadArgumentCheckNode::expectedTypeMessage) + .collect(Collectors.toList()); + return joinTypeParts(parts, "|"); } } diff --git a/engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala b/engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala index 4fe5b7d6eb..3e1c71f588 100644 --- a/engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala +++ b/engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala @@ -768,6 +768,12 @@ class IrToTruffle( comment, context.getTopScope().getBuiltins().function() ) + case typeWithError: Tpe.Error => + // When checking a `a ! b` type, we ignore the error part as it is only used for documentation purposes and is not checked. + extractAscribedType(comment, typeWithError.typed) + case typeInContext: Tpe.Context => + // Type contexts aren't currently really used. But we should still check the base type. + extractAscribedType(comment, typeInContext.typed) case t => { t.getMetadata(TypeNames) match { case Some(