By-type pattern matching #3742

hubertp · 2022-09-27T17:27:12Z

Pull Request Description

This change brings by-type pattern matching to Enso.
One can pattern match on Enso types as well as on polyglot types.

For example,

case x of
    _ : Integer -> ...
    _ : Text -> ...
   _ -> ...

as well as Java's types

case y of 
    _ : ArrayList -> ...
    _ : List -> ...
    _ : AbstractList -> ...
    _ -> ..

It is no longer possible to match a value with a corresponding type constructor.
For example

case Date.now of
    Date -> ...

will no longer match and one should match on the type (_ : Date) instead.

case Date of
    Date -> ...

is fine though, as requested in the ticket.

The change required further changes to type_of logic which wasn't dealing well with polyglot values.

Implements https://www.pivotaltracker.com/story/show/183188846

Important Notes

I discovered late in the game that nested patterns involving type patterns, such as Const (f : Foo) tail -> ... are not possible due to the old parser logic.
I would prefer to add it in a separate PR because this one is already getting quite large. This is now supported!

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

hubertp · 2022-09-30T10:24:29Z

...untime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/CatchTypeBranchNode.java

+    boolean test = isSameObject.execute(expectedType, typeOfTarget);
+    if (profile.profile(test)) {
+      accept(frame, state, new Object[] {target});
+    } else if (TypesGen.isType(typeOfTarget)) {


@JaroslavTulach I suppose I should profile conditions in this branch as well?

Can we analyze the positive/negative check? Check the IGV graphs or have a benchmark for the simple expectedType equals typeOfTarget and for the case when it is not? Especially when there are some tpe.getSupertype()?

If we can guarantee some stability of superTpe, then this while cycle would be candidate for @ExplodeLoop...

CHANGELOG.md

jdunkerley

couple of minor bits - only looked at the enso stuff.

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

test/Tests/src/Semantic/Case_Spec.enso

radeusgd

This is awesome! I'm glad we finally have that.

Looks good, just a few comments/questions in-line.

radeusgd · 2022-09-30T14:08:25Z

distribution/lib/Standard/Database/0.0.0-dev/src/Data/SQL.enso

-        Text -> if other == "" then self else Builder_Data (self.fragments ++ (code other).fragments)
+        text : Text -> if text == "" then self else Builder_Data (self.fragments ++ (code text).fragments)


Wow, I didn't know we'll be able to do that too! It's great!

...n/java/org/enso/interpreter/node/controlflow/caseexpr/CatchPolyglotSymbolTypeBranchNode.java

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

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

radeusgd · 2022-09-30T15:53:34Z

test/Tests/src/Data/Text/Regex_Spec.enso

@@ -25,4 +25,6 @@ spec =
            pattern = "http://example.com"
            Regex.escape pattern . should_equal "\Qhttp://example.com\E"

+    ## TODO: Missing tests for No_Such_Group_Error


Any more information on what happened here?

This is more of a wishlist since No_Such_Group_Error.to_display_text pattern matches on Text and Integer types but I only discovered those via a simple grep because all tests were passing.

test/Tests/src/Semantic/Case_Spec.enso

...n/java/org/enso/interpreter/node/controlflow/caseexpr/CatchPolyglotSymbolTypeBranchNode.java

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

hubertp · 2022-09-30T18:19:58Z

Added supported for deeply nested type patterns, such as Cons (num : Integer) Nil -> ...

JaroslavTulach

Without no doubts this change has to get in! I have some doubts when Node references a value, but that is a strategic problem, not something that has to be resolved in this PR. Thank you @hubertp for simplifying the logic of branching by reducing the number of various branch nodes!

JaroslavTulach · 2022-10-01T02:38:54Z

...n/java/org/enso/interpreter/node/controlflow/caseexpr/CatchPolyglotSymbolTypeBranchNode.java

+@NodeInfo(shortName = "PolyglotSymbolTypeMatch")
+public abstract class CatchPolyglotSymbolTypeBranchNode extends BranchNode {
+
+  private final Object polyglotSymbol;


Code (e.g. Node subclasses) cannot directly hold on values, if we ever want to use single Engine for multiple Contexts. CCing @kustosz

Enso symbols aren't referenced directly, but via Enso Context and that's OK. We need something same for non-Enso values.

...untime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/CatchTypeBranchNode.java

JaroslavTulach · 2022-10-01T02:43:21Z

...untime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/CatchTypeBranchNode.java

+  CatchTypeBranchNode(Type tpe, RootCallTarget functionNode) {
+    super(functionNode);
+    this.expectedType = tpe;
+    this.isArrayExepctedType = Context.get(this).getBuiltins().array() == tpe;


There is (or can be) 1:N mapping between Node and Context instances. Storing tpe in Node prevents us to use multiple Contexts with a single Engine.

JaroslavTulach · 2022-10-01T02:51:35Z

...untime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/CatchTypeBranchNode.java

+    boolean test = isSameObject.execute(expectedType, typeOfTarget);
+    if (profile.profile(test)) {
+      accept(frame, state, new Object[] {target});
+    } else if (TypesGen.isType(typeOfTarget)) {


Can we analyze the positive/negative check? Check the IGV graphs or have a benchmark for the simple expectedType equals typeOfTarget and for the case when it is not? Especially when there are some tpe.getSupertype()?

If we can guarantee some stability of superTpe, then this while cycle would be candidate for @ExplodeLoop...

JaroslavTulach · 2022-10-01T02:52:06Z

...runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/DateTimeBranchNode.java

@@ -1,63 +0,0 @@
-package org.enso.interpreter.node.controlflow.caseexpr;


Great to see all these special branch nodes being deleted!

test/Tests/src/Semantic/Case_Spec.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

JaroslavTulach · 2022-10-01T03:26:20Z

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

@@ -829,6 +829,17 @@ object AstView {
            case Pattern(pat) => Some((pat, right))
            case _            => None
          }
+        case AST.App.Infix(


CCing @kazcw.

JaroslavTulach · 2022-10-01T03:27:47Z

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

@@ -968,52 +968,9 @@ class IrToTruffle(
                    ) =>
                  val tpe =
                    mod.unsafeAsModule().getScope.getTypes.get(tp.name)
-                  val any         = context.getBuiltins.any


Removing all the specialized branch nodes and replacing them with a single construct is a magnificent improvement regardless of any other comments.

`scope.getType` vs `scope.getTypes.get` is easy to get wrong. Now ``` case Date_Time.now of _ : Date_Time.Date_Time -> "OK" _ -> ``` works as expected.

We can now pattern match on types!

Polyglot values coming from JS or Python were problematic because they followed a different path than Java's polyglot values. The type of JS' Array values was PolyglotProxy which we need to specialize on. Python's Array values are on the other hand represented as Python list. Since it is impossible to special case those when inferring the type, CatchType has a specialization on Array-like values that have a meta object but no type. Additionally added support for pattern matching on polyglot symbols (both in types and as values).

And replace it with pattern matching on Text type.

Also now supporting subtyping for Enso as well as polyglot types.

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

This messes up aliasing info, which in turn affects global names pass.

hubertp force-pushed the wip/hubert/type-pattern-183188846 branch from 6bb58fe to cafbb2e Compare September 30, 2022 08:32

hubertp marked this pull request as ready for review September 30, 2022 10:23

hubertp requested review from jdunkerley, radeusgd, 4e6 and JaroslavTulach as code owners September 30, 2022 10:23

hubertp commented Sep 30, 2022

View reviewed changes

radeusgd reviewed Sep 30, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

jdunkerley approved these changes Sep 30, 2022

View reviewed changes

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

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

radeusgd approved these changes Sep 30, 2022

View reviewed changes

JaroslavTulach approved these changes Oct 1, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/type-pattern-183188846 branch 2 times, most recently from 4348b1f to 52f274c Compare October 3, 2022 14:20

hubertp and others added 16 commits October 4, 2022 09:30

wip

a9ed280

Fixed execution for type pattern matching

21d6537

Don't use associated type for case comparisons

3162823

`scope.getType` vs `scope.getTypes.get` is easy to get wrong. Now ``` case Date_Time.now of _ : Date_Time.Date_Time -> "OK" _ -> ``` works as expected.

Vector, File and date/time BranchNodes are gone

b6d01ef

We can now pattern match on types!

BooleanConstructorBranchNode is gone

5e622da

Remove TextBranchNode

0de9a83

And replace it with pattern matching on Text type.

Number, Integer and Decimal branches are gone

4f3fc9c

Also now supporting subtyping for Enso as well as polyglot types.

Match on Any type rather than constructor

b81c6b0

post rebase tweaks

3b97fb0

Changelog

e00482e

More tests and typos

cb8af46

Update CHANGELOG.md

55b1be4

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

support deeply nested type patterns

1bab27d

PR comments

57c989f

Don't add type name to local scope usage

3013de1

This messes up aliasing info, which in turn affects global names pass.

hubertp added 2 commits October 4, 2022 09:30

Add test for the fixed alias bug

d76e6bd

add benchmark

cb7e952

hubertp force-pushed the wip/hubert/type-pattern-183188846 branch from 1d55a67 to cb7e952 Compare October 4, 2022 07:30

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

mergify bot merged commit ae66087 into develop Oct 4, 2022

mergify bot deleted the wip/hubert/type-pattern-183188846 branch October 4, 2022 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

By-type pattern matching #3742

By-type pattern matching #3742

hubertp commented Sep 27, 2022 •

edited

Loading

hubertp Sep 30, 2022

JaroslavTulach Oct 1, 2022

jdunkerley left a comment

radeusgd left a comment

radeusgd Sep 30, 2022

radeusgd Sep 30, 2022

hubertp Oct 3, 2022

hubertp commented Sep 30, 2022

JaroslavTulach left a comment

JaroslavTulach Oct 1, 2022

JaroslavTulach Oct 1, 2022

JaroslavTulach Oct 1, 2022

JaroslavTulach Oct 1, 2022

JaroslavTulach Oct 1, 2022

JaroslavTulach Oct 1, 2022

		Text -> if other == "" then self else Builder_Data (self.fragments ++ (code other).fragments)
		text : Text -> if text == "" then self else Builder_Data (self.fragments ++ (code text).fragments)

		@@ -1,63 +0,0 @@
		package org.enso.interpreter.node.controlflow.caseexpr;

By-type pattern matching #3742

By-type pattern matching #3742

Conversation

hubertp commented Sep 27, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

hubertp commented Sep 30, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented Sep 27, 2022 •

edited

Loading