Be sure to check trait-impl methods against the trait method's spec#188
Merged
Conversation
coord-e
marked this pull request as ready for review
July 23, 2026 11:25
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an unsoundness where unannotated trait-impl methods could be refined/checked before the corresponding trait method’s extern spec was registered, allowing implementations that contradict the trait spec (e.g., PartialEq::eq negating structural equality) to be accepted as safe.
Changes:
- Refactors
Analyzer::refine_local_defsto pre-scan MIR keys, avoid order-dependent double-registration forextern_spec_fntargets, and refine trait-method extern specs first. - Adds UI regression tests that ensure a
PartialEqimpl contradicting the blanket spec is rejected, while a consistent impl still verifies.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/analyze/crate_.rs |
Pre-scans MIR keys to prioritize trait-method spec registration and avoid extern-spec target double-registration/order dependence. |
tests/ui/pass/partialeq_impl.rs |
Positive regression test: PartialEq impl consistent with structural equality continues to verify. |
tests/ui/fail/partialeq_impl.rs |
Negative regression test: PartialEq impl that negates structural equality is now rejected (Unsat). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
67
to
+69
| for local_def_id in self.tcx.mir_keys(()) { | ||
| if self.tcx.def_kind(*local_def_id).is_fn_like() { | ||
| self.refine_fn_def(*local_def_id); | ||
| let analyzer = self.ctx.local_def_analyzer(*local_def_id); | ||
| if analyzer.is_annotated_as_extern_spec_fn() { |
Owner
Author
There was a problem hiding this comment.
local_def_analyzer の構築は重くないということにしています
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.
Summary
A hand-written trait-method impl was never checked against the trait method's (extern) spec, so an implementation that contradicts the spec was accepted as safe. For example, this program verified even though the assertion can fail:
x == xlowers to<A as PartialEq>::eq, which Thrust models with the blanket extern specresult == (*x == *y)(structural equality). The impl above returns the negation, sox == xis reallyfalseand the assertion can fail — yet Thrust reported the program safe.Root cause
A trait-impl method without its own annotation is meant to be checked against the trait method's spec, which
expected_tyobtains viatrait_item_ty→def_ty_with_argson the trait method def. That lookup returnsNoneunless the trait method's (extern) spec is already registered. Becauserefine_local_defswalkedmir_keysin a single pass, a userimpl PartialEq for A(lowDefId) was refined before the blanketPartialEq::eqextern spec injected bystd.rs(highDefId), so the impl fell back to an unconstrained template and its body was never checked against the spec.The pass also double-registered keys in an order-dependent way: a trait method and its extern-spec companion both register the same
defsentry, and correctness relied on the companion being inserted last.Changes
register_defof the target of anextern_spec_fn. The plain target is dropped from the refine set so only the extern spec registers that key, removing the order-dependent double-registration. Theignored/predicate/formula_fnclassification is done in the same pre-scan, so a trusted/ignored target still lands inskip_analysisand its body is not analyzed.AssocFn) extern specs before ordinary defs, so a trait method's spec is registered before trait-impl methods consult it viatrait_item_ty, independent ofmir_keysiteration order.Tests
tests/ui/fail/partialeq_impl.rs— aPartialEqimpl that negates structural equality is now rejected (Unsat).tests/ui/pass/partialeq_impl.rs— aPartialEqimpl consistent with structural equality still verifies.The full
uisuite passes.Generated by Claude Code