v2 parser: change assignment's lhs to expression

[saifalkatout]

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?

Parse lhs in assignment statements as an expression.

[saifalkatout]

@Y-Nak Done.

[saifalkatout]

So for parser::stmt::AugAssignStmtScope. It's accepting SK::Ident in the lhs, should I change that to the SKs that are in exprs? like Lbracket, LParen, Lt, etc... or maybe we can change the RE of ident ??

[Y-Nak]

Basically, what you need to do is just to call parse_expr to parse lhs in AugAssignStmtScope. But the problem is you probably need to restrict binding power to avoid parse_expr to parse an operator that should bind to = rather than lhs. Say, if you have x.y += 1, then parse_expr should terminate parsing just before +. Also, you probably need to modify parser2::ast and hir::lower. I feel this might be beyond the good-first-issue scope. If you think this is troublesome, I'd be happy to take over.

[saifalkatout]

I want to give it a try if that's ok.

[Y-Nak]

Ok, thanks

[saifalkatout]

Hey Yoshi, this is a rough sketch of what im trying to do with AugAssign. Could you check if I'm on the right path with infix_binding_power and BinExprScope ? Note that I didn't change bump_bin_op bcuz it doesn't return a value. So I just added my own function which I will populate later with -=, *=, etc... Also when I get the ok on this approach I'll get started with the assign.

[Y-Nak]

Sorry for the late response. This doesn't seem a right approach.

1. set_kind usage is wrong(You don't need to define Scope and call them if you use set_kind)
2. The way of lowering the AugAssign is semantically wrong. Also, AugAssign(ExprId, ExprId, BinOp) would be the better definition; this means you don't need to desugar AugAssign.
3. As 2. indicates, you can remove pub enum AugAssignDesugared.
4. You don't need to define PlusEqScope or LShiftEqScope just to check if the binary operator is AugAssign since those scopes have no corresponding SyntaxKind. What you need to do is 1. Seeing the current token is a binary operator, then proceed parser to the next token and see if the next token is = in dry run mode.

For example, The expected CST for the below expression

x += 1 + 1

is

  AugAssignExpr@52..62
    PathExpr@52..53
      Path@52..53
        PathSegment@52..53
          Ident@52..53 "x"
    WhiteSpace@53..54 " "
    Plus@54..55 "+"
    Eq@55..56 "="
    WhiteSpace@56..57 " "
    BinExpr@57..62
      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

, and not the one below that this PR generates.

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"

[saifalkatout]

Hey Yoshi, this is some bare bones changes, I want to see what CSTs will be generated

[saifalkatout]

Hey Yoshi, regarding the latest push:

1) The reason exprs like x += 1 + 1 was not working even after the changes on Oct 9th was because the infix_binding_power I had for eq was wrong (to fix it I used the following article as reference: https://rust-analyzer.github.io/blog/2020/09/16/challeging-LR-parsing.html)

2)This is the CST created from x += 1 + 1:

AugAssignExpr@0..10
      PathExpr@0..2
        Path@0..1
          PathSegment@0..1
            Ident@0..1 "x"
        WhiteSpace@1..2 " "
      Plus@2..3 "+"
      Eq@3..4 "="
      WhiteSpace@4..5 " "
      BinExpr@5..10
        LitExpr@5..6
          Lit@5..6
            Int@5..6 "1"
        WhiteSpace@6..7 " "
        Plus@7..8 "+"
        WhiteSpace@8..9 " "
        LitExpr@9..10
          Lit@9..10
            Int@9..10 "1"

3) I wanted to finish through with the stmt approach. Now I must research which is better; aug/augassign being expr or stmt, this is the final thing we agreed on in the meeting right ?

4) is lowering into hir and mir in the scope of this PR, for now I have this in the hir:

and I will have to write tests for our new augassign expression.

5) The lint fixes (clippy), are caching somewhere, thats why it keeps failing, I think I'm facing this issue: https://github.com/rust-lang/rust-clippy/issues/8991, do you have any advice ?

[Y-Nak]

3. I wanted to finish through with the stmt approach. Now I must research which is better; aug/augassign being expr or stmt, this is the final thing we agreed on in the meeting right ?

IIRC, we decided to make them exprs. I tried to make them stmts because I thought it might be better to restrict the side-effect location to statements. But I couldn't have enough time to consider it fully, and we decided to have mutable references. So, I can't come up with any advantages to having them as statements now.

4. is lowering into hir and mir in the scope of this PR, for now I have this in the hir:

Almost all parts of MIR will be discarded, so I think you don't need to modify MIR.

5. The lint fixes (clippy), are caching somewhere ...

You have two options.

1. Make your worktree clean.
2. Fix them manually. NOTE clippy-fix doesn't apply to all suggestions, please refer to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint_defs/enum.Applicability.html#variants. IIRC, only lints coming with Applicability::MachineApplicable are automatically applicable. This means you might need to fix some of them by hand.

[Y-Nak]

Oh, regarding the answer about 3., I did not mean you should not try to find a better solution. I just described my thoughts and the current situation. Your research is always welcome, of course.

[saifalkatout]

Hey Yoshi, just some last comments from my end:

1) Clippy is failing on some files I didn't edit (https://rust-lang.github.io/rust-clippy/master/index.html#/filter_map_bool_then). Maybe this was a change in Clippy policy ? If you want I can go around fixing each one of the cases, I already did some: https://github.com/ethereum/fe/pull/936/commits/55cb951172fd01dd3cb85fcd3bd22da61169bb1e

2) I believe, if we want to follow Rust defns, an assign/augassign would fit more as an expr (I am sure you know this already, I just wanted to tell you what I found on this). Also, could you help me understand how mutable references would affect our decision ?

3)I would still need to squash changes into 1 commit, just need to know how to proceed with 1.

[Y-Nak]

Clippy is failing on some files I didn't edit

Rust publishes a new version every six weeks, and Clippy as well. It often causes new lint errors that are detected by newly added lint pass of Clippy.

If you want I can go around fixing each one of the cases, I already did some: https://github.com/ethereum/fe/commit/55cb951172fd01dd3cb85fcd3bd22da61169bb1e

I fixed them in #931, so there is no need to fix them.

Also, could you help me understand how mutable references would affect our decision ?

I vaguely thought that it might be nice if we could guarantee side effects only happen in stmt. In this idea, stmt is a kind of escape hatch to allow side effect, and all expressions works as if purely functional language with linear types. But I didn't pursue the idea seriously, and I ended up thinking that having mutable references would be a better design and an easier way to achieve our goal. Assuming having mutable references, we can't avoid side effects in expression, and if so, there is no advantage to making them stmt.

would still need to squash changes into 1 commit

You can squash them into multiple commits that have reasonable granularity. I mean, you don't necessarily need to squash them to a single commit. But it's up to you.

[saifalkatout]

Hey @Y-Nak, do you think the granularity is acceptable like this ? GH doesn't allow squashing merge commits, I found a solution for this by reverting all the changes then merging to the latest commit and pushing without syncing with the main branch, please let me know if this is necessary. Other than this, I think we can merge ?

[saifalkatout]

hey @Y-Nak could u check my latest comments plz