search

nim-lang / fusion

Fusion is for now an idea about how to grow Nim's ecosystem without the pain points of more traditional approaches.
MIT License
128 stars 16 forks source link

Pattern matching follow-up PR. #61

Closed haxscramper closed 3 years ago

haxscramper commented 3 years ago

Misc documentation and implementation changes for pattern matching.

juancarlospaco commented 3 years ago

Typo on line 53 "mathc" should be match. (Github wont let me mark it for some reason)

juancarlospaco commented 3 years ago

If you can add more examples that would be great IMHO. :)

haxscramper commented 3 years ago

@juancarlospaco you mean more examples in documentation specifically (e.g. just copying some of them from tests), or coming up with more additional usage scenarios and putting them to documentation?

juancarlospaco commented 3 years ago

Whatever you feel like doing, but just more actual code examples that you can copy-paste and they work, like the one I suggested.

The explanations, descriptions and pseudo-code are more than enough, but just 1 example thats actual code that you can copy-paste and play with.

konsumlamm commented 3 years ago

You're probably aware, but the footnotes are broken (i.e. they don't link), but this seems to be an issue in the RST generator.

konsumlamm commented 3 years ago

I think it should be clarified that ref objects can be matched just like normal object (at least it seems to work), the current documentation makes it sound like you need to use of to match ref objects.

haxscramper commented 3 years ago

@konsumlamm to organize list of different implementation details that we've discussed (on discord, other issues etc)

Additional pattern match guards in form of [@a, .._] if a == 1: as opposed to [@a == 1, .._] - using if as infix operator is not possible, so it would have to be something like [@a, .._] and a == 1: which might be supported, but I suppose it could be confusing (additional way of doing the same), and it doesn't play well with sequence operations, because [all @start < 10] requires single iteration and fails earlier as opposed to [all @start] and allOfIt(start, it < 10) or something like that.

If some of your points are missing - I probably forgot to add something, it wasn't intentional.

If you have any code snippets that you would like to work (like Some(Some()) that you wrote when testing pattern matching (or any other things that you would like to see working) - feel free to just drop them in as a comment in this discussion, I would put them in unit test.

EDIT: and also add following points to documentation

konsumlamm commented 3 years ago

Some(Some(<pattern>)) Some(<pattern>)

FTFY.

Make sure that (field: <pattern>) is supported and has the same behavior as (field: is <pattern>). You edited this out to your comment on #245, but I guess it is better to have more intuitive syntax work too.

This seems to work, however I don't see the point of supporting is <pattern> in the first place. Or is it because @a is <pattern> works, to complement == 42?

Implementing custom unpackers - they are supported but not properly documented

Do you mean defining a proc and then matching (unpacker: <pattern>)? That is nice, but I still think we can do better.

Additional pattern match guards in form of [@a, .._] if a == 1: as opposed to [@a == 1, .._] - using if as infix operator is not possible, so it would have to be something like [@a, .._] and a == 1: which might be supported, but I suppose it could be confusing (additional way of doing the same), and it doesn't play well with sequence operations, because [all @start < 10] requires single iteration and fails earlier as opposed to [all @start] and allOfIt(start, it < 10) or something like that.

Hmm, I see, perhaps we could at least replace @a(<it expression>) with a custom template that gets exported from the module (to reuse the @a.predicate() mechanism). My biggest problem here is introducing a magic it variable.

haxscramper commented 3 years ago

Do you mean defining a proc and then matching (unpacker: <pattern>)? That is nice, but I still think we can do better.

I mean 

    type
      UserType1 = object
        fld1: float
        fld2: string
        case isDefault: bool
          of true: fld3: float
          of false: fld4: string

      UserType2 = object
        userFld: UserType1
        fld4: float

    proc `[]`(obj: UserType1, idx: static[FieldIndex]): any =
      when idx == 0:
        obj.fld1

      elif idx == 1:
        obj.fld2

      elif idx == 2:
        if obj.isDefault:
          obj.fld3

        else:
          obj.fld4

      else:
        static:
          error("Indvalid index for `UserType1` field " &
            "- expected value in range[0..2], but got " & $idx
          )

    proc `[]`(obj: UserType2, idx: static[FieldIndex]): any =
      when idx == 0:
        obj.userFld

      elif idx == 1:
        obj.fld4

      else:
        static:
          error("Indvalid index for `UserType2` field " &
            "- expected value in range[0..1], but got " & $idx
          )

    block:
      (@fld1, @fld2, _) := UserType1(fld1: 0.1, fld2: "hello")

      doAssert fld1 == 0.1
      doAssert fld2 == "hello"

    block:
      (fld1: @fld1, fld2: @fld2) := UserType1(fld1: 0.1, fld2: "hello")

      doAssert fld1 == 0.1
      doAssert fld2 == "hello"

    block:
      ((@fld1, @fld2, _), _) := UserType2(userFld: UserType1(fld1: 0.1, fld2: "hello"))

      doAssert fld1 == 0.1
      doAssert fld2 == "hello"

And https://github.com/nim-lang/fusion/blob/ea18f8559c514b227b148300a2900b3e2a282b0d/tests/tmatching.nim#L1326 - overload [] for static[FieldIndex]. But this is missing from documentation - this should be fixed.

FTFY. Some(Some(<pattern>)) Some(<pattern>)

We talked about nested optional patterns too, and they should be supported.

I don't see the point of supporting is in the first place. Or is it because @a is <pattern> works, to complement == 42?

The following patterns are identical because both 42 and == 42 are 'final expressions' - (integer/string/float literals) - they won't be parsed further, and pasted as-is. Any expressions in the form of <prefix-operator> <something-else> is a final expression, unless <prefix-operator> is any of the following: is, of. is was added only because it didn't make sense to me to make it a hard error. of is used for derived object matching.

These matches are identical wrt. to generated code 

    (a: 42) := (a: 42) # Final expression without prefix operator, compared for equality
    (a: is 42) := (a: 42) # `is <pattern>` with pattern being `42` (integer literal, final expression).
    (a: == 42) := (a: 42) # Final expression with prefix operator, pasted as-is

These generate different code

    (a: (@a, @b)) := (a: (1, 2)) # `(@a, @b)` is not a final expression, it will be decomposed further
    (a: (1, 2)) := (a: (1, 2)) # `(1, 2)` not a final expression, each element will be matched separately
    (a: == (1, 2)) := (a: (1, 2)) # Final expression, `.a` will be matched using `== (1, 2)`
```nim let expr_38315001 = (a: (1, 2),) let ok`gensym9350 = var b: typeof(( let tmp`gensym9345 = expr_38315001.a[1] tmp`gensym9345)) var a: typeof(( let tmp`gensym9347 = expr_38315001.a[0] tmp`gensym9347)) ( a = expr_38315001.a[0] true or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m@a\e[24m. ") false) and ( b = expr_38315001.a[1] true or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m@b\e[24m. ") false) or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m(@a, @b)\e[24m. ") false or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m(a: (@a, @b))\e[24m. ") false discard ok`gensym9350 let expr_38345001 = (a: (1, 2),) let ok`gensym9361 = (expr_38345001.a[0] == 1 or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m1\e[24m. ") false) and (expr_38345001.a[1] == 2 or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m2\e[24m. ") false) or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m(1, 2)\e[24m. ") false or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m(a: (1, 2))\e[24m. ") false discard ok`gensym9361 let expr_38365001 = (a: (1, 2),) let ok`gensym9368 = expr_38365001.a == (1, 2) or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m==(1, 2)\e[24m. ") false or block: raise MatchError(msg: "Pattern match failed: element does not match \e[4m(a: ==(1, 2))\e[24m. ") false discard ok`gensym9368 ```

But I understand your point about is being confusing - to be honest I was just stuck with this idea of using is explicitly, but it doesn't make much sense in the end, so I will update documentation/examples to just use (fld: <pattern>) when possible.

My biggest problem here is introducing a magic it variable.

Sequence matching was modeled after std/sequtils and it is a common idiom in other templates/macros. Explicit is better than implicit and variable injection should be used with care, but I think this case is justified.

haxscramper commented 3 years ago

This PR is ready to be merged as I haven't found anything else that needs change

konsumlamm commented 3 years ago

The first example in the documentation is still broken afaict (see review at the top).

metagn commented 3 years ago

Given that https://github.com/nim-lang/Nim/pull/16923 was merged, will you add a `case` alias to match?

haxscramper commented 3 years ago

With new release candidate near - https://forum.nim-lang.org/t/7494, and update for `case` I think this can be merged now

haxscramper commented 3 years ago

I also added some patterns from https://www.python.org/dev/peps/pep-0636/, just to test how similar they are overall.

haxscramper commented 3 years ago

CI failure seems to be completely unrelated: Error: Archive 'windows_x32.zip' could not be found and/or downloaded. Maybe your OS/architecture does not have any prebuilt available?