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

Improve error reporting of named argument mismatch #12238

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,25 @@ type Not_Invokable
to_display_text : Text
to_display_text self = "Type error: expected a function, but got "+self.target.to_display_text+"."

@Builtin_Type
type No_Such_Argument
## PRIVATE
Indicates that an argument was passed by name, but the function being
called did not take any argument that matched that name.

Arguments:
- argument_name: The name of the argument that was not found.
- call_target: Advanced. The value that this argument was attempted to be
applied to. This may be an unapplied function, or a result of a
function/constructor call (if all other arguments have already been
applied). This is the same value as `target` in analogous `Not_Invokable` error.
Error argument_name call_target

## PRIVATE
Convert the No_Such_Argument error to a human-readable format.
to_display_text : Text
to_display_text self = "The named argument `"+self.argument_name.to_text+"` did not match any argument names. Perhaps it is misspelled?"

@Builtin_Type
type Private_Access
## PRIVATE
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from Standard.Base import all
import Standard.Base.Errors.Common.No_Such_Argument
import Standard.Base.Errors.Common.Type_Error
import Standard.Base.Errors.Illegal_Argument.Illegal_Argument
import Standard.Base.Network.HTTP.Response.Response
Expand Down Expand Up @@ -61,13 +62,14 @@ type Delimited_Format
@delimiter make_file_read_delimiter_selector
@encoding Encoding.default_widget
@row_limit Rows_To_Read.default_widget
Delimited (delimiter:Text=',') (encoding:Encoding=Encoding.default) (skip_rows:Integer=0) (row_limit:Rows_To_Read=..All_Rows) (quote_style:Quote_Style=Quote_Style.With_Quotes) (headers:Headers=Headers.Detect_Headers) (value_formatter:Data_Formatter|Nothing=Data_Formatter.Value) (on_invalid_rows:Invalid_Rows=Invalid_Rows.Add_Extra_Columns) (line_endings:Line_Ending_Style|Infer=Infer) (comment_character:Text|Nothing=Nothing)
Delimited (delimiter:Text=',') (encoding:Encoding=Encoding.default) (skip_rows:Integer=0) (row_limit:Rows_To_Read=..All_Rows) (quote_style:Quote_Style=..With_Quotes) (headers:Headers=..Detect_Headers) (value_formatter:Data_Formatter|Nothing=Data_Formatter.Value) (on_invalid_rows:Invalid_Rows=..Add_Extra_Columns) (line_endings:Line_Ending_Style|Infer=Infer) (comment_character:Text|Nothing=Nothing)

## PRIVATE
Resolve an unresolved constructor to the actual type.
resolve : Function -> Delimited_Format | Nothing
resolve constructor =
Panic.catch Type_Error (constructor:Delimited_Format) _->Nothing
_catch_compatibility_changes <|
Panic.catch Type_Error (constructor:Delimited_Format) _->Nothing

## PRIVATE
ADVANCED
Expand Down Expand Up @@ -210,3 +212,13 @@ Delimited_Format.from (that : JS_Object) =
Delimited_Format.Delimited delimiter=delimiter encoding=encoding headers=headers skip_rows=skip_rows row_limit=row_limit quote_style=quote_style on_invalid_rows=on_invalid_rows
field ->
Error.throw (Illegal_Argument.Error ("The field `" ++ field ++ "` is currently not supported when deserializing the Delimited format from JSON."))

private _catch_compatibility_changes ~action =
on_no_such_argument caught_panic =
# Handling the compatibility change from https://github.com/enso-org/enso/pull/12231
is_keep_invalid_rows = caught_panic.payload.argument_name == "keep_invalid_rows"
if is_keep_invalid_rows.not then
Panic.throw caught_panic
Error.throw (Illegal_Argument.Error "The `keep_invalid_rows` argument has been renamed to `on_invalid_rows`.")
Panic.catch No_Such_Argument handler=on_no_such_argument <|
action
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from Standard.Base import all
import Standard.Base.Errors.Common.Not_Invokable
import Standard.Base.Errors.Common.No_Such_Argument
import Standard.Base.Metadata.Widget
from Standard.Base.Logging import all

Expand Down Expand Up @@ -27,9 +27,12 @@ get_widget_json value call_name argument_names uuids="{}" =

read_annotation argument = Panic.catch Any handler=(log_panic argument) <|
annotation = Warning.clear <| Meta.get_annotation value call_name argument
return_target err = err.payload.target
Panic.catch Not_Invokable handler=return_target
annotation value cache=cache
Panic.catch No_Such_Argument (annotation value cache=cache) caught_panic->
radeusgd marked this conversation as resolved.
Show resolved Hide resolved
is_missing_cache = caught_panic.payload.argument_name == "cache"
if is_missing_cache then caught_panic.payload.call_target else
# If this is some other error, we want to rethrow it.
Panic.throw caught_panic


annotations = argument_names.map (arg -> [arg, read_annotation arg])
annotations.to_json
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,18 @@ public Object invokeGeneric(
InvokeCallableNode.DefaultsExecutionMode defaultsExecutionMode,
InvokeCallableNode.ArgumentsExecutionMode argumentsExecutionMode,
BaseNode.TailStatus isTail) {
Atom error = EnsoContext.get(this).getBuiltins().error().makeNotInvokable(callable);
throw new PanicException(error, this);
// The IndirectInvokeCallableNode is used only from IndirectCurryNode, so it is always used for
// oversaturated arguments as long as schema.length >= 1.
if (schema.length >= 1 && schema[0].isNamed()) {
Atom error =
EnsoContext.get(this)
.getBuiltins()
.error()
.makeNoSuchArgument(schema[0].getName(), callable);
throw new PanicException(error, this);
} else {
Atom error = EnsoContext.get(this).getBuiltins().error().makeNotInvokable(callable);
throw new PanicException(error, this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,16 @@ public boolean shouldExecute() {
private final int thatArgumentPosition;

private final ArgumentsExecutionMode argumentsExecutionMode;
private final CallArgumentInfo[] schema;
radeusgd marked this conversation as resolved.
Show resolved Hide resolved
private final boolean isForOversaturatedArguments;

InvokeCallableNode(
CallArgumentInfo[] schema,
DefaultsExecutionMode defaultsExecutionMode,
ArgumentsExecutionMode argumentsExecutionMode) {
ArgumentsExecutionMode argumentsExecutionMode,
boolean isForOversaturatedArguments) {
this.schema = schema;
this.isForOversaturatedArguments = isForOversaturatedArguments;
Integer thisArg = thisArgumentPosition(schema);
this.canApplyThis = thisArg != null;
this.thisArgumentPosition = thisArg == null ? -1 : thisArg;
Expand Down Expand Up @@ -166,7 +171,8 @@ public static InvokeCallableNode build(
CallArgumentInfo[] schema,
DefaultsExecutionMode defaultsExecutionMode,
ArgumentsExecutionMode argumentsExecutionMode) {
return InvokeCallableNodeGen.create(schema, defaultsExecutionMode, argumentsExecutionMode);
return InvokeCallableNodeGen.create(
schema, defaultsExecutionMode, argumentsExecutionMode, false);
}

@Specialization
Expand Down Expand Up @@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Atom error =
EnsoContext.get(this)
.getBuiltins()
.error()
.makeNoSuchArgument(schema[0].getName(), callable);
throw new PanicException(error, this);
} else {
Atom error = EnsoContext.get(this).getBuiltins().error().makeNotInvokable(callable);
throw new PanicException(error, this);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ private void initializeOversaturatedCallNode(
InvokeCallableNodeGen.create(
postApplicationSchema.getOversaturatedArguments(),
defaultsExecutionMode,
argumentsExecutionMode);
argumentsExecutionMode,
true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

oversaturatedCallableNode.setTailStatus(getTailStatus());
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.enso.interpreter.node.expression.builtin.error;

import java.util.List;
import org.enso.interpreter.dsl.BuiltinType;
import org.enso.interpreter.node.expression.builtin.UniquelyConstructibleBuiltin;

@BuiltinType
public class NoSuchArgument extends UniquelyConstructibleBuiltin {
@Override
protected String getConstructorName() {
return "Error";
}

@Override
protected List<String> getConstructorParamNames() {
return List.of("argument_name", "call_target");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.enso.interpreter.node.expression.builtin.error.ModuleDoesNotExist;
import org.enso.interpreter.node.expression.builtin.error.ModuleNotInPackageError;
import org.enso.interpreter.node.expression.builtin.error.NoConversionCurrying;
import org.enso.interpreter.node.expression.builtin.error.NoSuchArgument;
import org.enso.interpreter.node.expression.builtin.error.NoSuchConversion;
import org.enso.interpreter.node.expression.builtin.error.NoSuchField;
import org.enso.interpreter.node.expression.builtin.error.NoSuchMethod;
Expand Down Expand Up @@ -61,6 +62,7 @@ public final class Error {
private final UnsupportedArgumentTypes unsupportedArgumentsError;
private final ModuleDoesNotExist moduleDoesNotExistError;
private final NotInvokable notInvokable;
private final NoSuchArgument noSuchArgument;
private final PrivateAccess privateAccessError;
private final InvalidConversionTarget invalidConversionTarget;
private final NoSuchField noSuchField;
Expand Down Expand Up @@ -100,6 +102,7 @@ public Error(Builtins builtins, EnsoContext context) {
unsupportedArgumentsError = builtins.getBuiltinType(UnsupportedArgumentTypes.class);
moduleDoesNotExistError = builtins.getBuiltinType(ModuleDoesNotExist.class);
notInvokable = builtins.getBuiltinType(NotInvokable.class);
noSuchArgument = builtins.getBuiltinType(NoSuchArgument.class);
privateAccessError = builtins.getBuiltinType(PrivateAccess.class);
invalidConversionTarget = builtins.getBuiltinType(InvalidConversionTarget.class);
noSuchField = builtins.getBuiltinType(NoSuchField.class);
Expand Down Expand Up @@ -315,6 +318,17 @@ public Atom makeNotInvokable(Object target) {
return notInvokable.newInstance(target);
}

/**
* Constructs an error that indicates that a named argument application could not find a matching
* parameter.
*
* @param argumentName name of the named argument being applied
* @return a not invokable error
*/
public Atom makeNoSuchArgument(String argumentName, Object callTarget) {
return noSuchArgument.newInstance(Text.create(argumentName), callTarget);
}

/**
* @param thisProjectName Current project name. May be null.
* @param targetProjectName Target method project name. May be null.
Expand Down
19 changes: 18 additions & 1 deletion test/Base_Tests/src/Semantic/Default_Args_Spec.enso
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from Standard.Base import all
import Standard.Base.Errors.Common.No_Such_Argument
import Standard.Base.Errors.Common.Not_Invokable

from Standard.Test import all

Expand Down Expand Up @@ -86,8 +88,23 @@ add_specs suite_builder =
v1.at 2 . should_equal h.y
v1.at 3 . should_equal h.y

group_builder.specify "should return a helpful error when applying a named argument that does not match" <|
f a='a' b='b' c='c' =
[a, b, c]

r1 = Test.expect_panic No_Such_Argument (f y='Y')
r1.to_display_text . should_equal "The named argument `y` did not match any argument names. Perhaps it is misspelled?"

g a=1 = [a]
r2 = Test.expect_panic No_Such_Argument (g x=2)
r2.to_display_text . should_equal "The named argument `x` did not match any argument names. Perhaps it is misspelled?"

group_builder.specify "should return Not_Invokable if a non-function is called" <|
not_f = 42 : Any
Test.expect_panic Not_Invokable (not_f 1)
Test.expect_panic Not_Invokable (not_f x=1)

main filter=Nothing =
suite = Test.build suite_builder->
add_specs suite_builder
suite.run_with_filter filter

6 changes: 6 additions & 0 deletions test/Table_Tests/src/IO/Delimited_Read_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,12 @@ add_specs suite_builder =
r3.at 'b' . to_vector . should_equal ['2', '0', '5']
r3.at 'c' . to_vector . should_equal ['3', Nothing, '6']

group_builder.specify "should offer a helpful error when using the old argument name" <|
r1 = Data.read (enso_project.data / "varying_rows.csv") (..Delimited "," headers=True keep_invalid_rows=False value_formatter=Nothing)
r1.should_fail_with Illegal_Argument
r1.to_display_text.should_contain "keep_invalid_rows"
r1.to_display_text.should_contain "on_invalid_rows"

group_builder.specify "should aggregate invalid rows over some limit" <|
action on_problems =
Data.read (enso_project.data / "many_invalid_rows.csv") (..Delimited "," headers=True on_invalid_rows=False value_formatter=Nothing) on_problems
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ type Not_Invokable

to_display_text self = "Type error: expected a function, but got "+self.target.to_display_text+"."

@Builtin_Type
type No_Such_Argument
Error argument_name call_target

to_display_text self = "The named argument `"+self.argument_name.to_text+"` did not match any argument names. Perhaps it is misspelled?"
radeusgd marked this conversation as resolved.
Show resolved Hide resolved

@Builtin_Type
type Compile_Error
Error message
Expand Down
Loading