Skip to content

Fix dict view comparisons and avoid temporary sets - #8849

Merged
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:dict_view_set_ops
Sep 27, 2026
Merged

Fix dict view comparisons and avoid temporary sets#8849
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:dict_view_set_ops

Conversation

@1ndahous3

@1ndahous3 1ndahous3 commented Sep 27, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Fix comparisons of dictionary keys/items views with frozenset, and use set semantics for comparisons between keys and items views.
  • Allow dict_items.isdisjoint() to handle unhashable values and stop at the first match.
  • Avoid temporary sets in view comparisons and isdisjoint(). Use one dictionary lookup for item membership.

Follow-up to #3437.

Performance

Release microbenchmarks on Windows, median of three runs with five timed samples each:

Operation Dict size Before After
Keys view == set 32 3.795 us 2.520 us
Keys view isdisjoint, one matching key 256 13.870 us 0.794 us
Keys view isdisjoint, 256 nonmatching keys 256 24.545 us 10.380 us
Items view isdisjoint, one matching pair 256 35.275 us 0.779 us

AI assistance

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

  • Bug Fixes
    • Dictionary key and item views now support set-style subset and superset comparisons with sets and other dictionary views.
    • Improved isdisjoint behavior across sets, views, and other iterables, including error handling and dictionaries that change during the check.
    • Item-view membership checks now handle unhashable values and avoid invoking dictionary subclass lookup hooks.
  • Changed Behavior
    • Dictionary views no longer support the removed set-operation overloads.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 2026

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 57ea5dee-5c4c-474e-ab02-cdf8d06d98f7

📥 Commits

Reviewing files that changed from the base of the PR and between e2ca208 and 3a8d4b6.

📒 Files selected for processing (2)
  • crates/vm/src/builtins/dict.rs
  • extra_tests/snippets/builtin_dict.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

Dictionary-view comparisons and isdisjoint now handle sets and dictionary views directly. Dictionary-item membership retrieves the value in one lookup. Added tests cover comparisons, disjointness, iterable behavior, and lookup hooks.

Changes

Dictionary view operations

Layer / File(s) Summary
Set-style comparisons and disjointness
crates/vm/src/builtins/dict.rs, extra_tests/snippets/builtin_dict.py		 Comparisons handle recognized set and dictionary-view operands. isdisjoint checks identity and iterates one side of the comparison. Tests cover operand types, ordering, iterator errors, and set subclasses.
Single-lookup item membership
crates/vm/src/builtins/dict.rs, extra_tests/snippets/builtin_dict.py		 Item-view membership retrieves the value with inner_getitem_opt before comparing it. Tests check lookup count and confirm that dictionary subclass lookup hooks are not called.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to 3a8d4

No confirmed issue blocks merging after normal validation.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 3a8d4

A set subclass can report a false length and cause a dictionary keys view to compare equal to a set with different contents. This could undermine code that uses equality to check an exact set of permitted keys. The exposure depends on an application accepting such an operand; it is not a new privilege granted by the dictionary view itself.

Retained concerns

  • Medium · security · inferred: Dictionary-view equality can trust a set subclass's reported length rather than its actual element count, allowing unequal collections to compare equal.
Security review details

Security Blast Radius

  • inferred — The equality issue is reachable through the VM's public dictionary-view comparison wherever a caller supplies a set subclass. A security consequence requires another component to rely on that comparison as an exact-key check; no such production caller was established.

Security Findings and Attack Paths

  • inferred — An operand subclass containing an allowed key and an extra key can report the view's length. Equality then checks that the view's key is present and can return true without checking the extra element. The prior internal set-size check would return false.

Trust Boundaries and Controls

  • inferred — The changed length gate moves a decision about equality from internally held set sizes to an operand-controlled method result. Operand-type checks limit the path to sets and dictionary views, but do not exclude set subclasses.

Hardening Proposals

  • proposed — Base equality and ordering length checks on internally held sizes for recognized set and view operands, or otherwise verify both directions of membership before accepting equality. Add a comparison case with a set subclass whose reported length differs from its contents.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main changes: fixing dictionary view comparisons and removing temporary sets.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

youknowone
youknowone approved these changes Sep 27, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@youknowone
youknowone merged commit 90afe00 into RustPython:main Sep 27, 2026
21 checks passed
@1ndahous3
1ndahous3 deleted the dict_view_set_ops branch September 27, 2026 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@1ndahous3 @youknowone