Do notation changes

[krux02]

• disable automatic nkStmtList to proc lambda lifting
• disable automatic nkDo node eating of the parser

Unfortunately it is a common pattern to pass two blocks of code to a template with the do notation. These templates are now broken. To fix them there is now a stripDoNode macros in macros.nim that can now manually be injected to strip the nnkDo node which was previously stripped by the parser.

Since nnkStmtList is not automatically converted to a proc argument, users are now required to pass them as lambda expressions. We have three syntaxes to choose from.

Note:

Don't merge it yet. This is a draft to see how many packages would break if this is changed.
Missing:

• compiler flag for old behavior
• documentation

[dom96]

It seems to me like we need to rethink the do notation from the ground up. This notation doesn't feel like it fits the language, we've already decided that it will be removed from closures and for templates we should really create a different keyword or come up with an alternative way to pass more than one body.

This change seems like a small thing that tries to fix some flaws in the do notation, and it just makes it more prevalent and more annoying to use. Why should templates have to strip the do nodes?

[krux02]

@dom96

It seems to me like we need to rethink the do notation from the ground up.

Maybe, but the do notation is part of Nim for quite some time now.

This notation doesn't feel like it fits the language, we've already decided that it will be removed from closures

Well, as an ugly syntax to pass a closure as the last argument of a proc, it doesn't really cause problems.

and for templates we should really create a different keyword or come up with an alternative way to pass more than one body.

I use this pattern, works like a charm:

myMacro:
  body1:
    echo 1
    echo 2
  body2:
    echo 3
    echo 4

This change seems like a small thing that tries to fix some flaws in the do notation, and it just makes it more prevalent and more annoying to use. Why should templates have to strip the do nodes?

This is not just about do. Before a node of kind nnkStmtList was implicitly converted into lambda expression during compilation. nnkStmtListExpr, or just a statement like a call was not converted to a lamba. This arbitrary logic was only applied, because for some other arbitrary logic decided that a do block without arguments is not a do block anymore, it is just a nnkStmtList. But then the stmtList needs to work as a lambda expression. This is just bad because it affects any statement list.

Templates should not be written with the do notation in mind. The do notation is just an ugly syntax for lambda expressions. For historical reasons the do notation has been used for some templates, because it was the best option at the time. I found a fix to let them keep their syntax, but I would not recommend to design templates like this anymore. Nim does not need for every corner case a feature that makes it look super nice.

[dom96]

Maybe, but the do notation is part of Nim for quite some time now.

Yes, but it feels like such a bolted on feature to me that I think it's worth it to deprecate it and provide a good alternative.

I use this pattern, works like a charm:

This works well, but writing a macro for simple templates can be a PITA. Maybe if we could create a macro to make it simpler or at least some code examples that can be readily copy and pasted we could give people a smooth transition?

This is not just about do.

Indeed, but it breaks templates that use it. Since you're already making this breaking change, why not take it a step further and just deprecate the do notation while you're at it? :)

[mratsim]

Regarding your example

myMacro:
  body1:
    echo 1
    echo 2
  body2:
    echo 3
    echo 4

does that match with

macro myMacro(body1, body2: untyped)

because if so, I think that sufficiently ergonomic to replace the do notation altogether.

I agree with @dom96, the do notation feels foreign, especially the absence of indentation for it (like case of actually). If we have a good alternative we should remove it.

With the changes, I'm concerned about backward compat (#12114 (comment))

[krux02]

This works well, but writing a macro for simple templates can be a PITA.
I would argue that a template that takes multiple blocks of code is not really a simple template.

Maybe if we could create a macro to make it simpler or at least some code examples that can be readily copy and pasted we could give people a smooth transition?

Yea maybe.

Indeed, but it breaks templates that use it. Since you're already making this breaking change, why not take it a step further and just deprecate the do notation while you're at it? :)

Well because I think the ugly do syntax is still the best syntax we have to pass multiline lambda expression to procs:

sortable.sort do (a, b: int) -> int:
  result = cmp( abs(base - a), abs(base - b) )
  # Yea, not really multiline, but I don't have a better example right now.

[krux02]

@mratsim I am afraind, no it is not that simple. This is more complete:

macro myMacro(metabody: untyped) =
  var body1, body2: NimNode
  for body in metabody:
    body.expectKind nnkCall
    if body[0].eqIdent "body1":
      body1 = body[1]
    elif body[0].eqIdent "body2":
      body2 = body[1]
    else:
      error("unexpected section", body)

  # [...] do stuff with body1 and body2
  echo body1.treeRepr
  echo body2.treeRepr

myMacro:
  body1:
    echo 1
    echo 2
  body2:
    echo 3
    echo 4

If this is really a demanded common pattern, I could write a helper macro for this.

Instruction unclear.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

[krux02]

since the tests are passing, this PR is ready to be merged.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.