feat: add C3 linearization printer for inheritance analysis#2917
feat: add C3 linearization printer for inheritance analysis#2917dguido merged 5 commits intocrytic:masterfrom
Conversation
e6c6dd1 to
aae6a83
Compare
dguido
left a comment
There was a problem hiding this comment.
Code Review: C3 Linearization Printer
Reviewing: PR #2917 - feat: add C3 linearization printer for inheritance analysis
Summary
This PR adds a useful C3 linearization printer that addresses the core requirements of issue #2784. The implementation is clean and follows existing printer patterns well. A few issues worth addressing:
Critical Issues (Confidence: 90+)
1. WIKI URL uses trailofbits instead of crytic (Confidence: 92)
File: slither/printers/inheritance/c3_linearization.py, line 17
The codebase is migrating WIKI URLs from trailofbits/slither to crytic/slither. The existing inheritance.py uses trailofbits, but the newer constructor_calls.py uses crytic. For consistency with the current repository ownership, consider:
# Current:
WIKI = "https://github.com/trailofbits/slither/wiki/Printer-documentation#c3-linearization"
# Suggested:
WIKI = "https://github.com/crytic/slither/wiki/Printer-documentation#c3-linearization"2. Potential IndexError when source_mapping.lines is empty (Confidence: 85)
File: slither/printers/inheritance/c3_linearization.py, line 56
The code accesses c.source_mapping.lines[0] without checking if lines is non-empty. While rare, lines can be an empty list:
# Current:
if c.source_mapping:
source_loc = f" ({c.source_mapping.filename.short}:{c.source_mapping.lines[0]})"
# Suggested:
if c.source_mapping and c.source_mapping.lines:
source_loc = f" ({c.source_mapping.filename.short}:{c.source_mapping.lines[0]})"Important Issues (Confidence: 80-89)
3. Lambda used for identity function (Confidence: 82)
File: slither/printers/inheritance/c3_linearization.py, line 52
Using a lambda for an identity function is functional but could be cleaner:
# Current:
color_fn = lambda x: x # no color for inherited
# Suggested (avoid lambda assignment per PEP 8):
def identity(x):
return x
color_fn = identity
# Or simply inline the logic to avoid the function callSuggestions (Not blocking)
-
Consider adding
start_lineto JSON output - The JSON includessourcefilename but not the line number, which could be useful for tooling. -
Test coverage - The test verifies the diamond pattern well. Consider adding a test case for contracts without constructors to ensure the empty constructor handling works correctly.
-
Issue #2784 scope - The issue requested several advanced features (verbose mode, conflict detection, super resolution). This PR implements the core functionality, which is a reasonable first step. Those could be follow-up enhancements.
Positive Observations
- Clean implementation following existing printer patterns
- Good use of
contract.is_interfaceto skip interfaces (per CLAUDE.md guidance) - JSON output structure is well-designed
- Test case covers the classic diamond inheritance pattern
- Constructor execution order display is a nice touch
Overall, this is a solid contribution. The two issues above (WIKI URL and potential IndexError) should be addressed before merging.
PR Review SummaryResearch FindingsPR Purpose: This PR adds a new C3 linearization printer (c3-linearization) that displays the method resolution order (MRO) for contract inheritance, addressing issue #2784. The printer shows:
The implementation correctly leverages contract.inheritance which already contains the C3 linearized order in Slither. Test StatusAll printer tests pass after merging master:
The failing unit tests (Vyper-related) are pre-existing and unrelated to this PR - they fail due to missing Vyper installation in the test environment. Code Review Issues Found and FixedAll issues from the initial review by @dguido have been addressed:
Merge ReadinessReady to merge after the author applies these changes. The implementation is solid and follows existing printer patterns well. The test coverage is appropriate for a new printer. The suggested fixes have been prepared and tested locally - I can provide a patch or diff if helpful. |
aae6a83 to
59e7521
Compare
Add a new printer that displays the C3 linearization order for contract inheritance. This helps developers understand the method resolution order (MRO), constructor execution order, and super call resolution. Features: - Shows linearization order with contract type labels (SELF/BASE/INHERITED) - Displays source file locations - Shows constructor presence for each contract - Lists constructor execution order (reverse of linearization) - Outputs both human-readable format and structured JSON Fixes crytic#2784
- Update WIKI URL from trailofbits to crytic - Check source_mapping.lines is non-empty before accessing [0] - Replace lambda with named function for identity (PEP 8 E731) Addresses review feedback from @dguido
7a755fe to
5ccb6e9
Compare
- Add defensive null check for `filename` before accessing `.short` - Extract identity function outside loop for cleaner code Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Fixes #2784
Adds a new printer that displays the C3 linearization order for contract inheritance, helping developers understand:
superrefers toFeatures
Example Output
Usage
slither . --print c3-linearizationTest Plan