-
Notifications
You must be signed in to change notification settings - Fork 321
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
Always call instance methods on Any #7033
Always call instance methods on Any #7033
Conversation
methods on Any have default `self` argument.
0ea6f75
to
8493dff
Compare
536e493
to
5dfdb69
Compare
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 don't think such manipulation is the right approach but I will defer to @kustosz to have the last word on this.
newInvokeFuncSchema[0] = new CallArgumentInfo("self"); | ||
newInvokeFuncSchema[1] = new CallArgumentInfo("self"); | ||
int toCopyLen = Math.min(newInvokeFuncSchema.length - 2, invokeFuncSchema.length); | ||
System.arraycopy(invokeFuncSchema, 0, newInvokeFuncSchema, 2, toCopyLen); |
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.
Last time I tried to do similar manipulation of arguments I was (rightfully) reminded that this approach is really bad and sounds like a hack, not to mention perf penalty.
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 don't think either, this is an elegant solution. It's the only one that I was able to come up with. Why do you think there will be any perf penalty? The penalty is paid just in the very first invocation of such a special static method invocation on Any
, which will construct invokeAnyFunctionNode
. Once this node is constructed, all the subsequent calls should have negligible perf penalty.
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.
Nevertheless, I will be more than happy for any other suggestions
str ++= fansi | ||
.Str(fileLocationFromSection(diagnostic.location, source)) | ||
.Str(fileLocation) |
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.
Unrelated change. This fixes the compiler diagnostic output when there is no source available. It used to print path:: error
, i.e., two consecutive colons. Which was weird.
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 am not completely sure I get the implementation but the "observable effect" - e.g. how it looks to the are relatively fine. I'd like to see behavior of an Any
method with argument however - we have to make sure that non-self arguments are applied first, and only when self=something
is used, we specify the self
argument.
var resolvedFuncArgCount = function.getSchema().getArgumentsCount(); | ||
CallArgumentInfo[] invokeFuncSchema = invokeFunctionNode.getSchema(); | ||
// Check if this is a static method call on Any | ||
if (isAnyEigenType(selfTpe) && arguments.length == resolvedFuncArgCount - 1) { |
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 think this is correct. We want to find out if a method is defined on Any
and treat it in a special way.
engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java
Show resolved
Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java
Outdated
Show resolved
Hide resolved
Any.x self=With_X . should_equal "Any:With_X" | ||
Any.x self=With_Y . should_equal "Any:With_Y" | ||
Any.x (My_Type.Value 22) . should_equal "Any:(My_Type.Value 22)" | ||
Any.x (With_X.Value 22) . should_equal "Any:(With_X.Value 22)" | ||
Any.x (With_Y.Value 22) . should_equal "Any:With_Y(22)" | ||
Date.to_display_text . should_equal "Date" | ||
|
||
Test.specify "static method calls on Any should have defaulted self argument to Any" <| |
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.
Regardless of my other comments, these tests are the most important thing. If they work & define the expected behavior, then we have a system we wanted.
# Any.to_text is a method that takes one argument (self) | ||
Any.to_text . should_equal "Any" | ||
Any.to_text self=Any . should_equal "Any" | ||
Any.to_text self=Vector . should_equal "Vector" |
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.
+1
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.
The Enso changes look good to me.
Please add a few more test cases that were requested.
Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
Meta.should_be_a (Any.spec_method Boolean) Function | ||
Meta.should_be_a (Any.spec_method self=Boolean) Function | ||
Meta.should_be_a (Any.spec_method self=Boolean Vector) Function | ||
Any.spec_method Boolean Vector . should_equal "Any.spec_method:{Any}{Boolean}{Vector}" |
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 correct. self
is defaulted and Boolean
and Vector
are the two arguments.
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.
Exactly. This is the most important difference from "normal" static method invocation.
Meta.should_be_a (Any.spec_method self=Boolean Vector) Function | ||
Any.spec_method Boolean Vector . should_equal "Any.spec_method:{Any}{Boolean}{Vector}" | ||
Any.spec_method Any Boolean Vector . should_equal "Any.spec_method:{Any}{Boolean}{Vector}" | ||
Any.spec_method Date Boolean Vector . should_equal "Any.spec_method:{Date}{Boolean}{Vector}" |
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 am surprised this works and self=Date
- but probably that is OKeyish behavior.
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.
Indeed, I think it could be clearer if the self
always had to be named if specified like this. What do you think?
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.
But this is the same semantics as for any other static method, e.g.:
> Vector.contains [42] 42
>>> true
> Vector.contains self=[42] 42
>>> true
I am surprised you find it confusing. To me, this is the least confusing part.
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.
Ah, right!
Fixes #5612 and #6473.
Fixes some static method invocations on
Any
, likeAny == Boolean
, orAny.to_text
.Pull Request Description
Previously, static method calls on
Any
have not worked as expected. For example,Any.to_text
returned Function instead of Text. That is because the function resolution forAny.to_text
findsAny.type.to_text
method on eigentype which expects twoself
arguments, but only one argument is provided.Note that
Boolean.to_text
worked previously, and returned "Boolean" as expected. This is because the method resolution findsAny.to_text
method that takes just oneself
argument.This PR solves this issue by introducing special handling for static method dispatch on
Any
. Simply put, an additionalself
argument is prepended to the argument list.Important Notes
A new child node is introduced to
InvokeMethodNode
. This child node is a copy of the currentinvokeFunctionNode
with one moreCallArgumentInfo
in its schema.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.