fix: resolve isoltest crash on interactive update#16438
fix: resolve isoltest crash on interactive update#16438JPier34 wants to merge 2 commits intoargotorg:developfrom
Conversation
When using isoltest's interactive update feature (pressing 'u' to accept new expectations), the tool would crash with "Contract '' not found" error during the re-run of the updated test. Root cause: In lastContractName(), when the specified source name doesn't match any key in m_sources (e.g., due to source name mismatch between mainSourceFile and actual source keys), the function returned an empty string, causing the "Contract '' not found" error. Changes: - CompilerStack.cpp: Add fallback search across all sources when the specified source is not found in m_sources - isoltest.cpp: Add explicit flush() and close() after writing updated test file to ensure complete write before re-read Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in isoltest when using the interactive update feature (pressing 'u' to accept updated expectations). The crash occurred when CompilerStack::lastContractName() couldn't find a contract because the mainSourceFile name didn't match the actual keys in the m_sources map, particularly in multi-source test files.
Changes:
- Added fallback logic in
CompilerStack::lastContractName()to search all sources when the specified source is not found - Added explicit
flush()andclose()calls after writing updated test files inisoltestto prevent potential I/O race conditions - Added a basic regression test case
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libsolidity/interface/CompilerStack.cpp | Added fallback search logic and null check for AST in lastContractName() to handle source name mismatches |
| test/tools/isoltest.cpp | Added explicit flush and close operations to ensure test file is fully written before rerun |
| test/libsolidity/semanticTests/test_issue_16337.sol | Added minimal test case (though it doesn't reproduce the multi-source scenario from the issue) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| break; | ||
| } | ||
| if (!contractName.empty()) | ||
| break; |
There was a problem hiding this comment.
The fallback logic returns the first contract found across all sources, which is inconsistent with the function name lastContractName and its original behavior. When the specified source is not found, the fallback should iterate through all contracts across all sources (like the original code when _sourceName is nullopt) to return the last contract, not break after finding the first one. Remove the break statements on lines 882 and 885 to maintain consistency with the original behavior.
| break; | |
| } | |
| if (!contractName.empty()) | |
| break; | |
| } |
There was a problem hiding this comment.
Agreed.
I will refactor lastContractName() so it first tries the requested source when available, and falls back to scanning all sources when the requested source is missing or has no contracts.
This keeps normal behavior unchanged while preventing the interactive-update crash path.
| contract TestContract { | ||
| function getValue() public pure returns (uint256) { | ||
| return 42; | ||
| } | ||
| } |
There was a problem hiding this comment.
The test case doesn't reproduce the actual issue described in #16337. The issue occurs with multi-source files where the mainSourceFile name doesn't match the key in m_sources (e.g., module_function_deployed.sol with mod.sol and main.sol). This single-file test won't trigger the fallback logic path that was added to fix the crash. Consider adding a test similar to module_function_deployed.sol structure to verify the fix works correctly.
| contract TestContract { | |
| function getValue() public pure returns (uint256) { | |
| return 42; | |
| } | |
| } | |
| // ==== Source: mod.sol ==== | |
| library Mod { | |
| function value() internal pure returns (uint256) { | |
| return 42; | |
| } | |
| } | |
| // ==== Source: main.sol ==== | |
| import "mod.sol"; | |
| contract TestContract { | |
| function getValue() public pure returns (uint256) { | |
| return Mod.value(); | |
| } | |
| } |
There was a problem hiding this comment.
Thanks for the suggestion.
I will also update the test to be explicitly multi-source (==== Source: ... ==== with an import) so it now validates the same class of scenario behind issue #16337, not just a single-file path.
| contract TestContract { | ||
| function getValue() public pure returns (uint256) { | ||
| return 42; | ||
| } | ||
| } | ||
| // ---- | ||
| // getValue() -> 0x2a |
There was a problem hiding this comment.
The test file is placed in the root of semanticTests directory, but all other semantic tests are organized into subdirectories. Since the issue being tested is related to multi-source compilation (as evident from the referenced module_function_deployed.sol), consider moving this test to the multiSource subdirectory for better organization and consistency with the existing test structure.
There was a problem hiding this comment.
Good point, agreed. Sorry for that.
I will move this regression test under semanticTests/multiSource/ to match the existing semantic test organization and keep the multi-source related coverage grouped consistently.
1 participant
Summary
Fixes a crash in
isoltestwhen using the interactive update feature (pressing 'u' to accept updated expectations).Fixes #16337
Problem
When
isoltestruns with a test file,CompilerStack::lastContractName()may fail to find the contract becausemainSourceFilecan differ from the actual key inm_sources. This mismatch causeslastContractName()to return an empty string, leading to a crash when the test framework attempts to use the contract name.Solution
Added a fallback search in
CompilerStack::lastContractName(): if the specified source is not found inm_sources, iterate over all sources to find a valid contract name. This ensures that even if there's a source name mismatch, the function will still return the correct contract name.Changes
lastContractName()to search all sources when specified source is not foundflush()andclose()after writing updated test files to prevent potential I/O race conditions