Skip to content

Fix memoized getter state after exceptions - #6803

Open
kleonyuan wants to merge 1 commit into
OpenZeppelin:masterfrom
kleonyuan:master
Open

kleonyuan wants to merge 1 commit into
OpenZeppelin:masterfrom
kleonyuan:master

Conversation

@kleonyuan

Copy link
Copy Markdown

Fixes #6802

Reset the memoized getter state to todo when the getter throws, then rethrow the original error. Later property reads can retry the getter, while recursive access during an active getter call still throws Detected recursion.

Add regression coverage for retries after exceptions and genuine recursive access.

PR Checklist

  • Tests (added; not run locally)
  • Documentation (not applicable)
  • Changeset entry

@kleonyuan
kleonyuan requested a review from a team as a code owner October 6, 2026 01:00
@changeset-bot

changeset-bot Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 9126e38

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4aa9fae6-1fc8-4007-aead-b51a11eb4528
📥 Commits

Reviewing files that changed from the base of the PR and between 40a51f7 and 8546f06.

📒 Files selected for processing (3)
  • .changeset/quiet-rivers-retry.md
  • hardhat/hardhat-solidity-docgen/internal/utils/memoized-getter.ts
  • test/hardhat/MemoizedGetter.test.js

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

When the memoized getter throws, it resets its state to 'todo' and rethrows the original error. Tests verify that later accesses retry the getter and that recursive access still reports Detected recursion. A patch changeset documents the retry behavior.

Priority: ➖ Normal

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 8546f

No merge-blocking issue is identified. Run the added tests as part of normal validation before merging.

Architecture Summary

Architecture risk: 🔵 Low · up to 8546f

The change affects 2 systems.

Changed systems: hardhat, test

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — hardhat (service) was modified; 1 changed file maps to changed impact.
  • observed — test (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in hardhat/hardhat-solidity-docgen/internal/utils/memoized-getter.ts: The getter now catches errors from getter(), resets the memoization state to 'todo', and rethrows the error. Previously, a thrown error left the state as 'doing'.
  • observed — Modified behavior in test/hardhat/MemoizedGetter.test.js: Adds Chai and defineGetterMemoized imports for the tests.
  • observed — Modified behavior in test/hardhat/MemoizedGetter.test.js: Adds tests expecting repeated access to a throwing getter to throw boom twice and increment its call count twice, and recursive access to throw an error containing Detected recursion.
  • observed — Modified behavior in .changeset/quiet-rivers-retry.md: Adds a patch changeset for openzeppelin-solidity with a release note describing docgen getter retry behavior after exceptions and preservation of the original error.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: resetting memoized getter state after exceptions.
Description check ✅ Passed The description explains the state reset, retry behavior, recursion handling, regression tests, and changeset entry. It is directly related to the changeset.
Linked Issues check ✅ Passed Issue #6802 requires a later read to retry a memoized getter after it throws and propagate the getter's error. The change in hardhat/hardhat-solidity-docgen/internal/utils/memoized-getter.ts resets …
Out of Scope Changes check ✅ Passed The memoized-getter implementation change, regression tests, and .changeset/quiet-rivers-retry.md all support the fix requested by issue #6802. The available change summary shows no unrelated change…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Comment thread test/hardhat/MemoizedGetter.test.js Resolved
Signed-off-by: kleonyuan <kleonyuan@outlook.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reset memoized getter state after exceptions

2 participants