Check type of `self` in static dispatch #8867

radeusgd · 2024-01-25T17:43:44Z

Pull Request Description

Fixes Only allow the correct type to be used for self in the static method call syntax #8805
also btw. fixes Internal IR representation 'leaked' in stack trace of a dataflow error thrown from a suspended argument with default value #8706

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,
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.
- Enable *BinaryDispatchTest.staticWithRFirst* tests
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala

radeusgd · 2024-01-25T20:59:44Z

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/DataflowErrorsTest.scala

+          |""".stripMargin
+      eval(code)
+      consumeOut should contain(
+        "        at <enso> <default::Test.fn::y>(Test:7:10-35)"


The reported location checked by this test used to be:

- at <enso> <default::Test.fn::Name.Literal(name = y, isMethod = false, location = Some(IdentifiedLocation[location=Location[start=89, end=90], uuid=None]), passData = MetadataStorage[], diagnostics = DiagnosticStorage(diagnostics = List()), id = null)>(Internal) + at <enso> <default::Test.fn::y>(Test:7:10-35)

the name was wrongly too long displaying IR internals

the location was Internal as it was not set correctly

radeusgd · 2024-01-25T21:04:56Z

engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java

+  @Test
+  public void selfTypeConversion() throws Exception {
+    final URI uri = new URI("memory://self_type_conversion.enso");
+    final Source src =
+        Source.newBuilder(
+                "enso",
+                """
+    from Standard.Base import all
+    type My_Type
+        Value x
+        f self y = self.x+y
+
+    type Convertible_Type
+        A x
+
+    type Inconvertible_Type
+        B x
+
+    My_Type.from (that : Convertible_Type) = My_Type.Value that.x+1
+
+    static_convertible = My_Type.f (Convertible_Type.A 23) 1000
+    static_inconvertible = My_Type.f (Inconvertible_Type.B 23) 1000
+    """,
+                uri.getAuthority())
+            .uri(uri)
+            .buildLiteral();
+
+    var module = ctx.eval(src);
+    var convertible = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "static_convertible");
+    assertEquals(1024, convertible.asInt());
+
+    try {
+      var invalid_static_call =
+          module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "static_inconvertible");
+      fail("Expecting an exception, not: " + invalid_static_call);
+    } catch (PolyglotException e) {
+      assertTypeError("`self`", "My_Type", "Inconvertible_Type", e.getMessage());
+    }
+  }


This test also ensures that not only values of requested type but also values convertible to it are supported, if they go through the conversion.

radeusgd · 2024-01-25T21:05:22Z

engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java

+
+    var module = ctx.eval(src);
+    var convertible = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "static_convertible");
+    assertEquals(1024, convertible.asInt());


We see that the value is 1024 not 1023, meaning that the conversion has been applied.

Probably better to turn this note into assertEquals("messgage", ...) message.

Ok, will do

JaroslavTulach

Tests seems to correctly request the desired behavior. The implementation is at surprising (to me) place, but it works and that's what counts.

JaroslavTulach · 2024-01-26T10:55:00Z

engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java

+          module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "invalid_static_call");
+      fail("Expecting an exception, not: " + invalid_static_call);
+    } catch (PolyglotException e) {
+      assertTypeError("`self`", "My_Type", "Other_Type", e.getMessage());


OK, the test looks fine.

JaroslavTulach · 2024-01-26T10:56:00Z

engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java

+
+    var module = ctx.eval(src);
+    var convertible = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "static_convertible");
+    assertEquals(1024, convertible.asInt());


Probably better to turn this note into assertEquals("messgage", ...) message.

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/resolve/MethodDefinitions.scala

JaroslavTulach · 2024-01-26T11:03:31Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/resolve/MethodDefinitions.scala

+              _,
+              _
+            )) :: rest =>
+          selfArg.copy(ascribedType = Some(typ)) :: rest


I was expecting fix in the IrToTruffle rather than in the IR passes. Probably because I am more comfortable with the Truffle nodes than the IR...

JaroslavTulach · 2024-01-26T11:10:23Z

@radeusgd, you will need to stop delegating from Array to Vector methods statically: https://github.com/enso-org/enso/actions/runs/7660354279/job/20877478960?pr=8867#step:10:2883 and move the shared static implementations to ArrayLikeHelpers.enso.

However that's all good - it demonstrates your check really detects violations!

GitHub
Check type of `self` in static dispatch · enso-org/enso@6760aed
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

radeusgd · 2024-01-26T14:42:59Z

@radeusgd, you will need to stop delegating from Array to Vector methods statically: enso-org/enso/actions/runs/7660354279/job/20877478960?pr=8867#step:10:2883 and move the shared static implementations to ArrayLikeHelpers.enso.

However that's all good - it demonstrates your check really detects violations!

GitHub**Check type of self in static dispatch · enso-org/enso@6760aed**Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

oh, right, I completely forgot about it! Of course, so I will need to do a slight refactor to make this work. I will see to it when I have a moment.

JaroslavTulach · 2024-01-30T02:57:06Z

Once you merge with develop, please enable:

[info] Test org.enso.interpreter.test.BinaryDispatchTest.staticWithRFirstArgumentIsConverted ignored
[info] Test org.enso.interpreter.test.BinaryDispatchTest.staticWithRFirstAndZSecondNoConversionHappens ignored

radeusgd · 2024-02-05T17:33:05Z

Once you merge with develop, please enable:

[info] Test org.enso.interpreter.test.BinaryDispatchTest.staticWithRFirstArgumentIsConverted ignored
[info] Test org.enso.interpreter.test.BinaryDispatchTest.staticWithRFirstAndZSecondNoConversionHappens ignored

I enabled them and they seem to be passing 🙂

radeusgd · 2024-02-07T17:35:43Z

Benchmarks trial 1
https://github.com/enso-org/enso/actions/runs/7818863156
https://github.com/enso-org/enso/actions/runs/7818865699

GitHub
Benchmark Standard Libraries · enso-org/enso@e8f5afb
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

GitHub
Benchmark Engine · enso-org/enso@e8f5afb
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

radeusgd · 2024-02-09T08:50:14Z

Stdlib Run after fixing remaining warning tests: https://github.com/enso-org/enso/actions/runs/7841630104

And one after changing to the simpler approach suggested by @jdunkerley (instead of extracting common vector/array logic, we just use the Vector methods by converting array by Vector.from_polyglot_array (O(1))), and some cleanups that should not influence benchmarks (just small compiler code refactor): https://github.com/enso-org/enso/actions/runs/7842961320

I was unable to run engine benchmarks as these have been failing for a while, I imagine it would be best to try checking these as well before merging this PR...

# Conflicts: # distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

radeusgd · 2024-02-10T21:58:41Z

@JaroslavTulach I re-requested your review as I have added some additional changes and thought you may want to see them.

My initial solution was a bit of a hack apparently - I assumed the type I define methods on is always in scope so I can just refer to that type by its regular name. The test https://github.com/enso-org/enso/blob/wip/radeusgd/8805-self-type-check/test/Base_Tests/src/Semantic/Names_Spec.enso#L8-L12 showed me that we may define methods on a name that is not in the current scope - by extension methods on a type accessed by a (not-necessarily-fully) qualified name.

I then tried just using fully qualified names for that - but that failed due to a bug #8997.

Towards the `Self` Type

As I did not see a very immediate solution, instead I relied on the Self type. The concept was partially to introduced to Enso - we have IR for it although I think it is not fully supported in the parser yet, e.g. even with these changes, the following program:

type My_Type
    Value x y

    swap : Self
    swap self -> Self = Self.Value self.y self.x

main =
    (My_Type.Value 1 2).swap

still fails with

X:\NBO\repr\test4.enso:4:12: error: The name `Self` could not be found.
    4 |     swap : Self
      |            ^~~~
X:\NBO\repr\test4.enso:5:18: error: The name `Self` could not be found.
    5 |     swap self -> Self = Self.Value self.y self.x
      |                  ^~~~
Aborting due to 2 errors and 0 warnings.
Execution finished with an error: Compilation aborted due to errors.

I did not aspire to implement the full scope of the Self type in this PR, as that far exceeded the scope of this PR and is not of immediate use anyway.

Instead, I added as much of self type resolution as was needed to get the type checks working.
Most of it happens in the TypeNames pass: https://github.com/enso-org/enso/blob/wip/radeusgd/8805-self-type-check/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/resolve/TypeNames.scala#L83-L120

Now, I am able to annotate the self argument in methods with the Name.Self IR which is resolved to the 'current' type and thus checked exactly in the way that we want.

Refactoring Array

My first approach was as we discussed - I've split the logic of vector operations into Array_Like_Helpers (that do not have any type ascriptions so they don't check the types) and made both Array and Vector delegate to these.

Later, James suggested that a simpler solution may be possible and so I've tried it b40b5ef - I'm converting the array (O(1) op) into a Vector and then passing it to the vector operations in the same way as before - but thanks to the conversion the operation now satisfies the type check.

I guess I could even try to avoid the static call convention altogether and just call the method on the converted vector... I will try that.

I'm not sure if I remember the rationale for that (@jdunkerley please correct me if I quote you wrongly), but I think it was to simplify the traces for Vector operations which are most common and could be a pain with debugging.

Benchmarking

I wanted to see if there's any performance impact.

I ran benchmarks both with the Array_Like_Helpers version (run 7841630104) and the to-vector-conversion (run 7842961320).

I could not yet run engine benchmarks as they are currently broken. Hopefully to be fixed by #8975. I guess I'd try to run the benchmarks again once that PR is merged.

Still, without #7927 it is relatively hard to be able to compare the benchmarks - apart from clear outliers, often it is hard to say if the difference is significant or just a random deviation within the usual variance.

Looking as much as I could at the results, it seems that the results fit in the usual benchmark variance, with either of the array-like-helper or vector-conversion approaches sometime being faster and sometime slower - no clear winners here.

… gets rid of the not-really-necessary self type check in that case

radeusgd · 2024-02-10T22:19:14Z

I guess I could even try to avoid the static call convention altogether and just call the method on the converted vector... I will try that.

Done in 28a4e34.
Another benchmark scheduled at https://github.com/enso-org/enso/actions/runs/7857761166

GitHub
Benchmark Standard Libraries · enso-org/enso@28a4e34
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

JaroslavTulach

I don't like the from_polyglot_array wrapping. If engine benchmarks are OK, then maybe... but the right fix was supposed to be to move the implementations into Array_Like_Helpers.

rationale ... was to simplify the traces for Vector operations

I see. I still continue to like Array_Like_Helpers solution more.

JaroslavTulach · 2024-02-12T10:20:39Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

@@ -159,7 +159,7 @@ type Array
         If a `Range`, the selection is specified by two indices, from and to.
    @range Index_Sub_Range.default_widget
    take : (Index_Sub_Range | Range | Integer) -> Vector Any
-    take self range=(Index_Sub_Range.First 1) = Vector.take self range
+    take self range=(Index_Sub_Range.First 1) = (Vector.from_polyglot_array self).take range


What's the effect of Vector.from_polyglot_array wrapping on engine benchmarks? I am afraid some of them might slow down.

Wrapping all Array objects by from_polyglot_array isn't the expected fix. I was expecting all the static methods to move into Array_Like_Helpers and both the Array and Vector instance methods to delegate to the Array_Like_Helpers implementations.

Array_Like_Helpers solution would directly operate on the vector/array. Wrapping by from_polyglot_array creates a proxy object in memory. All the code accessing it needs to unwrap the original object and then perform the original operation. E.g. we allocate more memory on heap, we need bigger Truffle ASTs (not that we'd measure any such aspect) than Array_Like_Helpers solution would require.

I did implement the Array_Like_Helpers solution in the first place, but @jdunkerley suggested the wrapping one might be better.

If we decide to go back to Array_Like_Helpers, it's just the matter of reverting the commit a8171168789d7252478cc334d82b2e29e51ee024

I guess either way we should wait for the fix of engine benchmarks and see how both solutions compare there.

I'm happy to go for either of these.

JaroslavTulach · 2024-02-12T10:38:56Z

I ran benchmarks both with the Array_Like_Helpers version (run 7841630104) and the to-vector-conversion (run 7842961320).

Those are StdLib benchmarks, not engine benchmarks! We need to compare the engine benchmarks (which are currently broken, being addressed by @Akirathan as part of his #8975). The engine benchmarks contain microbenchmarks comparing various array-like implementations. They will tell us if the from_polyglot_proxy implementation is fast or not.

Akirathan · 2024-02-12T10:49:37Z

Fix of the engine benchmarks is tracked in #9026 with the highest priority.

radeusgd · 2024-02-12T10:59:24Z

I ran benchmarks both with the Array_Like_Helpers version (run 7841630104) and the to-vector-conversion (run 7842961320).

Those are StdLib benchmarks, not engine benchmarks! We need to compare the engine benchmarks (which are currently broken, being addressed by @Akirathan as part of his #8975). The engine benchmarks contain microbenchmarks comparing various array-like implementations. They will tell us if the from_polyglot_proxy implementation is fast or not.

As noted below, I ran what I was able to run, but yes I also want to see the engine benchmarks comparison once they are fixed.

radeusgd · 2024-02-13T11:54:54Z

(Engine) benchmark run on current branch state: https://github.com/enso-org/enso/actions/runs/7886066367

Run after reverting the Array_Like_Helpers approach: https://github.com/enso-org/enso/actions/runs/7887354689

This reverts commit a817116

JaroslavTulach

I've just checked the benchmarks and I see no slowdown. I still maintain slight preference in favor of Array_Like_Helpers approach, but approving even the proxy way.

# Conflicts: # CHANGELOG.md

radeusgd self-assigned this Jan 25, 2024

radeusgd requested review from 4e6, JaroslavTulach, hubertp and Akirathan as code owners January 25, 2024 17:43

radeusgd commented Jan 25, 2024

View reviewed changes

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala Show resolved Hide resolved

radeusgd commented Jan 25, 2024

View reviewed changes

JaroslavTulach approved these changes Jan 26, 2024

View reviewed changes

enso-bot bot mentioned this pull request Jan 31, 2024

S3 File Writing #8809

Closed

6 tasks

radeusgd requested review from jdunkerley, GregoryTravis and AdRiley as code owners February 3, 2024 12:50

radeusgd added 10 commits February 9, 2024 09:58

add tests for self type check

920e1e6

probable fix for #8706

738dba7

add self type ascription to the static method variant

deda165

remove TODO

f974567

fmt

15c1ada

changelog

0f195bf

add test case for suspended default arg + fix its location

9da0231

scalafmt

14ac342

CR

9bd0070

enable pending tests in binary dispatch test

592c41d

radeusgd added 2 commits February 9, 2024 11:41

fix seemingly unrelated test?

aa46001

more fixes

28735bf

4e6 approved these changes Feb 9, 2024

View reviewed changes

radeusgd requested a review from JaroslavTulach February 10, 2024 21:34

Merge branch 'develop' into wip/radeusgd/8805-self-type-check

d61e4c9

# Conflicts: # distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

radeusgd mentioned this pull request Feb 10, 2024

Merge empty_error #9000

Merged

5 tasks

call member on converted vector instead of using static syntax - this…

28a4e34

… gets rid of the not-really-necessary self type check in that case

Merge branch 'develop' into wip/radeusgd/8805-self-type-check

a33a2bf

JaroslavTulach requested changes Feb 12, 2024

View reviewed changes

Merge branch 'develop' into wip/radeusgd/8805-self-type-check

f70776f

radeusgd added 4 commits February 13, 2024 13:05

Merge branch 'develop' into wip/radeusgd/8805-self-type-check

468a539

fix Empty_Error

651931d

Revert "Revert array helpers refactor"

f29f313

This reverts commit a817116

fixes after revert + updates

32ecd32

JaroslavTulach self-requested a review February 13, 2024 14:17

JaroslavTulach approved these changes Feb 13, 2024

View reviewed changes

radeusgd added 2 commits February 14, 2024 12:45

Merge branch 'develop' into wip/radeusgd/8805-self-type-check

038af50

# Conflicts: # CHANGELOG.md

javafmt

f17e643

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Feb 14, 2024

GregoryTravis approved these changes Feb 14, 2024

View reviewed changes

mergify bot merged commit d45f0fe into develop Feb 14, 2024
30 of 31 checks passed

mergify bot deleted the wip/radeusgd/8805-self-type-check branch February 14, 2024 15:50

radeusgd mentioned this pull request Feb 22, 2024

Extension methods for Java classes #9129

Closed

3 tasks

enso-bot bot mentioned this pull request Mar 1, 2024

Prototype for type inference IR pass #8590

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check type of `self` in static dispatch #8867

Check type of `self` in static dispatch #8867

radeusgd commented Jan 25, 2024 •

edited by JaroslavTulach

Loading

radeusgd Jan 25, 2024

radeusgd Jan 25, 2024

radeusgd Jan 25, 2024

JaroslavTulach Jan 26, 2024

radeusgd Jan 26, 2024

JaroslavTulach left a comment

JaroslavTulach Jan 26, 2024

JaroslavTulach Jan 26, 2024

JaroslavTulach Jan 26, 2024

JaroslavTulach commented Jan 26, 2024 •

edited by unfurl-links bot

Loading

radeusgd commented Jan 26, 2024

JaroslavTulach commented Jan 30, 2024

radeusgd commented Feb 5, 2024

radeusgd commented Feb 7, 2024 •

edited by unfurl-links bot

Loading

radeusgd commented Feb 9, 2024 •

edited

Loading

radeusgd commented Feb 10, 2024

radeusgd commented Feb 10, 2024 •

edited by unfurl-links bot

Loading

JaroslavTulach left a comment •

edited

Loading

JaroslavTulach Feb 12, 2024 •

edited

Loading

radeusgd Feb 12, 2024

JaroslavTulach commented Feb 12, 2024 •

edited

Loading

Akirathan commented Feb 12, 2024

radeusgd commented Feb 12, 2024

radeusgd commented Feb 13, 2024 •

edited

Loading

JaroslavTulach left a comment

Check type of self in static dispatch #8867

Check type of self in static dispatch #8867

Conversation

radeusgd commented Jan 25, 2024 • edited by JaroslavTulach Loading

Pull Request Description

Important Notes

Checklist

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

JaroslavTulach commented Jan 26, 2024 • edited by unfurl-links bot Loading

radeusgd commented Jan 26, 2024

JaroslavTulach commented Jan 30, 2024

radeusgd commented Feb 5, 2024

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

radeusgd commented Feb 9, 2024 • edited Loading

radeusgd commented Feb 10, 2024

Towards the Self Type

Refactoring Array

Benchmarking

radeusgd commented Feb 10, 2024 • edited by unfurl-links bot Loading

JaroslavTulach left a comment • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Feb 12, 2024 • edited Loading

Akirathan commented Feb 12, 2024

radeusgd commented Feb 12, 2024

radeusgd commented Feb 13, 2024 • edited Loading

JaroslavTulach left a comment

Choose a reason for hiding this comment

Check type of `self` in static dispatch #8867

Check type of `self` in static dispatch #8867

radeusgd commented Jan 25, 2024 •

edited by JaroslavTulach

Loading

JaroslavTulach commented Jan 26, 2024 •

edited by unfurl-links bot

Loading

radeusgd commented Feb 7, 2024 •

edited by unfurl-links bot

Loading

radeusgd commented Feb 9, 2024 •

edited

Loading

Towards the `Self` Type

radeusgd commented Feb 10, 2024 •

edited by unfurl-links bot

Loading

JaroslavTulach left a comment •

edited

Loading

JaroslavTulach Feb 12, 2024 •

edited

Loading

JaroslavTulach commented Feb 12, 2024 •

edited

Loading

radeusgd commented Feb 13, 2024 •

edited

Loading