solidity icon indicating copy to clipboard operation
solidity copied to clipboard

Immutable variables treated differently depending on type

Open frangio opened this issue 8 months ago • 8 comments

Description

An immutable variable that is initialized at declaration site can be used in pure context if it is of type uint but not if it is of type address.

Environment

  • Compiler version: 0.8.29

Steps to Reproduce

// SPDX-License-Identifier: GPL-3.0

pragma solidity 0.8.29;

contract Test {
    address internal immutable _a = 0x0000000000000000000000000000000000000001;
    uint256 internal immutable _b = 42;

    function a() external pure returns (address) {
        return _a; // TypeError: Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
    }

    function b() external pure returns (uint256) {
        return _b; // no error
    }
}

frangio avatar Apr 08 '25 19:04 frangio

I can confirm that this is the current behavior. As to whether it's a bug - it's complicated.

Integers and fixed byte arrays that have an initializer, which is a literal are intentionally considered pure. This does not apply to addresses or other value types (booleans, internal functions, contracts, enums, probably also UDVTs).

This seems to have been introduced as a feature in 0.7.5 (#10240). The motivation in the request (#9554) was that it was not possible to define a pure getter in an interface and override it with an immutable state variable in a contract that implements it. It was concluded back then that immutables do not really fit the current notion of pure in general, but in special cases, where we know they won't change, we could consider them as such.

So it generally works as intended, but with one wrinkle. This seems to have been done with the assumption that having the initializer prevents the immutable from being reassigned later. We later decided to relax that limitation (#14240) and it's already partially lifted (#14304) - immutables can be reassigned in constructor. Not sure yet if that may have some weird consequences (it would probably be much worse if we allowed reassignments in functions), but this means that we need to reconsider whether we should scrap this feature before relaxing it further.

cameel avatar Apr 09 '25 12:04 cameel

I honestly don't understand why an address should be considered differently from a bytes20 or a uint160.

This "maybe not a bug" is pushing us to cast objects that are clearly addresses to other types (of the same size) to work around issues. This is obfuscating the reality of the objects, overall decreassing readability and security of our code.

Amxx avatar Apr 09 '25 13:04 Amxx

in special cases, where we know they won't change, we could consider them as such

If this is the intended rule there is no reason not to apply it to every type.

But yes, I noticed what you point out, the original reasoning to justify making it pure doesn't hold any longer, which makes this a bug for a different reason. It was never the intention that this should compile:

contract Test {
    uint256 internal immutable _b = 0;

    constructor() {
        _b = block.number;
    }

    function b() external pure returns (uint256) {
        return _b;
    }
}

And the fact that adding an explicit = 0 makes it compile is extremely unintuitive.

frangio avatar Apr 09 '25 14:04 frangio

We discussed this on the Wednesday call:

  1. The first priority is to make sure that this bug does not have any other weird consequences. E.g. only the initial value being seen in some circumstances.
  2. We'll fix the bug by considering the immutable pure only if it does not get reassigned.
  3. We should make it work for all types. It will only depend on whether the initializer is pure.
  4. Document this behavior.

cameel avatar Apr 10 '25 18:04 cameel

I honestly don't understand why an address should be considered differently from a bytes20 or a uint160.

Yeah, it seems like an artificial restriction. I don't think it was even meant to remain like this long term - #10240 looks to me like just a first quick step to address the immediate issue without opening it up too much and risking unforeseen issue, but also not closing the way for further extension.

But yes, I noticed what you point out, the original reasoning to justify making it pure doesn't hold any longer, which makes this a bug for a different reason.

True. This should definitely be considered a bug. Since pure, unlike view is just an abstraction at compiler level, this bug does not really allow doing anything that could not be done otherwise (you could just pass it via memory or obtain the value via a pure external call), but it does break expectations and this was not meant to work that way.

And the fact that adding an explicit = 0 makes it compile is extremely unintuitive.

I mean, this "feature" is inherently unintuitive and hard to discover. Assuming we don't want to remove it. It would be better if it did not matter where the variable was initialized, only how, but the complexity of that and all the bugs around it is exactly why we decided to relax the restrictions on immutables. I'm not sure we can do anything else other than just document it though, assuming we want to keep it.

cameel avatar Apr 10 '25 18:04 cameel

BTW, this discussion has again brought the topic of pure having two possible interpretations and not sticking 100% to either (#8153, #12256). We're thinking whether we should change that in 0.9.0.

Personally, I agree with @ekpyron's latest stance (https://github.com/ethereum/solidity/issues/12829#issuecomment-1087273314) that we should make pure more permissive and either introduce a new modifier for things that are a compile-time constants or just detect it via analysis. It would actually not change all that much. The biggest one I can think of would be making all immutables pure in runtime context and allowing inline assembly opcodes for accessing code in pure functions (we'd drop #12260).

To be clear, this would still not really affect this issue. In construction context immutables are essentially variables so we have to fix it regardless.

cameel avatar Apr 10 '25 19:04 cameel

FWIW I think it would be fine to entirely disallow reading immutable variables in pure context. In general, immutable variables are not compile time constants, by design. The fact that some of them sometimes are shouldn't change that. The compilability of an expression should be determined by the types, not by the structure of the code (e.g., whether there is an assignment in the constructor).

The underlying issue is that users prefer immutable because it gives them a guarantee that the expression will not be recomputed on every occurrence of the variable, which constant does not guarantee (or at least historically did not).

The fix to this situation is to ensure that constant variables are computed at compile time so that users don't lean on immutable for that and then complain about the limitations of immutable variables.

frangio avatar Apr 10 '25 20:04 frangio

Good point. I guess in all cases where we're fine making the immutable pure, it can syntactically be simply replaced with a constant. You can even use a public constant to solve the original override problem. It sounds like this is the direction we should have gone with this.

cameel avatar Apr 10 '25 21:04 cameel