Make ImmutableValidator more precise while analyzing branches

[StrongerXi]

Motivation

Addresses #12864

Mechanism

1. Delay error generation. Originally we issue error as soon as we see any write to immutable state variable inside a branch, that's can be too conservative, e.g., when both branches write to the variable.
2. Collect all immutable state variable written in a branch (and their position, obtained via the write expression). After analyzing both branch, we can inspect the set difference, and determine which ones are not written (thus initialized) in both paths. Then we issue error for those variables, and collect the all written to continue this process while avoiding duplicated error generation.

Backward Compatibility

This shouldn't break backward compatibility because we are generating less errors, i.e., correct programs remain correct. The error message is changed, but we can discuss and tweak that.

Test Case

contract Immutable {
    uint256 public immutable iamImmutable;

    constructor(bool shouldInitWithZero) {
        if (shouldInitWithZero) {
            if (shouldInitWithZero) {
                iamImmutable = 0;
            } else {
                iamImmutable = 1;
            }
        } else {
            if (true) {
                iamImmutable = 1;
            }
        }
    }
}

This generates a single error at the last write to iamImmutable. If we remove the last conditional if (true), no error is issued.

Alternative

I actually think the Java way of handling such errors is worth to consider, i.e., it's no less precise than what this PR aims to achieve, and it issues a single error message at the variable declaration site, saying the final field ... may not have been initialized. Which is similar to error 2658 in Solidity.

This approach makes the algorithm a bit simpler -- we just need to keep track and filter out the variables-not-initialized-in-both-branches when we issue error 2658, but we lose the location of the problematic write.

[StrongerXi]

The more I think about it, the more I believe the alternative approach is preferable. Especially true for cases like this:

contract Immutable {
    uint256 public immutable iamImmutable;

    constructor(bool shouldInitWithZero) {
        if (shouldInitWithZero) {
            if (shouldInitWithZero) {
                iamImmutable = 0;
            }
        } else {
            if (true) {
                iamImmutable = 1;
            }
        }
    }
}

[chriseth]

Thank you for the pull request! I fear that the complete solution is a bit more complicated. Can you just ignore all the cases after the else if (m_inBranch)?

[StrongerXi]

I fear that the complete solution is a bit more complicated

Would you mind elaborating on what edge case I might be missing? Based on my limited understanding of Solidity, it seems sound if we take the alternative approach, i.e., mark any variable written only in 1 of the if/else branches to be "not properly initialized".

Can you just ignore all the cases after the else if (m_inBranch)?

I might be misunderstanding, do you mean deleting all these chunks?

		else if (m_initializedStateVariables.count(&_variableReference))
		{
			if (!read)
				m_errorReporter.typeError(
					1574_error,
					_expression.location(),
					"Immutable state variable already initialized."
				);
			else
				m_errorReporter.typeError(
					2718_error,
					_expression.location(),
					"Immutable variables cannot be modified after initialization."
				);
		}
		else if (read)
			m_errorReporter.typeError(
				3969_error,
				_expression.location(),
				"Immutable variables must be initialized using an assignment."
			);
		m_initializedStateVariables.emplace(&_variableReference)

[StrongerXi]

Rebase.

[Marenz]

I am not sure what exactly your suggestion for the alternative approach is.

This approach makes the algorithm a bit simpler -- we just need to keep track and filter out the variables-not-initialized-in-both-branches when we issue error 2658, but we lose the location of the problematic write.

Problematic write means a conditional write?

Could you explain the alternative approach a bit?

[chriseth]

The main issue I have with this PR is that I'm not convinced that it properly detects multiple writes to a variable in the same branch. Reporting the error about "not initialized" on the variable seems fine to me.

[chriseth]

The main issue I have with this PR is that I'm not convinced that it properly detects multiple writes to a variable in the same branch. Reporting the error about "not initialized" on the variable seems fine to me.

[StrongerXi]

@chriseth @Marenz I updated using the alternative approach. The extra check in ImmutableValidator::checkAllVariablesInitialized should make the high-level logic pretty clear.

Regarding proper detection of "multiple writes to a variable in the same branch", I believe error 2718 already takes care of it. I also added a test case to illustrate that.

[StrongerXi]

Rebase.

[StrongerXi]

I didn't find any code logic errors, mostly style. Oh, a changelog entry is also missing. Other than that, looks good to me.

Thanks!

1. All comments are addressed
2. Changelog entry added
3. ImmutableValidator.h documentation updated.

[StrongerXi]

1. Addressed all comments.
2. Added comments to help clarify code logic.
3. Rebase.

[chriseth]

Does this need updated documentation?

[Marenz]

@StrongerXi would you like to address the open comments or should we take the PR over from here?

[StrongerXi]

@StrongerXi would you like to address the open comments or should we take the PR over from here?

My apologies. Got very busy during the last few weeks. Will push a commit by end of today.

[Marenz]

My apologies. Got very busy during the last few weeks. Will push a commit by end of today.

All good, just pinging to see whats up :)

[StrongerXi]

Does this need updated documentation?

I already updated the documentation in the header.

[Marenz]

I already updated the documentation in the header.

In this case "documentation" means the files under docs/, our user-side documentation

[StrongerXi]

I already updated the documentation in the header.

In this case "documentation" means the files under docs/, our user-side documentation

I see. Just did some greps in docs/ and the only relevant section seems to be this. It doesn't mention anything about initialization constraints tho.

What do you think we should do here?

[StrongerXi]

Rebase.

[StrongerXi]

Address comment, rebase, & update changelog.

[StrongerXi]

I believe you also need to add a boost test to test/libsolidity/analysis/FunctionCallGraph.cpp, but I may be mistaken. See BOOST_AUTO_TEST_CASE(immutable_initialization).

Thanks, done.

[StrongerXi]

Offline chat -- a more general approach is preferred instead of covering control flow one-by-one (e.g., another case would be #13433).

[ekpyron]

I'm sorry about not doing this sooner, but especially since we will replace the immutable logic by a code data location that will not require the write-once restriction in the constructor at all, the added complexity here is not worth the temporary benefits it may or may not bring.

So thank you very much for the contribution and for your efforts, and again I apologize for not clarifying this sooner, but I'll be closing this PR now.