Improve error reporting of named argument mismatch #12238

radeusgd · 2025-02-05T13:40:05Z

Pull Request Description

Related to further work discussed in Confusing handling of named arguments #7359

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.
If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
or the Snowflake database integration, a run of the Extra Tests has been scheduled.
- If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

…rgument rename

JaroslavTulach

adventurous: I am afraid to touch CurryNode & co. myself
changing the type of thrown panic is an incompatible change
- better to keep the atom and add an info field to it
are the benchmarks result OK?
- Probably they are (as these are just exceptional paths and they are unlikely tested).
one minor @CompilationFinal note - otherwise looks decent

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

JaroslavTulach · 2025-02-06T03:55:46Z

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

@@ -78,7 +78,8 @@ private void initializeOversaturatedCallNode(
          InvokeCallableNodeGen.create(
              postApplicationSchema.getOversaturatedArguments(),
              defaultsExecutionMode,
-              argumentsExecutionMode);
+              argumentsExecutionMode,
+              true);


Why do we need new boolean argument? Isn't postApplicationSchema.getOversaturatedArguments() good enough indicator that there are oversaturated arguments?

But the create method/constructor does not see postApplicationSchema, it sees the result of postApplicationSchema.getOversaturatedArguments() which is just a CallArgumentInfo[] schema.

From inside of InvokeCallableNodeGen.create you can no longer tell if the arguments being passed were 'oversaturated' or 'normal' - you just get some schema, that is the same kind as in all different places where InvokeCallableNode is constructed.

That's why this argument is added to be able to distinguish the situations at call-site.

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

JaroslavTulach · 2025-02-06T04:01:28Z

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

@@ -375,8 +381,17 @@ static Object doPolyglot(
  @Fallback
  public Object invokeGeneric(
      Object callable, VirtualFrame callerFrame, State state, Object[] arguments) {
-    Atom error = EnsoContext.get(this).getBuiltins().error().makeNotInvokable(callable);
-    throw new PanicException(error, this);
+    if (isForOversaturatedArguments && schema.length >= 1 && schema[0].isNamed()) {


Is this a compilation constant? Try CompilerAsserts.partialEvaluationContant or co.

Tried in e645b05, is that how it is supposed to be used?

distribution/lib/Standard/Visualization/0.0.0-dev/src/Widgets.enso

…formation

radeusgd · 2025-02-06T13:07:46Z

Double checked in the GUI:

The widgets that rely on cache seem to all work fine - so the tweaking of the annotation invocation code seems to be all good.

radeusgd · 2025-02-06T13:08:12Z

are the benchmarks result OK?

Probably they are (as these are just exceptional paths and they are unlikely tested).

I will schedule a run once I ensure that the tests are passing.

radeusgd · 2025-02-06T15:30:53Z

JaroslavTulach · 2025-02-07T10:10:18Z

distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso

+
+       Arguments:
+       - argument_name: The name of the argument that was not found.
+    Error argument_name


Remove ## PRIVATE use:

Suggested change

Error argument_name

private Error argument_name

possibly remove the documentation comment altogether.

This is against the convention that we are using for all the errors.

I think changes in this PR should be consistent with the rest of the codebase - if we want to change the convention let's discuss and consider rewriting the whole Common.enso file.

We don't want the constructor to be private, this breaks the very simple check I've implemented like payload.cause == (No_Such_Argument.Error "foo"), at least outside of Base.

The PRIVATE is to avoid cluttering Component Browser only, I guess at some point we may remove that tag from errors.

JaroslavTulach · 2025-02-07T10:11:10Z

distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso

@@ -310,12 +310,32 @@ type Not_Invokable

       Arguments:
       - target: The called object.
-    Error target
+       - cause: Additional information about what may have caused the error.
+    Error target cause


This is still an incompatible change for the creators of this object. To make the change compatible use:

Suggested change

Error target cause

Error target cause=Nothing

I've tried and unfortunately constructors of builtins do not like default arguments.

With the change above, the code

from Standard.Base import all import Standard.Base.Errors.Common.Not_Invokable main = without_cause = Not_Invokable.Error "thing" IO.println without_cause

still prints

Not_Invokable.Error target='thing' cause=_

I think the defaultness has to be handled in engine and we don't have capability to do that.

Technically, even with the default argument this change would still breaking as any pattern matching of Not_Invokable.Error _ also will now mismatch the amount of fields.

We could try to have 2 constructors for Not_Invokable - the old one and the new one. But I think this is complicating stuff unnecessarily. Also not sure if the builtins machinery would currently allow this, as it would no longer be a UniquelyConstructibleBuiltin.

JaroslavTulach · 2025-02-07T10:20:33Z

distribution/lib/Standard/Visualization/0.0.0-dev/src/Widgets.enso

+                    ## If the cache argument was not expected, we ignore the problem
+                       and return just the result of `f value` - held by
+                       the target of the Not_Invokable error.
+                    was_not_expecting_cache_argument = caught_panic.payload.cause == (No_Such_Argument.Error "cache")


Question to myself: Why do I find this unreadable? Would following be better?

f : Function -> Panic.catch Not Invocable (f value cache=cache) caught_panic -> case caught.panic.payload.cause of No_Such_Argument.Error "cache" -> caught_panic.payload.target _ -> Panic.throw caught_panic

Maybe, but caught.panic.payload.cause is still quite long...

JaroslavTulach · 2025-02-07T10:21:14Z

distribution/lib/Standard/Visualization/0.0.0-dev/src/Widgets.enso

+            f : Function ->
+                # We pass the self value and the cache to the function.
+                Panic.catch Not_Invokable (f value cache=cache) caught_panic->
+                    ## If the cache argument was not expected, we ignore the problem


Why do you use ## instead of # here?

Suggested change

## If the cache argument was not expected, we ignore the problem

# If the cache argument was not expected, we ignore the problem

Because the ## allows to write a multi-line comment, and I don't want to create lines 300+ characters long.

JaroslavTulach · 2025-02-07T10:23:35Z

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

@@ -375,7 +385,16 @@ static Object doPolyglot(
  @Fallback
  public Object invokeGeneric(
      Object callable, VirtualFrame callerFrame, State state, Object[] arguments) {
-    Atom error = EnsoContext.get(this).getBuiltins().error().makeNotInvokable(callable);
+    Atom cause = null;


The same code is already present in IndirectInvokeCallableNode once. Can we refactor it to a single method?

Hmmm, perhaps a good point.

Any suggestions where this common function could live? Put it as static in InvokeCallableNode and call from IndirectInvokeCallableNode?

Done in 97b7d27

This reverts commit 38ca032.

radeusgd · 2025-02-07T10:56:59Z

Running benchmarks:

GitHub
Benchmark Engine · enso-org/enso@af3ba05
Enso Analytics is a self-service data prep and analysis platform designed for data teams. - Benchmark Engine · af3ba05

GitHub
Benchmark Standard Libraries · enso-org/enso@e6af654
Enso Analytics is a self-service data prep and analysis platform designed for data teams. - Benchmark Standard Libraries · e6af654

radeusgd added 5 commits February 5, 2025 18:10

Separate error for named argument mismatch

8ec0feb

CR from previous PR - use autoscoping

0a81afd

include call_target in No_Such_Argument, like Not_Invokable

bfcd675

refactor Widgets hack to be more precise catching the error

33c7e27

intercept No_Such_Argument and provide a better error for Delimited a…

343673d

…rgument rename

radeusgd force-pushed the wip/radeusgd/improve-mismatched-named-argument-error branch from c71e95c to 343673d Compare February 5, 2025 17:37

add tests

494f160

radeusgd marked this pull request as ready for review February 5, 2025 18:12

radeusgd requested review from jdunkerley, GregoryTravis, AdRiley, marthasharkey, 4e6, JaroslavTulach, hubertp and Akirathan as code owners February 5, 2025 18:12

radeusgd added 3 commits February 5, 2025 19:15

reorder

ca449ce

javafmt

215d497

also handle the indirect case

236822a

jdunkerley approved these changes Feb 5, 2025

View reviewed changes

JaroslavTulach reviewed Feb 6, 2025

View reviewed changes

radeusgd added 7 commits February 6, 2025 13:11

CR: revert to Not_Invokable error for compatibility, but add cause in…

09889da

…formation

after CR: adapt my changes to the changed API

c29ce71

CR: CompilationFinal array

f20f0e2

CR: assert compilation constant

e645b05

shorter error message

e64d35d

refactor and document annotation invocation logic

256ef29

fix changelog

1f3c7ec

radeusgd changed the title ~~Separate error for named argument mismatch~~ Improve error reporting of named argument mismatch Feb 6, 2025

radeusgd self-assigned this Feb 6, 2025

add to changelog

4e7b52a

radeusgd requested a review from JaroslavTulach February 6, 2025 13:08

update message to conform with test

fb7ec2c

javafmt

64e429c

GregoryTravis approved these changes Feb 6, 2025

View reviewed changes

enso-bot bot mentioned this pull request Feb 7, 2025

Dispatch of to_text, is_a & co. on intersection types #12192

Merged

2 tasks

JaroslavTulach reviewed Feb 7, 2025

View reviewed changes

JaroslavTulach approved these changes Feb 7, 2025

View reviewed changes

radeusgd added 2 commits February 7, 2025 11:37

CR: make cause default to Nothing

38ca032

CR: extract common logic

97b7d27

Akirathan approved these changes Feb 7, 2025

View reviewed changes

Revert "CR: make cause default to Nothing"

af3ba05

This reverts commit 38ca032.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error reporting of named argument mismatch #12238

Improve error reporting of named argument mismatch #12238

radeusgd commented Feb 5, 2025 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Feb 6, 2025

radeusgd Feb 6, 2025

JaroslavTulach Feb 6, 2025

radeusgd Feb 6, 2025

radeusgd commented Feb 6, 2025

radeusgd commented Feb 6, 2025

radeusgd commented Feb 6, 2025

JaroslavTulach Feb 7, 2025

radeusgd Feb 7, 2025

radeusgd Feb 7, 2025

JaroslavTulach Feb 7, 2025

radeusgd Feb 7, 2025

radeusgd Feb 7, 2025

JaroslavTulach Feb 7, 2025

JaroslavTulach Feb 7, 2025

radeusgd Feb 7, 2025

JaroslavTulach Feb 7, 2025

radeusgd Feb 7, 2025

radeusgd Feb 7, 2025

radeusgd commented Feb 7, 2025 •

edited by unfurl-links bot

Loading

	## If the cache argument was not expected, we ignore the problem
	# If the cache argument was not expected, we ignore the problem

Improve error reporting of named argument mismatch #12238

Are you sure you want to change the base?

Improve error reporting of named argument mismatch #12238

Conversation

radeusgd commented Feb 5, 2025 • edited Loading

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

radeusgd commented Feb 6, 2025

radeusgd commented Feb 6, 2025

radeusgd commented Feb 6, 2025

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

radeusgd commented Feb 7, 2025 • edited by unfurl-links bot Loading

radeusgd commented Feb 5, 2025 •

edited

Loading

radeusgd commented Feb 7, 2025 •

edited by unfurl-links bot

Loading