Explicit self #3569

hubertp · 2022-07-07T12:58:26Z

Pull Request Description

This change modifies the current language by requiring explicit self parameter declaration
for methods. Methods without self parameter in the first position should be treated as statics
although that is not yet part of this PR. We add an implicit self to all methods
This obviously required updating the whole stdlib and its components, tests etc but the change
is pretty straightforward in the diff.

Notice that this change does not change method dispatch, which was removed in the last changes.
This was done on purpose to simplify the implementation for now. We will likely still remove all
those implicit selfs to bring true statics.
Minor caveat - since main doesn't actually need self, already removed that which simplified
a lot of code.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide dist and ./run ide watch.

hubertp · 2022-07-18T10:40:59Z

engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala

+            ) =>
+          val global = fn.getMetadata(GlobalNames)
+          global.map(_.target) match {
+            case Some(BindingsMap.ResolvedMethod(module, method)) =>


This change is a bit controversial in a sense that by making GlobalNames code simple and conform to the overall approach (no self arguments) we have to compensate for that in IrToTruffle translation. It seemed like a good idea at the time but it complicated the logic slightly here.
On the upside, we don't do symbol resolution for local functions in runtime.

I can bring back the old approach by reverting the change in GlobalNames and removing this special handling here. It would be a tiny change and I checked that it works. So happy to get feedback on this piece!

kustosz

Explained on priv, I have some doubts about why this change reaches so deep into function execution

CHANGELOG.md

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

hubertp · 2022-07-22T08:39:28Z

@kustosz removed the controversial method dispatch changes.
Couldn't bring myself to reverting all those Object self in builtins, so I modified processor to add those instead :p

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

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

distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Postgres.enso

jdunkerley

Have only reviewed the enso files - a few minor fixed but otherwise LGTM

test/Tests/src/Semantic/Names_Spec.enso

JaroslavTulach

I went thru all the non-.enso files and tried to make a sense of the changes. My comments are more questions. If the system holds together the change is probably good. Great work!

JaroslavTulach · 2022-07-22T13:18:50Z

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/builtins/MethodGenerator.java

@@ -58,24 +58,24 @@ public abstract List<String> generate(
  protected String methodSigDef(String owner, List<MethodParameter> params, boolean isAbstract) {
    int paramsLen = params.size();
    String paramsDef;
+    boolean includeSelfParam = !(isStatic || isConstructor);


These DSL changes are interesting, but: @hubertp, didn't you have to update Ir or parser? I am asking because I am supposed to implement default keyword. I thought my task will be similar to your one. It seems to me that with explicit self we are also introducing new keyword self...

No? This particular change affects generating builtin nodes.
For example (roughly) for

@Builtin.Method public static void foo(Object x) { // ... }

it will generate Builtin node with execute without Object self i.e.

@BuiltinMethod(...) public class FooNode { public void execute(Object x) { /// ... } }

rather than

@BuiltinMethod(...) public class FooNode { public void execute(Object self, Object x) { /// ... } }

This is then accepted by the next processor which knows that it needs to add self as ArgumentDefinition at 0.
While strictly speaking not necessary, because in this version everything still has self ( insert sad face ) but at least most of it is now hidden.

JaroslavTulach · 2022-07-22T13:25:44Z

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

@@ -210,7 +210,7 @@ case object IgnoredBindings extends IRPass {
  ): IR.DefinitionArgument = {
    arg match {
      case spec @ IR.DefinitionArgument.Specified(
-            IR.Name.Self(_, _, _),
+            IR.Name.Self(_, _, _, _),


Here is IR.Name.Self! In the same manner I will have to make sure there is IR.Name.Default once I try to implement default arguments, right?

This change is only because I added a parameter to Self class definition and compiler would reject this pattern

engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala

JaroslavTulach · 2022-07-22T13:41:54Z

engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala

+      * @param ir the annotated application
+      * @param paramPosition the reason why the annotation cannot be obeyed
+      */
+    case class WrongSelfParameterPos(


Any reason why not make it an error instead of warning?

The discussion in discord mentions that it should be a warning. Truth be told, it will almost always be an error....

JaroslavTulach · 2022-07-22T13:44:18Z

engine/runtime/src/main/scala/org/enso/compiler/codegen/AstToIr.scala

      case AstView.Binding(AST.App.Section.Right(opr, arg), body) =>
+        val translatedArgs = arg match {
+          case AstView.FunctionParamList(items) =>


Is this change related to self? Or is it just adding a missing processing of FunctionParamList triggered by some new failing tests?

Yes, this is related to self.
Previously methods involving operands wouldn't have self.
For example, without this change we will crash on something as simple as

== self that = ....

engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala

engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/FunctionBinding.scala

engine/runtime/src/test/scala/org/enso/compiler/test/pass/analyse/AliasAnalysisTest.scala

On a scale of 1 to 10, the hack is probably 5. All the alternatives I considered, involved tweaking IR in a horrible way. And JS is a bit of a special case TBH. The solution adds an implicit self parameter with a default value resolving to the definining module. That way users don't have to provide an extra parameter and all arguments are magically applied

Added missing `self` in interop specs that were pending.

Minor nit: turns out methods are registered for modules under their lowercase name, rather than the original one. This is a bit surprising and might need to be removed, if it turns out to be unnecessary.

In the current approach the order of calling local functions matters which came up accidentally in a method with default arguments in Delimited_Reader. This is because methods are added to the scope in the order they are processed. Suprised this hasn't come up before. Will need to address this before releasing. Added a (pending) test-case to demonstrate the problem.

Handle local functions that are being used before being defined. Might want to revise that in GlobalNames

Discovered by accident.

This was requested to resolve the exceptional status of `this` in JS. It also introduces an inconsistency to the language as everything else uses `self`. Added some logic to detect situations when someone uses `self` in JS and issue warnings in such situation. The implementation implicitly replaces `this` with `self` in the parameter list so that we don't have to change the whole compiler to support this exception.

This approach might still go away, since we will explore adding implicit self always.

In order to limit the complexity of the change and its effect on method dispatch, this change brings back (implicit) self arguments for all methods. Couldn't force myself to bring all self parameters to builtins so instead adapted processor to add them.

`main` method is now special because it is trully static i.e. no implicit `self` is being generated for it. But since REPL's `main` isn't called `main` its invocation was missing an argument. Follow up on #3569 Will work on adding a CI test for REPL to avoid problems with REPL in a follow up PR.

`main` method is now special because it is trully static i.e. no implicit `self` is being generated for it. But since REPL's `main` isn't called `main` its invocation was missing an argument. Follow up on #3569 # Important Notes Will work on adding a CI test for REPL to avoid problems with REPL in a follow up PR.

hubertp force-pushed the wip/hubert/explicit-self-182582560 branch 3 times, most recently from b8bf352 to 460a334 Compare July 18, 2022 10:34

hubertp changed the title ~~WIP: Explicit self~~ Explicit self Jul 18, 2022

hubertp marked this pull request as ready for review July 18, 2022 10:34

hubertp requested review from 4e6, jdunkerley, radeusgd and JaroslavTulach as code owners July 18, 2022 10:34

hubertp requested a review from kustosz July 18, 2022 10:34

hubertp commented Jul 18, 2022

View reviewed changes

hubertp marked this pull request as draft July 18, 2022 22:23

kustosz reviewed Jul 19, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Numbers.enso Outdated Show resolved Hide resolved

hubertp force-pushed the wip/hubert/explicit-self-182582560 branch 3 times, most recently from 24cf852 to 47e909b Compare July 22, 2022 08:35

hubertp marked this pull request as ready for review July 22, 2022 08:36

hubertp requested a review from kustosz July 22, 2022 08:36

hubertp force-pushed the wip/hubert/explicit-self-182582560 branch from 47e909b to 429b86c Compare July 22, 2022 09:52

jdunkerley reviewed Jul 22, 2022

View reviewed changes

jdunkerley requested changes Jul 22, 2022

View reviewed changes

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

JaroslavTulach approved these changes Jul 22, 2022

View reviewed changes

kustosz approved these changes Jul 25, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/explicit-self-182582560 branch from 4799b21 to 1cb2229 Compare July 25, 2022 12:35

hubertp requested a review from jdunkerley July 25, 2022 12:36

jdunkerley approved these changes Jul 25, 2022

View reviewed changes

4e6 approved these changes Jul 25, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/explicit-self-182582560 branch 2 times, most recently from ffba928 to 809c6e5 Compare July 26, 2022 13:41

hubertp added 21 commits July 27, 2022 15:38

misc

ef55402

Last remaining adaptations to tests. All green!

30c86c5

Added missing `self` in interop specs that were pending.

nits

20ef330

Add self to Standard.{Database,Table} and adapt tests

f709070

Adapt remaining components and tests

97d9400

Fix remaining tests

f4723df

Minor nit: turns out methods are registered for modules under their lowercase name, rather than the original one. This is a bit surprising and might need to be removed, if it turns out to be unnecessary.

Rebase fixes

3154a79

Workaround initial limitation

4e6770b

Handle local functions that are being used before being defined. Might want to revise that in GlobalNames

Last minute tweaks, docs etc

1446a92

Changelog

dfccfff

Some text extensions should be static

24374f1

Discovered by accident.

Simplify function invocation logic

a042057

This approach might still go away, since we will explore adding implicit self always.

post-rebase fixes

6a6ab1f

nits

852f134

rebase

b0e0349

PR review

51fc187

Post rebase fixes

387904b

hubertp force-pushed the wip/hubert/explicit-self-182582560 branch from 98d4f7b to 387904b Compare July 27, 2022 14:46

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jul 27, 2022

mergify bot merged commit f63e40d into develop Jul 27, 2022

mergify bot deleted the wip/hubert/explicit-self-182582560 branch July 27, 2022 17:45

hubertp mentioned this pull request Jul 28, 2022

Fix REPL runner #3624

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicit self #3569

Explicit self #3569

hubertp commented Jul 7, 2022 •

edited

Loading

hubertp Jul 18, 2022

kustosz left a comment

hubertp commented Jul 22, 2022 •

edited

Loading

jdunkerley left a comment

JaroslavTulach left a comment

JaroslavTulach Jul 22, 2022

hubertp Jul 22, 2022 •

edited

Loading

JaroslavTulach Jul 22, 2022

hubertp Jul 22, 2022

JaroslavTulach Jul 22, 2022

hubertp Jul 22, 2022

JaroslavTulach Jul 22, 2022

hubertp Jul 22, 2022

Explicit self #3569

Explicit self #3569

Conversation

hubertp commented Jul 7, 2022 • edited Loading

Pull Request Description

Checklist

Choose a reason for hiding this comment

kustosz left a comment

Choose a reason for hiding this comment

hubertp commented Jul 22, 2022 • edited Loading

jdunkerley left a 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

hubertp Jul 22, 2022 • edited Loading

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

hubertp commented Jul 7, 2022 •

edited

Loading

hubertp commented Jul 22, 2022 •

edited

Loading

hubertp Jul 22, 2022 •

edited

Loading