ethereum / fe

Emerging smart contract language for the Ethereum blockchain.
https://fe-lang.org
Other
1.6k stars 178 forks source link

v2 parser: change assignment's lhs to expression #985

Closed saifalkatout closed 6 months ago

saifalkatout commented 6 months ago

resolves #875

What was wrong?

The new parser (v2 branch) wrongly supposes the assignment destination is a pattern, but it should be an expression.

How was it fixed?

change Assign and augassign to expressions

saifalkatout commented 6 months ago

Hey @Y-Nak, I created a new clean pull request for the parser issue. Just a few comments: 1) Are the lazyspans acceptable ? do I need to write a test for the LazyAssignSpan ?

2) The new bps will replicate the following issue x += 1 + 1 well produce

BinExpr@52..62
  AugAssignExpr@52..58
    PathExpr@52..53
      Path@52..53
        PathSegment@52..53
          Ident@52..53 "x"
    WhiteSpace@53..54 " "
    AugAssignExpr@54..58
      Plus@54..55 "+"
      Eq@55..56 "="
      WhiteSpace@56..57 " "
      LitExpr@57..58
        Lit@57..58
          Int@57..58 "1"
  WhiteSpace@58..59 " "
  Plus@59..60 "+"
  WhiteSpace@60..61 " "
  LitExpr@61..62
    Lit@61..62
      Int@61..62 "1"

3) commits will be squashed on finish

Y-Nak commented 6 months ago

First and foremost, please learn the basic usage of git. And please use git in the proper way since the next PR. There is a vast amount of learning resources available online. This is the third PR on the same issue. I think it is generally not preferable to have reviews dispersed across multiple PRs, and it also increases my workload (albeit slightly). While I believe that the flow where reviewers and authors work together to resolve issues in the code is excellent, but frankly speaking, I don't want to teach basic git usage.

Are the lazyspans acceptable ? do I need to write a test for the LazyAssignSpan ?

We need to get = span in AssignExpr, and don't need to get expr because all spans of expr is directly stored in Body. ~Also, we need to get binary operator span in AugAssignExpr~ sorry it seems this has been already implemented.

The new bps will replicate the following issue x += 1 + 1 well produce

This problem has nothing to do with the binding power. You just need to modify Plus | Minus arm as you did in Star | Slash | Percent arm.

saifalkatout commented 6 months ago

First and foremost, please learn the basic usage of git. And please use git in the proper way since the next PR. There is a vast amount of learning resources available online. This is the third PR on the same issue. I think it is generally not preferable to have reviews dispersed across multiple PRs, and it also increases my workload (albeit slightly). While I believe that the flow where reviewers and authors work together to resolve issues in the code is excellent, but frankly speaking, I don't want to teach basic git usage.

Basically what I was trying to do is open a new PR and then cherry pick the commits from the old one. A simple rebase back into the old PR and deleting this one would fix the issue of having the work dispersed over 2 PRs, at any rate, apologies for the inconvenience.

sbillig commented 6 months ago

Basically what I was trying to do is open a new PR and then cherry pick the commits from the old one.

Instead of opening a new PR, you could have, for example, created a new branch locally, cherry-picked the commits to it, reset your original branch to the new branch head, and force pushed. With git, you can always make your local branch look exactly how you want it, then you can force push to github so that your PR reflects your local branch.

It's usually easiest to work on a new branch (with a name like "augassign-expr") for each PR, then if changes happen on the main branch (fe-v2), you can pull those changes to your local fe-v2 branch, and rebase your PR branch onto the main branch, and git push --force to github. This way, your PR will always only contain your commits, not a bunch of unrelated commits by other people. Anyway, being good at git is pretty important in modern software development, so it's worth the time investment to learn. (If you're using a gui, I'd recommend using the command line until you can do everything there, then feel free to use a gui for convenience for regular daily workflow.)

saifalkatout commented 6 months ago

Yeah I was kind of way off with my solution. Also, creating a new local branch probably would've been much cleaner.

Anyway, being good at git is pretty important in modern software development, so it's worth the time investment to learn.

I will definitely read more into git and start using git from the command line. Thanks for the advice.

Y-Nak commented 6 months ago

LGTM. Thanks!