Skip to content

Immutable variables treated differently depending on type #15989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
frangio opened this issue Apr 8, 2025 · 8 comments
Open

Immutable variables treated differently depending on type #15989

frangio opened this issue Apr 8, 2025 · 8 comments
Labels
bug 🐛 medium effort Default level of effort medium impact Default level of impact must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it.

Comments

@frangio
Copy link
Contributor

frangio commented Apr 8, 2025

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
    }
}
@cameel
Copy link
Member

cameel commented Apr 9, 2025

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.

@Amxx
Copy link

Amxx commented Apr 9, 2025

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.

@frangio
Copy link
Contributor Author

frangio commented Apr 9, 2025

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.

@cameel
Copy link
Member

cameel commented Apr 10, 2025

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
Copy link
Member

cameel commented Apr 10, 2025

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
Copy link
Member

cameel commented Apr 10, 2025

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 (#12829 (comment)) 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 cameel added medium effort Default level of effort medium impact Default level of impact must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. labels Apr 10, 2025
@frangio
Copy link
Contributor Author

frangio commented Apr 10, 2025

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.

@cameel
Copy link
Member

cameel commented Apr 10, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 medium effort Default level of effort medium impact Default level of impact must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it.
Projects
None yet
Development

No branches or pull requests

3 participants