Add missing_fused_iterator pedantic lint - #17690
Conversation
|
Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews. In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews. |
|
Lintcheck changes for 5c030c0
This comment will be updated if you push new changes |
| if !matches!( | ||
| item.kind, | ||
| ItemKind::Struct(..) | ItemKind::Enum(..) | ItemKind::Union(..) | ||
| ) || !span_is_local(item.span) | ||
| || !cx.effective_visibilities.is_reachable(item.owner_id.def_id) | ||
| || is_lint_allowed(cx, MISSING_FUSED_ITERATOR, item.hir_id()) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| let Some(iterator_trait) = cx.tcx.lang_items().iterator_trait() else { | ||
| return; | ||
| }; | ||
| let Some(fused_iterator_trait) = cx.tcx.lang_items().fused_iterator_trait() else { | ||
| return; | ||
| }; | ||
| if !self.msrv.is_stable(cx, fused_iterator_trait) { | ||
| return; | ||
| } | ||
|
|
||
| let ty = cx.tcx.type_of(item.owner_id).instantiate_identity().skip_norm_wip(); | ||
| if !cx | ||
| .tcx | ||
| .non_blanket_impls_for_ty(iterator_trait, ty) | ||
| .any(|impl_id| cx.tcx.impl_polarity(impl_id) == ty::ImplPolarity::Positive) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Any implementation, including a conditional or negative one, indicates that the | ||
| // `FusedIterator` status of this nominal type has been considered explicitly. | ||
| if cx | ||
| .tcx | ||
| .non_blanket_impls_for_ty(fused_iterator_trait, ty) | ||
| .next() | ||
| .is_some() | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| span_lint_and_help( | ||
| cx, | ||
| MISSING_FUSED_ITERATOR, | ||
| item.span, | ||
| "this publicly reachable type implements `Iterator` but not `FusedIterator`", | ||
| None, | ||
| "if this iterator remains exhausted after returning `None`, consider implementing `FusedIterator`", | ||
| ); |
There was a problem hiding this comment.
I think the "usual" way to do this is by an long if-chain instead of returning, or?
| if !matches!( | |
| item.kind, | |
| ItemKind::Struct(..) | ItemKind::Enum(..) | ItemKind::Union(..) | |
| ) || !span_is_local(item.span) | |
| || !cx.effective_visibilities.is_reachable(item.owner_id.def_id) | |
| || is_lint_allowed(cx, MISSING_FUSED_ITERATOR, item.hir_id()) | |
| { | |
| return; | |
| } | |
| let Some(iterator_trait) = cx.tcx.lang_items().iterator_trait() else { | |
| return; | |
| }; | |
| let Some(fused_iterator_trait) = cx.tcx.lang_items().fused_iterator_trait() else { | |
| return; | |
| }; | |
| if !self.msrv.is_stable(cx, fused_iterator_trait) { | |
| return; | |
| } | |
| let ty = cx.tcx.type_of(item.owner_id).instantiate_identity().skip_norm_wip(); | |
| if !cx | |
| .tcx | |
| .non_blanket_impls_for_ty(iterator_trait, ty) | |
| .any(|impl_id| cx.tcx.impl_polarity(impl_id) == ty::ImplPolarity::Positive) | |
| { | |
| return; | |
| } | |
| // Any implementation, including a conditional or negative one, indicates that the | |
| // `FusedIterator` status of this nominal type has been considered explicitly. | |
| if cx | |
| .tcx | |
| .non_blanket_impls_for_ty(fused_iterator_trait, ty) | |
| .next() | |
| .is_some() | |
| { | |
| return; | |
| } | |
| span_lint_and_help( | |
| cx, | |
| MISSING_FUSED_ITERATOR, | |
| item.span, | |
| "this publicly reachable type implements `Iterator` but not `FusedIterator`", | |
| None, | |
| "if this iterator remains exhausted after returning `None`, consider implementing `FusedIterator`", | |
| ); | |
| let kind_can_impl_iter = matches!( | |
| item.kind, | |
| ItemKind::Struct(..) | ItemKind::Enum(..) | ItemKind::Union(..) | |
| ); | |
| if kind_can_impl_iter | |
| && span_is_local(item.span) | |
| && cx.effective_visibilities.is_reachable(item.owner_id.def_id) | |
| && !is_lint_allowed(cx, MISSING_FUSED_ITERATOR, item.hir_id()) | |
| && let Some(iterator_trait) = cx.tcx.lang_items().iterator_trait() | |
| && let Some(fused_iterator_trait) = cx.tcx.lang_items().fused_iterator_trait() | |
| && self.msrv.is_stable(cx, fused_iterator_trait) | |
| && let ty = cx.tcx.type_of(item.owner_id).instantiate_identity().skip_norm_wip() | |
| && cx | |
| .tcx | |
| .non_blanket_impls_for_ty(iterator_trait, ty) | |
| .any(|impl_id| cx.tcx.impl_polarity(impl_id) == ty::ImplPolarity::Positive) | |
| // Any implementation, including a conditional or negative one, indicates that the | |
| // `FusedIterator` status of this nominal type has been considered explicitly. | |
| && cx | |
| .tcx | |
| .non_blanket_impls_for_ty(fused_iterator_trait, ty) | |
| .next() | |
| .is_none() { | |
| span_lint_and_help( | |
| cx, | |
| MISSING_FUSED_ITERATOR, | |
| item.span, | |
| "this publicly reachable type implements `Iterator` but not `FusedIterator`", | |
| None, | |
| "if this iterator remains exhausted after returning `None`, consider implementing `FusedIterator`", | |
| ); | |
| } |
| MISSING_FUSED_ITERATOR, | ||
| item.span, | ||
| "this publicly reachable type implements `Iterator` but not `FusedIterator`", | ||
| None, |
There was a problem hiding this comment.
could we suggest the span of the impl Iterator here? I think it would be fairly handy if for example the users do "cursed things" such as putting the iterator impls into different files or such.
| /// this lint, even if its generic bounds do not match every `Iterator` implementation. | ||
| #[clippy::version = "1.100.0"] | ||
| pub MISSING_FUSED_ITERATOR, | ||
| pedantic, |
There was a problem hiding this comment.
should this be restriction, perf or pedantic?
Given that this does restrict the syntax in order to improve perf for users who want, I feel like pedantic might not be the right choice, or? 🤔
| item.span, | ||
| "this publicly reachable type implements `Iterator` but not `FusedIterator`", | ||
| None, | ||
| "if this iterator remains exhausted after returning `None`, consider implementing `FusedIterator`", |
There was a problem hiding this comment.
Should add suggestions for both
impl !FusedIterator for IteratableThing {};impl FusedIterator for IteratableThing {};
negative polarity might not be something that the wider userbase thinks about when reading this lint..
| //@no-rustfix | ||
| //@aux-build:proc_macros.rs | ||
|
|
||
| #![allow(dead_code)] |
There was a problem hiding this comment.
| #![allow(dead_code)] | |
| #![expect(dead_code)] |
There was a problem hiding this comment.
nit: there are a lot of impls. To keep this more "tidy", could you use one module per testcase to wrap this up nicely so that it is clearer what impl belongs to what check and such?
Adds
clippy::missing_fused_iterator, an MSRV-awarepedanticlint for publicly reachable structs, enums, and unions that have a positiveIteratorimpl but noFusedIteratorimpl.Fixes #17608
The lint uses
is_reachablerather than the more commonis_exported, so it also covers types leaked through public signatures. AnyFusedIteratorimpl suppresses it, including a conditional or negative one. I deliberately left two cases out: impls on a reference to the type, and types reachable only through RPIT.I filed it as
pedanticbecause Clippy cannot prove an iterator stays exhausted. If your iterator resumes on purpose, allow the lint and say so in the docs..stderrfile)cargo testpasses locallycargo dev update_lintscargo dev fmtchangelog: new lint: [
missing_fused_iterator]Used an LLM to help me understand the issue and as an extra reviewer for my work.