Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to indent of/else/elif/except/finally for block commands #420

Closed
haxscramper opened this issue Sep 4, 2021 · 26 comments
Closed

Allow to indent of/else/elif/except/finally for block commands #420

haxscramper opened this issue Sep 4, 2021 · 26 comments

Comments

@haxscramper
Copy link

haxscramper commented Sep 4, 2021

Proposal is pretty simple - allow additional level of indentation for command calls:

  match arg:
    of test1: discard
    of test2: discard
    elif 12: discard
    except: discard
    else: discard

This topic came up several times in discussion of caseStmtMacros. Main reason for this change - there should be only one case in the language, and implicitly overloading it with multiple different semantics is not the best idea

  • can't google it ("nim case <macro-related error>" would probably confuse every possible syntax at once),
  • have to keep in mind all the imports,
  • not clear what is going on in the code based on syntax alone (which overload would implicitly trigger in this case?),
  • the name is not descriptive, behavior is implicit, it relies on experimental feature that has to be explicitly turned for per-file or global (more language dialects) basis,
  • it clashes with generics since it depends on the expression type (I believe this is the reason for Pattern matching implementation fusion#33 (comment)).

Instead, being able to write a custom macro that looks like case is much easier, but right now case allows for indented of, but command block doesn't. This is probably the only limitation, it is pretty minor, but still annoying because of random inconsistencies.

Related

@Araq
Copy link
Member

Araq commented Sep 4, 2021

The way to write the case statement is:

case variable
of value1, value2: ...

No additional indentation and no colon after variable. The alternative was allowed because some users demanded it but it never made sense as no "code block" follows after the (optional) colon.

Likewise we only need to support

  match arg
  of test1: discard
  of test2: discard
  elif 12: discard
  except: discard
  else: discard

IMO.

But I still like the overloaded case better.

@haxscramper
Copy link
Author

haxscramper commented Sep 4, 2021

match arg:
  myof ...

match arg:
of ...

case arg:
of ..

case arg:
  of ...

Are allowed, but

match arg:
  of ...

Is not allowed. If we disregard stylistic preferences about number of spaces before 'of', optionally of the colon, we still have this inconsistency where parsing of the 'of' branches differs between case statement and user commands. Both allow to use branches, but one supports indent, and other does not.

Also,

match arg:
  myof ...

Is the only way to write it, because

match arg:
myof ...

Is not valid.

@haxscramper
Copy link
Author

But I still like the overloaded case better.

What about the reasoning for the customMacro that I provided? This RFC is directly tied to it, so I think it can be addressed as well.

@disruptek
Copy link

How about, instead, deprecating case expr: syntax; it promotes a mental model that doesn't match reality and I cannot see how this is advantageous to someone learning the language.

@haxscramper
Copy link
Author

Okay, got it, stylistic preferences etc. Don't think there is any reason to keep this RFC open then, probably won't be accepted anyway.

@Araq Araq reopened this Sep 5, 2021
@haxscramper
Copy link
Author

haxscramper commented Sep 5, 2021

Why reopen? It seems like most responses would be "no, because I don't like to indent 'of'" (i.e. this will turn into stylistic holywar without addressing any other points).

@Araq
Copy link
Member

Araq commented Sep 5, 2021

Well it's not a bad idea in itself. I merely happen to dislike it but not strongly enough. Plus my own proposal (match x\nof...) does not work with today's parser either.

@haxscramper
Copy link
Author

haxscramper commented Sep 5, 2021

Got it. I reopened PR then, its CI is green and fix itself is ~7 lines, so it is mostly a yes/no decision in the RFC about whether we want this in the language.

@juancarlospaco
Copy link
Contributor

I am neutral, but I want Pattern Matching more close to StdLib,
even if it has to be a more simplified version of it.

@haxscramper
Copy link
Author

haxscramper commented Sep 7, 2021

Pattern matching in stdlib also should wait for (1) let expressions, (2) stable view types, and some reimplementation effort for fusion/matching. This issue is not directly blocking std/matching ... it really just an absolutely tiny inconsistency that I think could be fixed, that's all. Most languages with pattern matching allow to indent arms, and case can be indented, so IMO it would be weird to explicitly disallow this, or make it work somehow differently.

@Varriount
Copy link

I'd prefer pattern matching to be available as a non-standard library module first, and then later incorporated into the standard library. That way we have some definite evidence regarding which design or approach works best.

@haxscramper
Copy link
Author

haxscramper commented Sep 9, 2021

@Varriount and that's exactly why I decided to add pattern matching to fusion first, so everyone could test it out and see what is working and what is not. Over the past 11 months since I made my PR nim-lang/fusion#33 I didn't have any major complaints with design, things that people did complain about were mostly about missing functionality, or lack of docs for certain parts, which I also tried to fix in timely manner. Overall, I think my specification from #245 worked well enough, with the only noticeable inconvenience being that there can be only one implementation of the patter matching in a given scope, and it would implicitly change the semantics of the built-in language construct based on the switch expression type. Which is, IMO, quite questionable.

Also, if you have any specific complaints about pattern matching implementation you can open the issue in fusion, or comment on #245, or tag me on any of the real-time chats (that's where a noticeable portion of fusion improvements were designed).

@haxscramper
Copy link
Author

haxscramper commented Sep 9, 2021

I also did make a PR nim-lang/Nim#17047 into stdlib when fusion was unbundled from nim, but was told (in discord discussion that followed) that "well if you want it in the standard, it should look exactly as we want it", (in context of let expressions https://irclogs.nim-lang.org/15-02-2021.html#18:49:45). I agree that we need to wait for let expressions, and some other things, and after all I decided to close the PR because "current implementation can be improved in certain areas, and regardless whether we would get it into stdlib at some point, it would differ from this PR" nim-lang/Nim#17047 (comment)

And while view types and let expressions are relatively huge, 'match' syntax requires 7 line fix in the compiler, and would make it possible to write macros that provide completely seamless syntactic interoperability with 'case'. Without any new features, implicit type-based change-semantics-of-basic-constructs, must-use-experimental-feature solutions.

@Araq
Copy link
Member

Araq commented Sep 10, 2021

What would a parser fix that allows for match x\nof take?

@haxscramper
Copy link
Author

haxscramper commented Sep 10, 2021

Just to make sure, you mean this, right?

match arg
of test

Well in my PR I just expand postExprBlocks with trivial check for indentation. In case of match arg ... it should be able to handle all the edge cases, so command statement parsing should look ahead, check if there is an of, and parse it, otherwise finish parsing. At least that's what I would probably try first, maybe it would require more look ahead, or checking around to support all the syntax variations.

  match arg1, arg2
  of 1: discard

  match arg1 = test
  of 1: discard

This can probably be done, though I don't understand why we need to get rid of colon delimiter for this, since every single block statement uses it - block/try/catch/except/finally/if/else/elif/when/of, {.pragma.} section. And macro/template/proc block calls also used this syntax.

Also, how it should treat

myMacro 123
myOf 12: discard

I imagine it should be the same, or somehow considered as well?

@Araq
Copy link
Member

Araq commented Sep 10, 2021

Just to make sure, you mean this, right?

Right.

This can probably be done, though I don't understand why we need to get rid of colon delimiter for this, since every single block statement uses it ...

Because of is not a block statement.

I imagine it should be the same, or somehow considered as well?

Not for me, no, of is special and a keyword.

@haxscramper
Copy link
Author

So you want to make match work the same way as case wrt. to optional colon etc.?

match arg
of 12: ...
else: ...

match arg:
of 12: ...
else: ...

match arg:
  of 12: ...
  else: ...

match arg
  of 12: ...
  else: ...

@haxscramper
Copy link
Author

And note that indenting of might not "make sense" to you when it comes to of, but as a macro author I might have a different opinion about what makes sense when it comes to DSL that I'm implementing, including indentation. gara pattern matching used indentation for arms, https://github.com/alehander92/gara#values

match(a):
  Rectangle: # matches Rectangle(a: 0), Rectangle(a: 2, b: 4)
    echo "rectangle"
  _:
    echo "other"

and so did patty https://github.com/andreaferretti/patty#example

proc sum(xs: List[int]): int = (block:
  match xs:
    Nil: 0
    Cons(y, ys): y + sum(ys[])
)

@Araq
Copy link
Member

Araq commented Sep 10, 2021

I only care about

match arg
of 12: ...
else: ...

The other forms can be supported for consistency, but I don't care. :-)

@haxscramper
Copy link
Author

haxscramper commented Sep 10, 2021

Alright, if we can agree on supporting all forms mentioned above, I will expand my PR if possible. Can I treat your reply as "Let's see what it breaks accepted RFC"

@Araq
Copy link
Member

Araq commented Sep 10, 2021

We can agree on that, yes.

@haxscramper
Copy link
Author

nim-lang/Nim#18798 (comment)

Closing because it seems like it is impossible to implement cleanly, considering all of these ridiculous edge cases that I didn't even know were valid. Basically allowing any kind of flexibility with block arguments would create 'danging else' kind of situation, turning implementation in a completely broken piece of garbage, and it is not worth it.

  readStmt = if 1: quote do: 3 # ok now
    else: quote do: 2

  readStmt = if 1: quote: 3 # ok now
    else: quote: 2

  match: # Supported now
    myOf: 12
  of:
    discard

  match 1: # supported now, would be ambigous to implement
    match 2:
    of 1: discard # For inner one, indented against 'match 2'
    of 1: discard # Meant to be added to outer, or inner?
  of 1: discard # Added to outer one

The same goes for match arg\nof - very ambiguous, current grammar is overly flexible, someone probably already depends on all edge cases that exist, and breaking their code for minor consistency improvements does not make any sense.

@Araq
Copy link
Member

Araq commented Sep 11, 2021

I think the language should be fixed to require else to be in the same column as the if etc. Misaligned code first would produce a warning, of course.

@haxscramper
Copy link
Author

haxscramper commented Sep 11, 2021

We already support too much random AST arrangements, so fixing them is nearly impossible.

import std/macros

dumpTree:
  match if a: a 
  else: b: 
    if c: 
      c
    else: d
    myof: zzz
  of a: q
StmtList
  Command
    Ident "match"
    IfExpr
      ElifExpr
        Ident "a"
        StmtList
          Ident "a"
      ElseExpr
        StmtList
          Call
            Ident "b"
            StmtList
              IfStmt
                ElifBranch
                  Ident "c"
                  StmtList
                    Ident "c"
                Else
                  StmtList
                    Ident "d"
              Call
                Ident "myof"
                StmtList
                  Ident "zzz"
            OfBranch
              Ident "a"
              StmtList
                Ident "q"

That does not make sense to me (I don't really know what should "correct" AST look like for this code. (Colon after b is supposed to close post expr block for match, so second if should be in the main body, but it got parser into call for 'b' for some reason.)), I assume it does not make sense to you either, but I'm pretty sure there is a line somewhere (maybe even in important packages) that relies on this behavior.

Replacing if a: a else: b (expression) with bbb (expression) and leaving colon intact switches 'of' into different subtree.

import std/macros

dumpTree:
  match bbb: 
    if c: 
      c
    else: d
    myof: zzz
  of a: q
StmtList
  Command
    Ident "match"
    Ident "bbb"
    StmtList
      IfStmt
        ElifBranch
          Ident "c"
          StmtList
            Ident "c"
        Else
          StmtList
            Ident "d"
      Call
        Ident "myof"
        StmtList
          Ident "zzz"
    OfBranch
      Ident "a"
      StmtList
        Ident "q"

@Araq
Copy link
Member

Araq commented Sep 11, 2021

I assume it does not make sense to you either, but I'm pretty sure there is a line somewhere (maybe even in important packages) that relies on this behavior.

As long as the code breakage isn't silent I don't mind it too much. We need to have a 2.0 release anyway for --gc:orc. Cannot hurt to make Nim's grammer saner while we're at it.

@haxscramper
Copy link
Author

Assessing damage done by parser changes is relatively easy and can be done for whole ecosystem (at least it was not particularly time-consuming to parse all existing code to figure out stdlib usages), so we can make an informed decision when it comes to considering this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants