tact-lang / tact

Tact compiler main repository
https://tact-lang.org
MIT License
275 stars 56 forks source link

Limit usage of `ASTStatementExpression` #366

Closed byakuren-hijiri closed 3 weeks ago

byakuren-hijiri commented 4 weeks ago

The grammar contains the following line: https://github.com/tact-lang/tact/blob/37ff1512a6cd68ad795e71f1666f95d95ab96e0b/src/grammar/grammar.ohm#L106

This means that all kinds of expressions can be placed into a block of statements. Here is a valid code that passes compilation:

contract SampleTactContract with Deployable {
    init() {
        -4;
        2 == 2;
        true;
    }
}

Actually, all the expressions in the AST do not have any side effects, except function and method calls.

Moreover, having an expression like this: a + b sometimes means that the developer forgot to add an assignment operation, like c = a + b. Therefore, the scope of that error is similar to read-only variables analysis provided by many compilers.

My suggestion is to limit the usage of ASTStatementExpression to contain only ASTOpCall or ASTOpCallStatic.

anton-trunov commented 4 weeks ago

all the expressions in the AST do not have any side effects, except function and method calls.

Expression evaluation may result in TVM side-effects: various kinds of exceptions, like integer overflows, out-of-gas errors, etc. and also we have some control flow incorporated into expressions:

And the language of conditions is complex enough for us not to disallow its usage.

novusnota commented 4 weeks ago

Adding to what @anton-trunov has said, there are at least two more use-cases:

  1. Currently I use StatementExpression to test highlighting and correct parsing of expressions in tree-sitter-tact. Moving away to something else, like an assignment statement (_ = Expression; over Expression;) is easy, but will introduce some noise to tests.

  2. I think that various showcasing matters benefit from StatementExpression. Like, when teaching Tact to people in a "learnxinyminutes" kind of style or stuff like that.

anton-trunov commented 3 weeks ago

Discussed this with @byakuren-hijiri a bit more elsewhere and decided to close this issue. This can be covered by static analysis