From 20e18d22dfceb470b9cea89582f5620a88bedef5 Mon Sep 17 00:00:00 2001 From: Jaroslav Tulach Date: Thu, 24 Aug 2023 20:04:08 +0200 Subject: [PATCH] More descriptive function information (#7629) Fixes #7359 by printing more information about the function including partially applied arguments and over-saturated arguments. --- .../Base/0.0.0-dev/src/Errors/Common.enso | 23 ++-- .../test/instrument/ReplTest.scala | 2 +- .../runtime/callable/atom/Atom.java | 5 +- .../callable/atom/AtomConstructor.java | 9 +- .../runtime/callable/function/Function.java | 45 ++++++- .../enso/interpreter/test/SignatureTest.java | 110 ++++++++++++++++++ test/Tests/src/Semantic/Error_Spec.enso | 65 +++++++++++ 7 files changed, 245 insertions(+), 14 deletions(-) diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso index 4b0d723d6f8..ebafe8c7844 100644 --- a/distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso +++ b/distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso @@ -4,6 +4,8 @@ import project.Errors.Common.Arithmetic_Error import project.Meta import project.Nothing.Nothing import project.Panic.Panic +import project.Function.Function +import project.Data.Text.Extensions from project.Data.Boolean import Boolean, False, True @@ -58,16 +60,23 @@ type Type_Error - name: The name of the argument whose type is mismatched. Error expected actual name - ## PRIVATE - Infer the type of the actual value in a human-readable format. - type_of_actual self = - tpe = Meta.type_of self.actual - if tpe.is_error then self.actual.to_display_text else tpe.to_display_text - ## PRIVATE Convert the Type_Error error to a human-readable format. to_display_text : Text - to_display_text self = "Type error: expected `"+self.name+"` to be "+self.expected.to_display_text+", but got "+self.type_of_actual+"." + to_display_text self = + type_of_actual = + tpe = Meta.type_of self.actual + if tpe.is_error then self.actual.to_display_text else case self.actual of + fn:Function -> + fn_info = fn.to_text + missing_args = fn_info.split " " . filter (_.ends_with "=_") . map (_.split "=") . map (_.at 0) + fn_info + case missing_args.length of + 0 -> "" + 1 -> ". Try to apply " + (missing_args.at 0) + " argument" + _ -> ". Try to apply " + (missing_args.join ", ") + " arguments" + _ -> tpe.to_display_text + + "Type error: expected `"+self.name+"` to be "+self.expected.to_display_text+", but got "+type_of_actual+"." @Builtin_Type type Compile_Error diff --git a/engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/ReplTest.scala b/engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/ReplTest.scala index b885d0f7773..ab000a724f0 100644 --- a/engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/ReplTest.scala +++ b/engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/ReplTest.scala @@ -102,7 +102,7 @@ class ReplTest result.toString shouldEqual "Error in method `to_text` of [Bar 1]: Expected Text but got 42" } inside(executor.evaluate("C.Baz 1")) { case Right(result) => - result.toString shouldEqual "Error in method `to_text` of [Baz 1]: Expected Text but got C.to_text[Test:18-40]" + result.toString shouldEqual "Error in method `to_text` of [Baz 1]: Expected Text but got C.to_text[Test:18:1-29]" } inside(executor.evaluate("Pattern.compile 'foo'")) { case Right(result) => diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/Atom.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/Atom.java index e851976cddf..5cbff728765 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/Atom.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/Atom.java @@ -116,7 +116,10 @@ public abstract class Atom implements EnsoObject { sb.append(suffix); } if (obj != null) { - var errorMessage = InteropLibrary.getUncached().toDisplayString(obj); + var errorMessage = switch (obj) { + case Function fn -> fn.toString(false); + default -> InteropLibrary.getUncached().toDisplayString(obj); + }; if (errorMessage != null) { sb.append(errorMessage); } else { diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/AtomConstructor.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/AtomConstructor.java index 897c047185d..0f2fbcf3589 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/AtomConstructor.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/AtomConstructor.java @@ -318,7 +318,14 @@ public final class AtomConstructor implements EnsoObject { @ExportMessage @TruffleBoundary String toDisplayString(boolean allowSideEffects) { - return "Constructor<" + getDisplayName() + ">"; + var sb = new StringBuilder(); + sb.append("Constructor<").append(getDisplayName()).append(">"); + for (var f : getFields()) { + if (!f.hasDefaultValue()) { + sb.append(" ").append(f.getName()).append("=_"); + } + } + return sb.toString(); } /** @return the fully qualified name of this constructor. */ diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java index 0d889d47219..3b2ab63f773 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java @@ -402,16 +402,53 @@ public final class Function implements EnsoObject { } @Override - @CompilerDirectives.TruffleBoundary public String toString() { + return toString(true); + } + + @CompilerDirectives.TruffleBoundary + public final String toString(boolean includeArguments) { var n = callTarget.getRootNode(); var ss = n.getSourceSection(); if (ss == null) { return super.toString(); } - var s = ss.getSource(); + var iop = InteropLibrary.getUncached(); + var src = ss.getSource(); var start = ss.getStartLine(); - final int end = start + s.getLineCount(); - return n.getName() + "[" + s.getName() + ":" + start + "-" + end + "]"; + var end = ss.getEndLine(); + var sb = new StringBuilder(); + sb.append(n.getName()); + sb.append("[").append(src.getName()).append(":").append(start); + if (end == start) { + sb.append(":").append(ss.getStartColumn()).append("-").append(ss.getEndColumn()); + } else { + sb.append("-").append(end); + } + sb.append("]"); + if (includeArguments) { + for (var i = 0; i < schema.getArgumentsCount(); i++) { + ArgumentDefinition info = schema.getArgumentInfos()[i]; + if (info.hasDefaultValue()) { + continue; + } + var name = info.getName(); + sb.append(" ").append(name).append("="); + if (preAppliedArguments != null && preAppliedArguments[i] != null) { + sb.append(iop.toDisplayString(preAppliedArguments[i], false)); + } else { + sb.append("_"); + } + } + if (schema.getOversaturatedArguments() != null) { + for (var i = 0; i < schema.getOversaturatedArguments().length; i++) { + if (oversaturatedArguments != null && oversaturatedArguments[i] != null) { + sb.append(" +").append(schema.getOversaturatedArguments()[i].getName()).append("="); + sb.append(iop.toDisplayString(oversaturatedArguments[i], false)); + } + } + } + } + return sb.toString(); } } diff --git a/engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java b/engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java index 3c0dca07b94..6ec4136f48b 100644 --- a/engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java +++ b/engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java @@ -2,6 +2,7 @@ package org.enso.interpreter.test; import java.net.URI; import java.net.URISyntaxException; + import org.graalvm.polyglot.Context; import org.graalvm.polyglot.PolyglotException; import org.graalvm.polyglot.Source; @@ -353,6 +354,109 @@ public class SignatureTest extends TestBase { assertEquals("binary.Bin", ok3.getMetaObject().getMetaQualifiedName()); } + @Test + public void partiallyAppliedConstructor() throws Exception { + final URI uri = new URI("memory://partial.enso"); + final Source src = Source.newBuilder("enso", """ + from Standard.Base import Integer + + type V + Val a b c + + create x:V = x.a + x.b + x.c + + mix a = + partial = V.Val 1 a + create partial + """, uri.getHost()) + .uri(uri) + .buildLiteral(); + + var module = ctx.eval(src); + var mix = module.invokeMember("eval_expression", "mix"); + + try { + var res = mix.execute(7); + fail("No result expected: " + res); + } catch (PolyglotException ex) { + assertContains("Type error", ex.getMessage()); + assertContains("expected `x` to be V", ex.getMessage()); + assertContains("got V.Val[partial", ex.getMessage()); + assertContains("a=1", ex.getMessage()); + assertContains("b=7", ex.getMessage()); + assertContains("c=_", ex.getMessage()); + } + } + + @Test + public void oversaturatedFunction() throws Exception { + final URI uri = new URI("memory://oversaturated.enso"); + final Source src = Source.newBuilder("enso", """ + from Standard.Base import Integer + + fn a b c = + sum = a + b + c + add a = sum + a + add + + neg x:Integer = -x + + mix n = neg (fn 2 a=4 n) + """, uri.getHost()) + .uri(uri) + .buildLiteral(); + + var module = ctx.eval(src); + var mix = module.invokeMember("eval_expression", "mix"); + + try { + var res = mix.execute(7); + fail("No result expected: " + res); + } catch (PolyglotException ex) { + assertContains("Type error", ex.getMessage()); + assertContains("expected `x` to be Integer", ex.getMessage()); + assertContains("got oversaturated.fn[", ex.getMessage()); + assertContains("a=2", ex.getMessage()); + assertContains("b=7", ex.getMessage()); + assertContains("c=_", ex.getMessage()); + assertContains("+a=4", ex.getMessage()); + } + } + + @Test + public void suspendedArgumentsUnappliedFunction() throws Exception { + final URI uri = new URI("memory://suspended.enso"); + final Source src = Source.newBuilder("enso", """ + from Standard.Base import Integer + + fn ~a ~b ~c = + add x = if x == 0 then 0 else x * (a + b + c) + add + + neg x:Integer = -x + + mix a = neg (fn c=(2/0) b=(a/0)) + """, uri.getHost()) + .uri(uri) + .buildLiteral(); + + var module = ctx.eval(src); + var mix = module.invokeMember("eval_expression", "mix"); + + try { + var res = mix.execute(0); + fail("No result expected: " + res); + } catch (PolyglotException ex) { + assertContains("Type error", ex.getMessage()); + assertContains("expected `x` to be Integer", ex.getMessage()); + assertContains("got suspended.fn[", ex.getMessage()); + assertContains("a=_", ex.getMessage()); + assertContains("b=suspended.mix", ex.getMessage()); + assertContains("c=suspended.mix", ex.getMessage()); + assertContains("[suspended:9:28-30]", ex.getMessage()); + } + } + private static void assertTypeError(String expArg, String expType, String realType, String msg) { if (!msg.contains(expArg)) { fail("Expecting value " + expArg + " in " + msg); @@ -364,4 +468,10 @@ public class SignatureTest extends TestBase { fail("Expecting value " + realType + " in " + msg); } } + + private static void assertContains(String exp, String msg) { + if (!msg.contains(exp)) { + fail("Expecting " + msg + " to contain " + exp); + } + } } diff --git a/test/Tests/src/Semantic/Error_Spec.enso b/test/Tests/src/Semantic/Error_Spec.enso index fddc4d16c9c..9cf4a701e27 100644 --- a/test/Tests/src/Semantic/Error_Spec.enso +++ b/test/Tests/src/Semantic/Error_Spec.enso @@ -1,5 +1,6 @@ from Standard.Base import all import Standard.Base.Errors.Common.Unsupported_Argument_Types +import Standard.Base.Errors.Common.Type_Error import Standard.Base.Errors.Illegal_Argument.Illegal_Argument import Standard.Base.Errors.Illegal_State.Illegal_State @@ -16,6 +17,8 @@ import Standard.Test.Extensions type My_Type Value foo + Multi_Value foo bar + Default_Value foo=2 bar throw_a_bar = Error.throw "bar" throw_a_bar_panicking = Panic.throw "bar" @@ -341,4 +344,66 @@ spec = c2 . should_equal "finalizer" v2.to_vector . should_equal [1, 2] + Test.group "Type Errors" <| + my_func x y = + x + y + + my_defaulted_func x=5 y = + x + y + + neg n:Number = -n + + extract x:My_Type = x.foo + + Test.specify "everything is ok" <| + neg (my_func -5 -2) . should_equal 7 + + Test.specify "try to apply one argument" <| + r = Panic.recover Type_Error <| neg (my_func -5) + r . should_fail_with Type_Error + r.to_display_text.should_contain "Try to apply y argument." + + Test.specify "try to apply two arguments" <| + r = Panic.recover Type_Error <| neg my_func + r . should_fail_with Type_Error + r.to_display_text.should_contain "Try to apply x, y arguments." + + Test.specify "apply two arguments with one defaulted" <| + r = Panic.recover Type_Error <| neg my_defaulted_func + r . should_fail_with Type_Error + r.to_display_text.should_contain "Try to apply y argument." + + Test.specify "report unapplied constructor nicely" <| + r = Panic.recover Type_Error <| extract My_Type.Value + r . should_fail_with Type_Error + r.to_display_text.should_contain "Try to apply foo argument." + + Test.specify "report unapplied constructor with default value nicely" <| + r = Panic.recover Type_Error <| extract My_Type.Default_Value + r . should_fail_with Type_Error + r.to_display_text.should_contain "Try to apply bar argument." + + Test.specify "report partially applied constructor nicely" <| + r = Panic.recover Type_Error <| extract (My_Type.Multi_Value 42) + r . should_fail_with Type_Error + r.to_display_text.should_contain "Try to apply bar argument." + + Test.specify "try to apply two arguments with over-saturated" <| + r = Panic.recover Type_Error <| neg (my_func z=10) + r . should_fail_with Type_Error + r.to_display_text . should_contain "Try to apply x, y arguments" + + Test.specify "types and unapplied arguments" <| + c = C.Baz C.to_text + r = Panic.recover Type_Error <| neg (c.to_num c=3) + r . should_fail_with Type_Error + r.to_display_text . should_contain "Try to apply a, b arguments" + + + +type C + Baz x + +C.to_num self a b c = a+b+c + main = Test_Suite.run_main spec