Allow assignment of immutable variables (#838)

OpenZeppelin · Jul 10, 2023 · ceeafbb · ceeafbb
1 parent 44d0149
commit ceeafbb
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 15 deletions.
diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc
@@ -87,9 +87,12 @@ In some cases you may want to allow multiple errors in a single line.
 
 [source,solidity]
 ----
+/// @custom:oz-upgrades-unsafe-allow constructor state-variable-immutable
 contract SomeOtherContract {
-  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
-  uint256 immutable x = 1;
+  uint256 immutable x;
+  constructor() {
+    x = block.number;
+  }
 }
 ----
 
@@ -108,7 +111,7 @@ It may be possible to safely use `delegatecall` and `selfdestruct` if they are g
 [source,solidity]
 ----
 abstract contract OnlyDelegateCall {
-    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     address private immutable self = address(this);
 
     function checkDelegateCall() private view {

diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- Allow assignment of immutable variables if the `state-variable-immutable` override is present.
+
 ## 1.27.1 (2023-06-15)
 
 - Update recommended Foundry config for CLI. ([#818](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/818))

diff --git a/packages/core/contracts/test/ValidationsNatspec.sol b/packages/core/contracts/test/ValidationsNatspec.sol
@@ -47,18 +47,18 @@ contract HasStateVariableAssignmentNatspec3 {
   uint y = 2;
 }
 
-/// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
 contract HasImmutableStateVariableNatspec1 {
   uint immutable x = 1;
 }
 
 contract HasImmutableStateVariableNatspec2 {
-  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
   uint immutable x = 1;
 }
 
 contract HasImmutableStateVariableNatspec3 {
-  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
   uint immutable x = 1;
   uint immutable y = 2;
 }

diff --git a/packages/core/src/validate/report.ts b/packages/core/src/validate/report.ts
@@ -35,8 +35,10 @@ const errorInfo: ErrorDescriptions<ValidationError> = {
     link: 'https://zpl.in/upgrades/error-004',
   },
   'state-variable-immutable': {
-    msg: e => `Variable \`${e.name}\` is immutable`,
-    hint: () => `Use a constant or mutable variable instead`,
+    msg: e => `Variable \`${e.name}\` is immutable and will be initialized on the implementation`,
+    hint: () =>
+      `If by design, annotate with '@custom:oz-upgrades-unsafe-allow state-variable-immutable'\n` +
+      `Otherwise, consider a constant variable or use a mutable variable instead`,
     link: 'https://zpl.in/upgrades/error-005',
   },
   'external-library-linking': {

diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts
@@ -386,19 +386,19 @@ function* getStateVariableErrors(
 ): Generator<ValidationErrorWithName> {
   for (const varDecl of contractDef.nodes) {
     if (isNodeType('VariableDeclaration', varDecl)) {
-      if (!varDecl.constant && !isNullish(varDecl.value)) {
-        if (!skipCheck('state-variable-assignment', contractDef) && !skipCheck('state-variable-assignment', varDecl)) {
+      if (varDecl.mutability === 'immutable') {
+        if (!skipCheck('state-variable-immutable', contractDef) && !skipCheck('state-variable-immutable', varDecl)) {
           yield {
-            kind: 'state-variable-assignment',
+            kind: 'state-variable-immutable',
             name: varDecl.name,
             src: decodeSrc(varDecl),
           };
         }
-      }
-      if (varDecl.mutability === 'immutable') {
-        if (!skipCheck('state-variable-immutable', contractDef) && !skipCheck('state-variable-immutable', varDecl)) {
+      } else if (!varDecl.constant && !isNullish(varDecl.value)) {
+        // Assignments are only a concern for non-immutable variables
+        if (!skipCheck('state-variable-assignment', contractDef) && !skipCheck('state-variable-assignment', varDecl)) {
           yield {
-            kind: 'state-variable-immutable',
+            kind: 'state-variable-assignment',
             name: varDecl.name,
             src: decodeSrc(varDecl),
           };