fix: support use of constant in other constant and for struct field default value before declaration #1478

i582 · 2025-01-21T22:39:14Z

@jeshecdom I don't know any of the nuances of your vision for this interpreter, so maybe this PR is complete nonsense and it should be done other way. I'd be glad to hear any suggestions!

The main idea of this PR is that we recursively (on demand) calculate the value of a constant without depending on the order. The most important part is to handle circular dependencies correctly, otherwise the compiler will crash with an exception due to the depth of the recursion.

Issue

Closes #1477.
Closes #879.

Checklist

I have updated CHANGELOG.md
I have added tests to demonstrate the contribution is correctly implemented: this usually includes both positive and negative tests, showing the happy path(s) and featuring intentionally broken cases
I have run all the tests locally and no test failure was reported
I have run the linter, formatter and spellchecker
I did not do unrelated and/or undiscussed refactorings

…efault value before declaration Fixes #1477 Fixes #879

jeshecdom

It is a nice solution, I actually like it. It is treating the constants as inlining their definition, as long as they have not been evaluated already. Since evaluation is on demand, it handles cases like this:

const A: Int = B;
const B: Int = foo(1);

function foo(v: Int) {
   if (v == 1) {
      return 0;
   } else {
      return A;   // The else never executes, so no circular dependence at compile-time
   }
}

I was wondering if we could do this process for contract constants, but then I realized that the solution already evaluates contract constants. Note however that contract constants cannot depend on each other, because self is not allowed in their initializers and only calls to global functions are allowed (which cannot reference the contract's constants). Nevertheless, the solution already handles any dependencies contract constants may have with global constants, which is nice.

Just a couple of minimal comments below. Suggestions from other team members are welcomed.

src/optimizer/interpreter.ts

src/types/stmts/constant-as-default-value-of-struct-field.tact

anton-trunov · 2025-01-22T05:12:07Z

Note however that contract constants cannot depend on each other, because self is not allowed in their initializers

@jeshecdom Why is self not allowed?

i582 · 2025-01-22T05:53:45Z

Note however that contract constants cannot depend on each other, because self is not allowed in their initializers

Why is self not allowed?

Looks like this code is not working:

contract Master {
    const C: Int = 10;
    const D: Int = self.C;

    get fun foo(a: Int): Int {
        if (a == D) {
            return 10;
        }
        return 0;
    }
}

With this error:

test/main.tact:6:20: Unable to resolve id 'self'
  5 |     const C: Int = 10;
> 6 |     const D: Int = self.C;
                         ^~~~
  7 |

So we need to support this case in the resolver first in order to work with it in the interpreter
#1481

jeshecdom · 2025-01-22T09:47:05Z

So we need to support this case in the resolver first in order to work with it in the interpreter
#1481

We need to be careful though, because as soon as self is allowed in constant's initializers, the contract constants could be defined using contract fields, meaning that their value will be dependent on what the Interpreter has in its environment at the moment of interpretation:

contract Test {
   f: Int = 5;
   const A: Int = self.foo();
   const B: Int = 10 + self.f;  
}
.........

One may think that in the above code B = 15, but what if self.foo() changes the value of self.f to, say 2?. Then, B = 12. Even worse, changing the order of the constant declarations:

contract Test {
   f: Int = 5;
   const B: Int = 10 + self.f;  
   const A: Int = self.foo();
}
.........

would now give B = 15, because B would be interpreted first. So, I think that references to fields should be avoided in constant declarations (and probably other expressions involving self that I have not thought of).

i582 · 2025-01-22T09:49:02Z

I think that a contract constant cannot (and shouldn't) depend on a contract field

jeshecdom

I approve these changes, unless @anton-trunov and @verytactical have additional comments.

# Conflicts: # src/test/compilation-failed/tact.config.json

.github/workflows/tact.yml

verytactical · 2025-01-23T11:52:18Z

src/types/resolveDescriptors.ts

+): CompilerContext {
+    for (const constant of constants) {
+        if (constant.ast.kind === "constant_def") {
+            constant.value ??= evalConstantExpression(


I'm unsure it's even safe to do it here: typechecking is not over, how can we evaluate it?

But this function is called only after checkConstants, where we check types

Mmmm.... @verytactical has a good point. What if the constant initializer is calling a function that has not been typechecked yet? Or is there a guarantee that before constant evaluation all functions have been typechecked?

Mmmm.... @verytactical has a good point. What if the constant initializer is calling a function that has not been typechecked yet? Or is there a guarantee that before constant evaluation all functions have been typechecked?

Looks like ctx = initializeConstantsAndDefaultContractAndStructFields(ctx, util); is the last step in type checking, so this is not the case

It is correct that it is the last step in resolveDescriptors. But then I checked precompile.ts and noticed that resolveDescriptors is done before resolveStatements, which means that functions are actually not type-checked before evaluating constants.

This explains why the tests did not complain about assigning an integer to self (which has the type of a contract), like self = 100. I was wandering about that when reading the tests.

If we want to keep this behavior, then the interpreter should not rely on the type-checker and carry out type checks while it interprets the code (we will need to extensively test the interpreter). The other option is to move constant evaluation as the last step in precompile.ts.

@anton-trunov, we need your opinion on this, because I do not know if some process after resolveDescriptors in precompile.ts requires constants to be already evaluated.

const eval should be done after typechecking, it's just historically it wasn't the case :)

src/optimizer/interpreter.ts

anton-trunov · 2025-01-26T07:51:06Z

src/types/resolveDescriptors.ts

+): CompilerContext {
+    for (const constant of constants) {
+        if (constant.ast.kind === "constant_def") {
+            constant.value ??= evalConstantExpression(


const eval should be done after typechecking, it's just historically it wasn't the case :)

anton-trunov · 2025-01-26T08:00:37Z

src/types/stmts/const-eval-almost-constant-circular-dependency.tact

const eval tests should not go to the typechecker tests

anton-trunov · 2025-01-26T08:26:03Z

src/test/compilation-failed/const-eval-failed.spec.ts

let's add a test with nested function calls foo() -> bar() -> ...

Also, let's add a test like const A: Int = A

anton-trunov · 2025-01-26T18:12:26Z

src/optimizer/interpreter.ts

+                this.constantComputationPath.push(name);
+                constant.value = this.interpretExpression(
+                    constant.ast.initializer,
+                );
+                this.constantComputationPath.pop();


Since these push / pop operations need to be always balanced, let's define a helper that ensures that

fix: support use of constant in other constant and for struct field d…

5a68b9c

…efault value before declaration Fixes #1477 Fixes #879

i582 added this to the v1.6.0 milestone Jan 21, 2025

i582 requested a review from jeshecdom January 21, 2025 22:39

i582 requested a review from a team as a code owner January 21, 2025 22:39

added CHANGELOG.md entry

56fbbd2

jeshecdom reviewed Jan 22, 2025

View reviewed changes

src/optimizer/interpreter.ts Outdated Show resolved Hide resolved

src/optimizer/interpreter.ts Outdated Show resolved Hide resolved

src/types/stmts/constant-as-default-value-of-struct-field.tact Show resolved Hide resolved

fixed review issues and added more positive tests

5555a4a

use ??=

526ec6b

jeshecdom reviewed Jan 22, 2025

View reviewed changes

Merge branch 'main' into pmakhnev/fix-constant-initialization

ca42a3a

i582 requested review from anton-trunov and verytactical January 22, 2025 16:59

i582 added 4 commits January 22, 2025 21:08

Merge branch 'main' into pmakhnev/fix-constant-initialization

a9fe927

fix

8c280eb

Merge branch 'main' into pmakhnev/fix-constant-initialization

d8f6825

# Conflicts: # src/test/compilation-failed/tact.config.json

merge

9404799

verytactical reviewed Jan 22, 2025

View reviewed changes

.github/workflows/tact.yml Outdated Show resolved Hide resolved

Merge branch 'main' into pmakhnev/fix-constant-initialization

2f26c16

verytactical reviewed Jan 23, 2025

View reviewed changes

Merge branch 'main' into pmakhnev/fix-constant-initialization

327cf18

anton-trunov self-assigned this Jan 24, 2025

anton-trunov requested changes Jan 26, 2025

View reviewed changes

anton-trunov reviewed Jan 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support use of constant in other constant and for struct field default value before declaration #1478

fix: support use of constant in other constant and for struct field default value before declaration #1478

i582 commented Jan 21, 2025 •

edited

Loading

jeshecdom left a comment

anton-trunov commented Jan 22, 2025

i582 commented Jan 22, 2025 •

edited

Loading

jeshecdom commented Jan 22, 2025 •

edited

Loading

i582 commented Jan 22, 2025

jeshecdom left a comment

verytactical Jan 23, 2025

i582 Jan 23, 2025

jeshecdom Jan 23, 2025

i582 Jan 24, 2025

jeshecdom Jan 24, 2025

anton-trunov Jan 26, 2025

anton-trunov Jan 26, 2025

anton-trunov Jan 26, 2025

anton-trunov Jan 26, 2025

anton-trunov Jan 26, 2025

anton-trunov Jan 26, 2025

fix: support use of constant in other constant and for struct field default value before declaration #1478

Are you sure you want to change the base?

fix: support use of constant in other constant and for struct field default value before declaration #1478

Conversation

i582 commented Jan 21, 2025 • edited Loading

Issue

Checklist

jeshecdom left a comment

Choose a reason for hiding this comment

anton-trunov commented Jan 22, 2025

i582 commented Jan 22, 2025 • edited Loading

jeshecdom commented Jan 22, 2025 • edited Loading

i582 commented Jan 22, 2025

jeshecdom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i582 commented Jan 21, 2025 •

edited

Loading

i582 commented Jan 22, 2025 •

edited

Loading

jeshecdom commented Jan 22, 2025 •

edited

Loading