Skip to content

Conversation

@ep0chzer0
Copy link
Contributor

Summary

Refines the inheritance-graph printer to only exclude virtual functions that are not overridden, instead of excluding all virtual functions.

Problem

The current logic excludes ALL virtual functions from the inheritance graph:

and not f.is_virtual

This is overly aggressive - virtual functions that ARE overridden by derived contracts represent actual implementations being used and should be shown.

Solution

Changed the condition to only exclude virtual functions that have no overrides:

and not (f.is_virtual and not f.overridden_by)

This means:

  • Virtual functions with overrides → included (they're being used)
  • Virtual functions without overrides → excluded (just interface, no implementation)
  • Non-virtual functions → included (unchanged behavior)

Example

abstract contract Base {
    function foo() public virtual;  // No override → excluded
    function bar() public virtual;  // Has override → included
}

contract Derived is Base {
    function bar() public override { }  // Overrides Base.bar
}

Before: Both foo and bar excluded from Base node
After: Only foo excluded, bar is shown

Partially addresses #2150

@ep0chzer0 ep0chzer0 requested a review from smonicas as a code owner January 16, 2026 12:40
@ep0chzer0 ep0chzer0 force-pushed the fix/inheritance-graph-virtual-functions branch 2 times, most recently from a7bc8a0 to 178e07c Compare January 16, 2026 23:28
Copy link
Member

@dguido dguido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Reviewed file: slither/printers/inheritance/inheritance_graph.py

Change Analysis

The change modifies the filter condition for public/external functions in the inheritance graph:

-            and not f.is_virtual
+            and not (f.is_virtual and not f.overridden_by)  # Exclude unoverridden virtual functions

Before: All virtual functions were excluded from the inheritance graph output.

After: Only virtual functions that have no overrides are excluded. Virtual functions that ARE overridden by derived contracts are now shown.

Verification

  1. Logic correctness: The new condition (f.is_virtual and not f.overridden_by) correctly identifies virtual functions without any derived implementations. The overridden_by property returns an empty list [] when no overrides exist, and empty lists are falsy in Python, so not f.overridden_by works correctly.

  2. API usage: The overridden_by attribute is properly typed as list[FunctionContract] and always initialized to [] in slither/core/declarations/function.py:128, so it will never be None.

  3. Tests pass: Ran test_inheritance_printer and test_virtual_overrides - all pass.

  4. Linting: ruff check passes with no issues.

  5. Comment quality: The inline comment is appropriate and explains the filtering behavior.

Observations

  • The private_functions filter (lines 130-137) does not and never did filter by is_virtual, so this change doesn't introduce an inconsistency.

  • This addresses part of issue #2150 by making the inheritance graph output more complete while still excluding pure interface methods (virtual functions with no implementation).

Approved. Clean, minimal change that improves the inheritance graph output.

@dguido
Copy link
Member

dguido commented Jan 20, 2026

Bulk PR Review Summary

Research Findings

  • Change: One-line modification in slither/printers/inheritance/inheritance_graph.py
  • Before: and not f.is_virtual (excludes ALL virtual functions)
  • After: and not (f.is_virtual and not f.overridden_by) (excludes only virtual functions without overrides)
  • Addresses: Issue ignore interfaces for inheritance-graph printer #2150 comment requesting removal of "virtual functions which aren't overridden"

Test Status

  • Rebased on current master (26e5224)
  • Unit tests: 198 passed (same as master)
  • E2E printer tests: Skipped due to pre-existing solc-select environment issues (same failures on master)
  • Ruff lint: Passes

Code Review (Pedantic Level)

  1. Logic verification: Correct. overridden_by is initialized as [] (line 128 in function.py), so not f.overridden_by is True when no overrides exist.
  2. Comment quality: Good inline comment explains the filtering behavior
  3. CLAUDE.md compliance: Single-line change, no additional requirements triggered
  4. Edge cases: None identified - the overridden_by property is always a list, never None

Merge Readiness

Ready to merge. Clean, minimal change that improves inheritance graph output by showing virtual functions that have implementations while still hiding pure interface methods. No regressions introduced.

@ep0chzer0 ep0chzer0 force-pushed the fix/inheritance-graph-virtual-functions branch from 178e07c to 55b98bf Compare January 20, 2026 09:46
Previously, all virtual functions were excluded from the inheritance-graph
printer output. This change refines the logic to only exclude virtual
functions that are not overridden by any derived contract.

Virtual functions that ARE overridden are now included, as they represent
actual implementations being used in the codebase.

Partially addresses crytic#2150
@ep0chzer0 ep0chzer0 force-pushed the fix/inheritance-graph-virtual-functions branch from 55b98bf to 955bac5 Compare January 20, 2026 16:45
@dguido
Copy link
Member

dguido commented Jan 20, 2026

Code review

Found 1 issue:

  1. Comment repeats what the code does (CLAUDE.md says: "Code should be self-documenting. No commented-out code (delete it). No comments that repeat what code does.")

and not f.is_constructor_variables
and not (f.is_virtual and not f.overridden_by) # Exclude unoverridden virtual functions
and f.contract_declarer == contract

The comment # Exclude unoverridden virtual functions directly restates the boolean logic not (f.is_virtual and not f.overridden_by). The property names is_virtual and overridden_by already make the intent clear.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

The comment "# Exclude unoverridden virtual functions" directly restates
what the boolean expression already expresses through property names.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@dguido dguido merged commit 12fdde0 into crytic:master Jan 20, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants