Always call instance methods on Any #7033

Akirathan · 2023-06-14T17:02:26Z

Fixes some static method invocations on Any, like Any == Boolean, or Any.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 for Any.to_text finds Any.type.to_text method on eigentype which expects two self 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 finds Any.to_text method that takes just one self argument.

This PR solves this issue by introducing special handling for static method dispatch on Any. Simply put, an additional self 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 current invokeFunctionNode with one more CallArgumentInfo in its schema.

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,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

methods on Any have default `self` argument.

hubertp

I don't think such manipulation is the right approach but I will defer to @kustosz to have the last word on this.

hubertp · 2023-06-21T18:45:45Z

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

+        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);


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.

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.

Nevertheless, I will be more than happy for any other suggestions

test/Tests/src/Semantic/Any_Spec.enso

…-any

Akirathan · 2023-06-22T14:04:46Z

engine/runtime/src/main/scala/org/enso/compiler/Compiler.scala

          str ++= fansi
-            .Str(fileLocationFromSection(diagnostic.location, source))
+            .Str(fileLocation)


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.

JaroslavTulach

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.

JaroslavTulach · 2023-06-22T14:22:55Z

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

+    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) {


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

JaroslavTulach · 2023-06-22T14:35:50Z

test/Tests/src/Semantic/Any_Spec.enso

            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" <|


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.

JaroslavTulach · 2023-06-22T14:36:51Z

test/Tests/src/Semantic/Any_Spec.enso

+            # 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"


test/Tests/src/Semantic/Any_Spec.enso

radeusgd

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>

JaroslavTulach · 2023-06-23T07:48:40Z

test/Tests/src/Semantic/Any_Spec.enso

+            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}"


This is correct. self is defaulted and Boolean and Vector are the two arguments.

Exactly. This is the most important difference from "normal" static method invocation.

JaroslavTulach · 2023-06-23T07:50:50Z

test/Tests/src/Semantic/Any_Spec.enso

+            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}"


I am surprised this works and self=Date - but probably that is OKeyish behavior.

Indeed, I think it could be clearer if the self always had to be named if specified like this. What do you think?

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.

…ents

…-any

Akirathan added 2 commits June 14, 2023 17:02

Compiler diagnostic output does not print empty double colon

49dfc8f

Enable pending test, add more tests

5e1392a

Akirathan linked an issue Jun 14, 2023 that may be closed by this pull request

Equality on types with Any as LHS does not work #5612

Closed

Akirathan added 2 commits June 16, 2023 13:46

Update tests to match the new semantics.

bfc487a

methods on Any have default `self` argument.

Remove outdated comment

8493dff

Akirathan force-pushed the wip/akirathan/5612-equality-on-types-with-any branch from 0ea6f75 to 8493dff Compare June 16, 2023 11:46

Akirathan added 3 commits June 16, 2023 17:13

Fix and cleanup tests

3ff28b6

Add tests for 3 arguments method on Any

9f949bb

Use Meta.is_a ... Function for testing if something is a function

09111bd

Akirathan self-assigned this Jun 21, 2023

Add special handling for static method calls on Any

5dfdb69

Akirathan force-pushed the wip/akirathan/5612-equality-on-types-with-any branch from 536e493 to 5dfdb69 Compare June 21, 2023 15:39

Add parenthesis to tests

530d740

Akirathan marked this pull request as ready for review June 21, 2023 17:17

Akirathan requested review from jdunkerley, radeusgd, GregoryTravis, 4e6, JaroslavTulach and hubertp as code owners June 21, 2023 17:17

GregoryTravis approved these changes Jun 21, 2023

View reviewed changes

hubertp reviewed Jun 21, 2023

View reviewed changes

hubertp requested a review from kustosz June 21, 2023 18:46

enso-bot bot mentioned this pull request Jun 22, 2023

Major performance difference between inline function and as a node within IDE #7070

Closed

Akirathan commented Jun 22, 2023

View reviewed changes

test/Tests/src/Semantic/Any_Spec.enso Outdated Show resolved Hide resolved

Akirathan added 3 commits June 22, 2023 15:44

fmt

8788c14

Merge branch 'develop' into wip/akirathan/5612-equality-on-types-with…

dfc9997

…-any

Add docs

44ea9b7

Akirathan commented Jun 22, 2023

View reviewed changes

Update CHANGELOG

f122d08

JaroslavTulach approved these changes Jun 22, 2023

View reviewed changes

Akirathan added 2 commits June 22, 2023 16:51

Use constant instead of hard-coded string literal

43fbab3

Use transferToInterpreterAndInvalidate

45d7fa5

radeusgd reviewed Jun 22, 2023

View reviewed changes

test/Tests/src/Semantic/Any_Spec.enso Outdated Show resolved Hide resolved

radeusgd reviewed Jun 22, 2023

View reviewed changes

test/Tests/src/Semantic/Any_Spec.enso Outdated Show resolved Hide resolved

radeusgd reviewed Jun 22, 2023

View reviewed changes

test/Tests/src/Semantic/Any_Spec.enso Show resolved Hide resolved

radeusgd reviewed Jun 22, 2023

View reviewed changes

test/Tests/src/Semantic/Any_Spec.enso Outdated Show resolved Hide resolved

radeusgd reviewed Jun 22, 2023

View reviewed changes

test/Tests/src/Semantic/Any_Spec.enso Outdated Show resolved Hide resolved

radeusgd approved these changes Jun 22, 2023

View reviewed changes

Akirathan and others added 3 commits June 22, 2023 18:08

Apply suggestions from code review

1b12abb

Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>

Add more type.to_text tests

b4b93bc

Replace rest of Meta.is_a with Meta.should_be_a

7d62c63

Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Jun 22, 2023

enso-bot bot mentioned this pull request Jun 23, 2023

@widgets for extension functions of Table may not be found #6955

Closed

JaroslavTulach reviewed Jun 23, 2023

View reviewed changes

Akirathan added 3 commits June 23, 2023 11:38

Add require test from PR review

d4f0b6d

Fix should_be_a method calls

bddb7ab

Fix static method invocations on Any in the presence of default argum…

42e0597

…ents

Akirathan linked an issue Jun 23, 2023 that may be closed by this pull request

Meta.meta Any . methods doesn't work #6473

Closed

Akirathan added 3 commits June 23, 2023 14:11

Merge branch 'develop' into wip/akirathan/5612-equality-on-types-with…

ec8ad44

…-any

Fix native-image build - no Java streams on fast path

7021fa6

Fix type error on test

d0cef3a

hubertp approved these changes Jun 23, 2023

View reviewed changes

mergify bot merged commit 31aad1d into develop Jun 23, 2023

mergify bot deleted the wip/akirathan/5612-equality-on-types-with-any branch June 23, 2023 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always call instance methods on Any #7033

Always call instance methods on Any #7033

Akirathan commented Jun 14, 2023 •

edited

Loading

hubertp left a comment

hubertp Jun 21, 2023

Akirathan Jun 22, 2023

Akirathan Jun 22, 2023

Akirathan Jun 22, 2023

JaroslavTulach left a comment

JaroslavTulach Jun 22, 2023

JaroslavTulach Jun 22, 2023

JaroslavTulach Jun 22, 2023

radeusgd left a comment

JaroslavTulach Jun 23, 2023

Akirathan Jun 23, 2023

JaroslavTulach Jun 23, 2023

radeusgd Jun 23, 2023

Akirathan Jun 23, 2023

radeusgd Jun 23, 2023

Always call instance methods on Any #7033

Always call instance methods on Any #7033

Conversation

Akirathan commented Jun 14, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

hubertp 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

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

radeusgd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan commented Jun 14, 2023 •

edited

Loading