Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Sep 24, 2025

PR Type

Enhancement, Tests


Description

  • Add attrs-based object comparison

  • Support superset mode for attrs

  • Comprehensive attrs comparator tests

  • Preserve non-eq field semantics


Diagram Walkthrough

flowchart LR
  comparator["comparator: recursive equality utility"] -- "add support" --> attrsCheck["attrs objects handling"]
  attrsCheck -- "build dict of eq fields" --> compareDicts["delegate to comparator on dicts"]
  attrsCheck -- "superset mode" --> supersetCompare["subset compare vs new attrs"]
  tests["tests/test_comparator.py"] -- "add cases" --> attrsScenarios["attrs classes and edge cases"]
Loading

File Walkthrough

Relevant files
Enhancement
comparator.py
Implement attrs-aware comparison in comparator                     

codeflash/verification/comparator.py

  • Detect attrs availability flag.
  • Add comparison for objects with __attrs_attrs__.
  • Compare only attributes marked with eq=True.
  • Support superset mode by subset comparison against new attrs.
+28/-0   
Tests
test_comparator.py
Add comprehensive tests for attrs comparison                         

tests/test_comparator.py

  • Add tests for various attrs class patterns.
  • Verify eq/non-eq fields handling.
  • Test nested attrs and containers.
  • Validate mismatch and superset-like scenarios.
+144/-1 

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Robustness

The attrs handling relies on checking for 'attrs_attrs' rather than using 'attrs.has' when 'attrs' is available. This may miss edge cases or custom classes that mimic the attribute or future API changes. Consider guarding with 'HAS_ATTRS and attrs.has(obj)' for both objects before proceeding.

if hasattr(orig, "__attrs_attrs__") and hasattr(new, "__attrs_attrs__"):
    orig_dict = {}
    new_dict = {}

    for attr in orig.__attrs_attrs__:
        if attr.eq:
            attr_name = attr.name
            orig_dict[attr_name] = getattr(orig, attr_name, None)
            new_dict[attr_name] = getattr(new, attr_name, None)

    if superset_obj:
        new_attrs_dict = {}
        for attr in new.__attrs_attrs__:
            if attr.eq:
                attr_name = attr.name
                new_attrs_dict[attr_name] = getattr(new, attr_name, None)
        return all(
            k in new_attrs_dict and comparator(v, new_attrs_dict[k], superset_obj) for k, v in orig_dict.items()
        )
    return comparator(orig_dict, new_dict, superset_obj)
Superset Logic

In superset mode, the code rebuilds 'new_attrs_dict' but then compares only orig eq-fields against it. This ignores type/class differences and non-eq fields by design, but also bypasses checking that both objects are of compatible attrs classes. Confirm this is intended; otherwise add a class/type check before subset comparison.

if superset_obj:
    new_attrs_dict = {}
    for attr in new.__attrs_attrs__:
        if attr.eq:
            attr_name = attr.name
            new_attrs_dict[attr_name] = getattr(new, attr_name, None)
    return all(
        k in new_attrs_dict and comparator(v, new_attrs_dict[k], superset_obj) for k, v in orig_dict.items()
    )
return comparator(orig_dict, new_dict, superset_obj)
Redundant Class Names

Two 'ExtendedClass' definitions exist in the same test function, which can be confusing and may shadow the earlier definition. Consider renaming one to avoid ambiguity.

@attrs.define
class ExtendedClass:
    name: str
    value: int
    extra_field: str = "default"
    metadata: dict = attrs.field(factory=dict)
    timestamp: float = attrs.field(eq=False, default=0.0)  # This should be ignored

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Build dicts from respective attrs

Avoid populating new_dict using orig.attrs_attrs, which can miss fields on new
or misalign attributes when classes differ. Build new_dict by iterating
new.attrs_attrs (filtered by eq) to reflect its actual comparable fields. This
prevents false positives/negatives when attribute sets diverge.

codeflash/verification/comparator.py [245-265]

 if hasattr(orig, "__attrs_attrs__") and hasattr(new, "__attrs_attrs__"):
     orig_dict = {}
     new_dict = {}
 
     for attr in orig.__attrs_attrs__:
         if attr.eq:
             attr_name = attr.name
             orig_dict[attr_name] = getattr(orig, attr_name, None)
+
+    for attr in new.__attrs_attrs__:
+        if attr.eq:
+            attr_name = attr.name
             new_dict[attr_name] = getattr(new, attr_name, None)
 
     if superset_obj:
-        new_attrs_dict = {}
-        for attr in new.__attrs_attrs__:
-            if attr.eq:
-                attr_name = attr.name
-                new_attrs_dict[attr_name] = getattr(new, attr_name, None)
         return all(
-            k in new_attrs_dict and comparator(v, new_attrs_dict[k], superset_obj) for k, v in orig_dict.items()
+            k in new_dict and comparator(v, new_dict[k], superset_obj) for k, v in orig_dict.items()
         )
     return comparator(orig_dict, new_dict, superset_obj)
Suggestion importance[1-10]: 7

__

Why: Correctly notes that new_dict is populated using orig.__attrs_attrs__, which can misrepresent new's comparable fields; proposing to iterate new.__attrs_attrs__ fixes potential mismatches and aligns with the superset handling logic.

Medium
General
Fail on missing comparable fields

Guard against attribute access errors when comparing different attrs classes. If new
lacks an attribute present on orig, accessing it with getattr(new, attr_name, None)
masks missing fields as None, leading to silent equality. Explicitly check for
attribute presence and treat missing comparable fields as inequality.

codeflash/verification/comparator.py [245-265]

 if hasattr(orig, "__attrs_attrs__") and hasattr(new, "__attrs_attrs__"):
     orig_dict = {}
     new_dict = {}
 
     for attr in orig.__attrs_attrs__:
         if attr.eq:
             attr_name = attr.name
-            orig_dict[attr_name] = getattr(orig, attr_name, None)
-            new_dict[attr_name] = getattr(new, attr_name, None)
+            if not hasattr(orig, attr_name):
+                return False
+            orig_dict[attr_name] = getattr(orig, attr_name)
 
+    for attr in new.__attrs_attrs__:
+        if attr.eq:
+            attr_name = attr.name
+            if not hasattr(new, attr_name):
+                return False
+            new_dict[attr_name] = getattr(new, attr_name)
+
+    if superset_obj:
+        return all(
+            (k in new_dict) and comparator(v, new_dict[k], superset_obj) for k, v in orig_dict.items()
+        )
+    return comparator(orig_dict, new_dict, superset_obj)
+
Suggestion importance[1-10]: 5

__

Why: The idea to avoid masking missing comparable fields with None is reasonable and may prevent silent equality, but it adds early returns and presence checks that are partly redundant with building dicts per-class; impact is moderate.

Low
Security
Validate genuine attrs metadata

Ensure both objects are instances of attrs classes before using attrs_attrs, not
just have the attribute by coincidence. Use getattr(obj, "attrs_attrs", None)
and verify it is a sequence to avoid attribute spoofing leading to incorrect
comparison paths.

codeflash/verification/comparator.py [245-265]

-if hasattr(orig, "__attrs_attrs__") and hasattr(new, "__attrs_attrs__"):
-    ...
+attrs_orig = getattr(orig, "__attrs_attrs__", None)
+attrs_new = getattr(new, "__attrs_attrs__", None)
+if attrs_orig and attrs_new and isinstance(attrs_orig, (list, tuple)) and isinstance(attrs_new, (list, tuple)):
+    orig_dict = {}
+    new_dict = {}
+
+    for attr in attrs_orig:
+        if getattr(attr, "eq", False):
+            attr_name = attr.name
+            orig_dict[attr_name] = getattr(orig, attr_name)
+
+    for attr in attrs_new:
+        if getattr(attr, "eq", False):
+            attr_name = attr.name
+            new_dict[attr_name] = getattr(new, attr_name)
+
     if superset_obj:
-        new_attrs_dict = {}
-        for attr in new.__attrs_attrs__:
-            if attr.eq:
-                attr_name = attr.name
-                new_attrs_dict[attr_name] = getattr(new, attr_name, None)
         return all(
-            k in new_attrs_dict and comparator(v, new_attrs_dict[k], superset_obj) for k, v in orig_dict.items()
+            (k in new_dict) and comparator(v, new_dict[k], superset_obj) for k, v in orig_dict.items()
         )
     return comparator(orig_dict, new_dict, superset_obj)
Suggestion importance[1-10]: 4

__

Why: Verifying __attrs_attrs__ shape could harden against spoofing, but the PR already checks via attribute presence and this scenario is unlikely; added complexity yields marginal benefit here.

Low

@KRRT7 KRRT7 requested a review from misrasaurabh1 September 24, 2025 08:07
@KRRT7 KRRT7 enabled auto-merge September 24, 2025 08:07
@KRRT7 KRRT7 requested review from aseembits93 and removed request for misrasaurabh1 September 24, 2025 08:07
@KRRT7 KRRT7 merged commit 5dffd2a into main Sep 24, 2025
18 of 23 checks passed
@KRRT7 KRRT7 deleted the attrs-support branch September 24, 2025 22:48
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.

2 participants