-
Notifications
You must be signed in to change notification settings - Fork 328
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
base: develop
Are you sure you want to change the base?
Changes from all commits
8ec0feb
0a81afd
bfcd675
33c7e27
343673d
494f160
ca449ce
215d497
236822a
09889da
c29ce71
f20f0e2
e645b05
e64d35d
256ef29
1f3c7ec
4e7b52a
fb7ec2c
64e429c
38ca032
97b7d27
af3ba05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
||||||
## PRIVATE | ||||||
Convert the Not_Invokable error to a human-readable format. | ||||||
to_display_text : Text | ||||||
to_display_text self = "Type error: expected a function, but got "+self.target.to_display_text+"." | ||||||
to_display_text self = | ||||||
suffix = case self.cause of | ||||||
Nothing -> "" | ||||||
_ -> " Caused by: "+self.cause.to_display_text | ||||||
"Type error: expected a function, but got "+self.target.to_display_text+"."+suffix | ||||||
|
||||||
@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. | ||||||
Error argument_name | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove
Suggested change
possibly remove the documentation comment altogether. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want the constructor to be The |
||||||
|
||||||
## 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 | ||||||
|
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.Not_Invokable | ||||||
import Standard.Base.Metadata.Widget | ||||||
from Standard.Base.Logging import all | ||||||
|
@@ -27,9 +28,21 @@ 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 | ||||||
case annotation of | ||||||
# The annotation is a function that is taking the 'self' value and _maybe_ a cache parameter. | ||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you use
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the |
||||||
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") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
if was_not_expecting_cache_argument then caught_panic.payload.target else | ||||||
# If this is some other error, we want to rethrow it. | ||||||
Panic.throw caught_panic | ||||||
|
||||||
# The annotation is a constant, so we just return it. | ||||||
_ -> annotation | ||||||
|
||||||
annotations = argument_names.map (arg -> [arg, read_annotation arg]) | ||||||
annotations.to_json |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,7 +78,8 @@ private void initializeOversaturatedCallNode( | |
InvokeCallableNodeGen.create( | ||
postApplicationSchema.getOversaturatedArguments(), | ||
defaultsExecutionMode, | ||
argumentsExecutionMode); | ||
argumentsExecutionMode, | ||
true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the From inside of That's why this argument is added to be able to distinguish the situations at call-site. |
||
oversaturatedCallableNode.setTailStatus(getTailStatus()); | ||
} | ||
} | ||
|
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"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still an incompatible change for the creators of this object. To make the change compatible use:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried and unfortunately constructors of builtins do not like default arguments.
With the change above, the code
still prints
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 aUniquelyConstructibleBuiltin
.