Towards a full-blown builtins DSL (part 3) #3471

hubertp · 2022-05-23T13:12:32Z

Pull Request Description

Auto-generate all builtin methods for builtin File type from method signatures.
Similarly, for ManagedResource and Warning.
Additionally, support for specializations for overloaded and non-overloaded methods is added.
Coverage can be tracked by the number of hard-coded builtin classes that are now deleted.

Important notes

Notice how type File now lacks prim_file field and we were able to get rid off all of those
propagating method calls without writing a single builtin node class.
Similarly ManagedResource and Warning are now builtins and Prim_Warnings stub is now gone.

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.sh ide dist and ./run.sh ide watch.

kustosz

This is starting to look like such a cool system <3

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java

kustosz · 2022-05-23T17:29:20Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java

  public String getName() {
    return this.truffleFile.getName();
  }

+  @Builtin.Method


❤️ I love how these are now just auto-exporting

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/FunctionThrowingIOException.java

kustosz

This is awesome, my dreams coming true. A few questions inline. Also, I'd like to ask for all the dsl-related classes to contain docs with examples showing source code and generated code, so that it's clear what does what

kustosz · 2022-05-30T20:20:54Z

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso

-    == that = this.prim_file.isEqual that.prim_file
+    == that = this.is_equal that
+
+    is_equal : File -> Boolean


why is this method needed?

That can go, true.

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

JaroslavTulach · 2022-05-31T07:16:41Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java

  private final TruffleFile truffleFile;
+  private static final String home = System.getProperty("user.home");


Such a static constant may be problematic when NativeImage is created from these classes and the class initializers are executed at build time. home would be a constant in the code referring to the home directory on the build machine.

Better to avoid such static constants and always read the property.

Fair enough

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

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso

WIP Partially auto-generated, partially manually written builtins nodes for File. The next step is to auto-generate builtins craft using enhanced annotation processor.

Enhanced processor to handle methods - `list` - `delete` - `outputStream` - `inputStream` One thing that was visible from the hardcoded node classes was that we had to handle situations where `asGuestValue`/`toHostObject` conversions were inferred automatically. As more types are marked as builtin(e.g. `Output_Stream`) those conversions will be very easy to drop.

Specializations force a similar, yet slightly different, code-gen of builtin methods compared to non-specialized variants. Half-way down the road it was clear which parts could be generalized and which could not. Did some re-shuffling to simplify File API and avoid some unnecessary specializations (like `File.new` builtins). The end result: all File builtins are now generated from method signatures. Added a bunch of ManagedResource methods as well, because why not.

Processor with >1.5k loc was getting hard to manage. Split the logic into individual files.

Forgot that try/catch inference was only supported in non-specialized nodes. Translated that to the specialized generator as well. Now it was possible to generate all System builtin methods from public methods. Also, we now generate Enso-friendly parameter names; previously it would be impossible to pass arguments by name.

Some tweaks to address review.

Builtins DSL in Action. Had to modify processor so that we can - specialize with fallbacks - infer AcceptWarnings or Warnings aren't propagated Some duplication exists in Warning definitions to deal with Vector -> Array conversion. We might want to revisit that at some point.

hubertp · 2022-06-07T13:31:09Z

Added a ton of documentation. Should be much clearer what specific annotations are capable of. Also, compared to the initial submission, got rid of more handwritten classes using the new DSL.

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso

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

kustosz

Love it!

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

4e6

Awesome!

CHANGELOG.md

radeusgd · 2022-06-10T11:15:34Z

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso

+    ## PRIVATE
+
+       Utility function that lists immediate children of a directory.
+    list_immediate_children_array : Array File
+    list_immediate_children_array = @Builtin_Method "File.list_immediate_children_array"


Maybe instead we should call this with suffix (or prefix?) _builtin? Or _primitive? Then all 'internal' methods would have similar names which would be helpful understanding which are those.

Good idea, but I'd rather leave it to a separate PR

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso

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

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

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

…sl/Builtin.java Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>

Auto-generate all builtin methods for builtin `File` type from method signatures. Similarly, for `ManagedResource` and `Warning`. Additionally, support for specializations for overloaded and non-overloaded methods is added. Coverage can be tracked by the number of hard-coded builtin classes that are now deleted. ## Important notes Notice how `type File` now lacks `prim_file` field and we were able to get rid off all of those propagating method calls without writing a single builtin node class. Similarly `ManagedResource` and `Warning` are now builtins and `Prim_Warnings` stub is now gone.

hubertp requested review from 4e6, jdunkerley, radeusgd and JaroslavTulach as code owners May 23, 2022 13:12

hubertp marked this pull request as draft May 23, 2022 13:12

kustosz reviewed May 23, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/builtin-objects-file-181499077 branch from 68c45cc to 477c7ef Compare May 24, 2022 13:29

hubertp marked this pull request as ready for review May 27, 2022 13:40

hubertp changed the title ~~WIP: Towards a full-blown builtins DSL (part 3)~~ Towards a full-blown builtins DSL (part 3) May 27, 2022

hubertp requested a review from kustosz May 30, 2022 16:24

kustosz reviewed May 30, 2022

View reviewed changes

JaroslavTulach reviewed May 31, 2022

View reviewed changes

JaroslavTulach approved these changes May 31, 2022

View reviewed changes

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/BuiltinsProcessor.java Show resolved Hide resolved

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso Show resolved Hide resolved

JaroslavTulach approved these changes May 31, 2022

View reviewed changes

hubertp added 12 commits June 7, 2022 12:27

Towards a full-blown builtins DSL (part 3)

4817708

WIP Partially auto-generated, partially manually written builtins nodes for File. The next step is to auto-generate builtins craft using enhanced annotation processor.

More refactorings

25db53b

Processor with >1.5k loc was getting hard to manage. Split the logic into individual files.

format

99e1255

nit

e0c7df9

Remove unused

623c0d2

Address PR comments

181de31

Some tweaks to address review.

Documentation + formatting

c8216e8

More documentation

92f39ba

hubertp force-pushed the wip/hubert/builtin-objects-file-181499077 branch from 23a9ea3 to 92f39ba Compare June 7, 2022 13:29

hubertp requested a review from kustosz June 7, 2022 13:29

radeusgd reviewed Jun 8, 2022

View reviewed changes

hubertp requested a review from radeusgd June 8, 2022 15:52

Address PR review

fcc650f

hubertp force-pushed the wip/hubert/builtin-objects-file-181499077 branch from fd6a32e to fcc650f Compare June 8, 2022 15:53

kustosz approved these changes Jun 9, 2022

View reviewed changes

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/Builtin.java Show resolved Hide resolved

Merge branch 'develop' into wip/hubert/builtin-objects-file-181499077

279fedb

hubertp mentioned this pull request Jun 10, 2022

Add Text.write Function #3518

Merged

4 tasks

4e6 approved these changes Jun 10, 2022

View reviewed changes

radeusgd reviewed Jun 10, 2022

View reviewed changes

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/Builtin.java Outdated Show resolved Hide resolved

radeusgd reviewed Jun 10, 2022

View reviewed changes

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/BuiltinsProcessor.java Outdated Show resolved Hide resolved

radeusgd approved these changes Jun 10, 2022

View reviewed changes

hubertp and others added 4 commits June 10, 2022 15:52

Apply suggestions from code review

723f458

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

Update lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/d…

abf1233

…sl/Builtin.java Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>

PR review

dcb52cf

Merge branch 'develop' into wip/hubert/builtin-objects-file-181499077

df08293

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

Merge branch 'develop' into wip/hubert/builtin-objects-file-181499077

c0fbcc9

hubertp removed the CI: Ready to merge This PR is eligible for automatic merge label Jun 13, 2022

Merge branch 'develop' into wip/hubert/builtin-objects-file-181499077

c0af918

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

mergify bot merged commit fd46e84 into develop Jun 13, 2022

mergify bot deleted the wip/hubert/builtin-objects-file-181499077 branch June 13, 2022 11:48

wdanilo mentioned this pull request Feb 6, 2023

Demonstrate Benefits of Incremental Updates for Small Code Change #4385

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Towards a full-blown builtins DSL (part 3) #3471

Towards a full-blown builtins DSL (part 3) #3471

hubertp commented May 23, 2022 •

edited

Loading

kustosz left a comment

kustosz May 23, 2022

kustosz left a comment

kustosz May 30, 2022

hubertp Jun 1, 2022

JaroslavTulach May 31, 2022

hubertp Jun 1, 2022

hubertp commented Jun 7, 2022 •

edited

Loading

kustosz left a comment

4e6 left a comment

radeusgd Jun 10, 2022

hubertp Jun 13, 2022

		private final TruffleFile truffleFile;
		private static final String home = System.getProperty("user.home");

Towards a full-blown builtins DSL (part 3) #3471

Towards a full-blown builtins DSL (part 3) #3471

Conversation

hubertp commented May 23, 2022 • edited Loading

Pull Request Description

Important notes

Checklist

kustosz left a comment

Choose a reason for hiding this comment

kustosz May 23, 2022

Choose a reason for hiding this comment

kustosz left a comment

Choose a reason for hiding this comment

kustosz May 30, 2022

Choose a reason for hiding this comment

hubertp Jun 1, 2022

Choose a reason for hiding this comment

JaroslavTulach May 31, 2022

Choose a reason for hiding this comment

hubertp Jun 1, 2022

Choose a reason for hiding this comment

hubertp commented Jun 7, 2022 • edited Loading

kustosz left a comment

Choose a reason for hiding this comment

4e6 left a comment

Choose a reason for hiding this comment

radeusgd Jun 10, 2022

Choose a reason for hiding this comment

hubertp Jun 13, 2022

Choose a reason for hiding this comment

hubertp commented May 23, 2022 •

edited

Loading

hubertp commented Jun 7, 2022 •

edited

Loading