Incorrect location generated when desugaring arithmetic assign ops (e.g., a += b)

[BenTheKush]

Describe the bug
An expression a += b is parsed as a pt::Expression::AssignAdd{left, right}. When this is translated from a pt to an ast it is desugared into an Assign { ..., Expression::Add { left, right}}. When left is a Storage access, left's location is wrong when it gets desugared: it ends up spanning the entire AssignAdd expression instead of just the left expression.

To Reproduce
I have a repository that reproduces the bug.

Hyperledger Solang version

solang = { git = "https://github.com/hyperledger/solang.git", rev = "70af554c42748009e414e6263dd4607fb380e5dc", default-features = false }
solang-parser = { git = "https://github.com/hyperledger/solang.git", rev = "70af554c42748009e414e6263dd4607fb380e5dc" }

Include the complete solidity source code

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity >0.7.0;
pragma experimental ABIEncoderV2;

contract foo {
    uint256 private _totalSupply;

    function updateStorage() public {
        _totalSupply += 1;
        _totalSupply *= 1;
        _totalSupply /= 1;
        _totalSupply -= 1;
        _totalSupply &= 1;
        _totalSupply |= 1;
    }

    function updateVariable() public pure {
        uint256 x;
        x += 1;
        x *= 1;
        x /= 1;
        x -= 1;
        x &= 1;
        x |= 1;
    }
}

Additional context
Running my code visits each binary expression (after desugaring), and checks to ensure that the left.loc().end() < right.loc.start(), and if not, prints an error. This is the output of my test on the above program:

$ cargo run                      
   === Function: updateStorage() ===

Error: start: 213 >= end 212 in expression _totalSupply += 1
    left: _totalSupply += 1
    right: 1

Error: start: 240 >= end 239 in expression _totalSupply *= 1
    left: _totalSupply *= 1
    right: 1

Error: start: 267 >= end 266 in expression _totalSupply /= 1
    left: _totalSupply /= 1
    right: 1

Error: start: 294 >= end 293 in expression _totalSupply -= 1
    left: _totalSupply -= 1
    right: 1

Error: start: 321 >= end 320 in expression _totalSupply &= 1
    left: _totalSupply &= 1
    right: 1

Error: start: 348 >= end 347 in expression _totalSupply |= 1
    left: _totalSupply |= 1
    right: 1

   === Function: updateVariable() ===

Success

Success

Success

Success

Success

Success  

It's clear that the bug exists only when the LHS of an op-assign (+=, |=) is a storage access. This does not occur for local variables.

[BenTheKush]

For what it's worth, from a client perspective I think there is strong value added for an AST library to not do this desugaring of a += b -> a = a + b. I may want to treat these two operations differently, and it's possible that I will try to access a source location that doesn't exist.

For instance, there is no way for me to access the location of the + in add expression aside from looking between the left and right expressions in the original source. But now, I need to double check that I don't end up with a += instead of a +...it's weird that we aren't guaranteed that Add{left, right} corresponds to a + operator in the original code.

Anyway, not the point of this issue, just my two cents

[seanyoung]

You're right, we should have an Expression::AddAssign in the AST. There are a few other things we de-sugar which we should not, like type() functions.