Fix naming-convention false positive on Yul-local function parameters#3038
Open
MarkLee131 wants to merge 2 commits into
Open
Fix naming-convention false positive on Yul-local function parameters#3038MarkLee131 wants to merge 2 commits into
MarkLee131 wants to merge 2 commits into
Conversation
The Yul parser mangles every local identifier to <source_name>__<scope_chain> (solc_parsing/yul/parse_yul.py:130), so a Yul parameter `base_` declared inside `unsafeExtend`'s 0th assembly block ends up named `base__unsafeExtend_asm_0_extendInline`. The naming-convention detector iterates `contract.functions_declared`, which includes these synthetic Yul- local functions, and runs `is_mixed_case_with_underscore` on the mangled name. The mangled name is a parser artifact, not an identifier the developer wrote. Synthetic Yul-local functions carry `Function.internal_scope` non-empty. Skip them in the naming-convention loop so neither the synthetic function name nor its parser-mangled parameters are checked. Adds tests/e2e/detectors/test_data/naming-convention/0.8.20/naming_convention_yul_local.sol covering two Yul-local function declarations (extendInline, memcpy) plus a tripwire `tripwire(uint256 Bad_Param)` that uses a deliberately non-mixedCase Solidity-level parameter. The snapshot after this change contains exactly the tripwire finding; the Yul-local FPs are gone. All 8 existing NamingConvention fixtures still pass with identical output. Closes crytic#1815.
Generated via: python tests/e2e/detectors/test_detectors.py --compile pytest -k NamingConvention-0.8.20-naming_convention_yul_local --insta update All 9 NamingConvention test cases pass with the fix applied.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3037. Also addresses #1815 (open since 2023).
naming-conventionwas flagging parameters of functions defined inside inline assembly with their parser-mangled internal name, e.g.Parameter LibArr.unsafeExtend.asm_0.extendInline().base__unsafeExtend_asm_0_extendInline is not in mixedCasefor a source-levelbase_.Root Cause
The Yul parser mangles every local identifier to
<source_name>__<scope_chain>(solc_parsing/yul/parse_yul.py:130), so a Yul parameterbase_declared insideunsafeExtend's 0th assembly block ends up namedbase__unsafeExtend_asm_0_extendInline. The detector iteratescontract.functions_declared, which includes synthetic Yul-local functions, and then runsis_mixed_case_with_underscoreon the mangled name.The mangled name is a parser artifact, not an identifier the developer wrote. Slither already tracks this distinction: synthetic Yul functions carry
Function.internal_scopenon-empty (set when the parser binds them under their containingYulBlock).Fix
Skip Yul-local functions in the naming-convention loop:
Tests
Added
tests/e2e/detectors/test_data/naming-convention/0.8.20/naming_convention_yul_local.sol. Covers two Yul-local function declarations (extendInline,memcpy) plus a tripwiretripwire(uint256 Bad_Param)that uses a deliberately non-mixedCase Solidity-level parameter. After the fix the snapshot contains exactly the tripwire finding; the Yul-local FPs are gone.All 8 existing NamingConvention fixtures (0.4.25 / 0.5.16 / 0.6.11 / 0.7.6 ×
naming_convention.solandno_warning_for_public_constants.sol) still pass with identical output.