-
Notifications
You must be signed in to change notification settings - Fork 1.9k
minor: fix some clippy lints #10135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
minor: fix some clippy lints #10135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -384,7 +384,7 @@ impl DefCollector<'_> { | |
| self.unresolved_imports.extend(partial_resolved); | ||
| self.resolve_imports(); | ||
|
|
||
| let unresolved_imports = std::mem::replace(&mut self.unresolved_imports, Vec::new()); | ||
| let unresolved_imports = std::mem::take(&mut self.unresolved_imports); | ||
| // show unresolved imports in completion, etc | ||
| for directive in &unresolved_imports { | ||
| self.record_resolved_import(directive) | ||
|
|
@@ -417,7 +417,7 @@ impl DefCollector<'_> { | |
| fn reseed_with_unresolved_attribute(&mut self) -> ReachedFixedPoint { | ||
| cov_mark::hit!(unresolved_attribute_fallback); | ||
|
|
||
| let mut unresolved_macros = std::mem::replace(&mut self.unresolved_macros, Vec::new()); | ||
| let mut unresolved_macros = std::mem::take(&mut self.unresolved_macros); | ||
| let pos = unresolved_macros.iter().position(|directive| { | ||
| if let MacroDirectiveKind::Attr { ast_id, mod_item, attr } = &directive.kind { | ||
| self.skip_attrs.insert(ast_id.ast_id.with_value(*mod_item), attr.id); | ||
|
|
@@ -689,7 +689,7 @@ impl DefCollector<'_> { | |
| /// Tries to resolve every currently unresolved import. | ||
| fn resolve_imports(&mut self) -> ReachedFixedPoint { | ||
| let mut res = ReachedFixedPoint::Yes; | ||
| let imports = std::mem::replace(&mut self.unresolved_imports, Vec::new()); | ||
| let imports = std::mem::take(&mut self.unresolved_imports); | ||
| let imports = imports | ||
| .into_iter() | ||
| .filter_map(|mut directive| { | ||
|
|
@@ -1005,7 +1005,7 @@ impl DefCollector<'_> { | |
| } | ||
|
|
||
| fn resolve_macros(&mut self) -> ReachedFixedPoint { | ||
| let mut macros = std::mem::replace(&mut self.unresolved_macros, Vec::new()); | ||
| let mut macros = std::mem::take(&mut self.unresolved_macros); | ||
| let mut resolved = Vec::new(); | ||
| let mut res = ReachedFixedPoint::Yes; | ||
| macros.retain(|directive| { | ||
|
|
@@ -1279,13 +1279,12 @@ impl DefCollector<'_> { | |
|
|
||
| for directive in &self.unresolved_imports { | ||
| if let ImportSource::Import { id: import, use_tree } = &directive.import.source { | ||
| match (directive.import.path.segments().first(), &directive.import.path.kind) { | ||
| (Some(krate), PathKind::Plain | PathKind::Abs) => { | ||
| if diagnosed_extern_crates.contains(krate) { | ||
| continue; | ||
| } | ||
| if let (Some(krate), PathKind::Plain | PathKind::Abs) = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was intentional.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how so? It doesn't really change the nesting level, though on another note match + if guard seems like the proper way here
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think matklad generally prefers
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this isn't an
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you're right.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that was indeed intentional -- multiline conditions are not readable. In if let (Some(krate), PathKind::Plain | PathKind::Abs) =
(directive.import.path.segments().first(), &directive.import.path.kind)you need to read a lot to understand what the heck are you even matching. With But no strong opinion here! |
||
| (directive.import.path.segments().first(), &directive.import.path.kind) | ||
| { | ||
| if diagnosed_extern_crates.contains(krate) { | ||
| continue; | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
||
| self.def_map.diagnostics.push(DefDiagnostic::unresolved_import( | ||
|
|
@@ -1579,13 +1578,13 @@ impl ModCollector<'_, '_> { | |
| { | ||
| Ok((file_id, is_mod_rs, mod_dir)) => { | ||
| let item_tree = db.file_item_tree(file_id.into()); | ||
| if item_tree | ||
| let is_enabled = item_tree | ||
| .top_level_attrs(db, self.def_collector.def_map.krate) | ||
| .cfg() | ||
| .map_or(true, |cfg| { | ||
| self.def_collector.cfg_options.check(&cfg) != Some(false) | ||
| }) | ||
| { | ||
| }); | ||
| if is_enabled { | ||
| let module_id = self.push_child_module( | ||
| module.name.clone(), | ||
| ast_id, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| //! Inference of closure parameter types based on the closure's expected type. | ||
|
|
||
| use chalk_ir::{cast::Cast, AliasTy, FnSubst, WhereClause}; | ||
| use chalk_ir::{cast::Cast, AliasEq, AliasTy, FnSubst, WhereClause}; | ||
| use hir_def::{expr::ExprId, HasModule}; | ||
| use smallvec::SmallVec; | ||
|
|
||
|
|
@@ -42,46 +42,38 @@ impl InferenceContext<'_> { | |
|
|
||
| let fn_traits: SmallVec<[ChalkTraitId; 3]> = | ||
| utils::fn_traits(self.db.upcast(), self.owner.module(self.db.upcast()).krate()) | ||
| .map(|tid| to_chalk_trait_id(tid)) | ||
| .map(to_chalk_trait_id) | ||
| .collect(); | ||
|
|
||
| let self_ty = TyKind::Error.intern(&Interner); | ||
| let bounds = dyn_ty.bounds.clone().substitute(&Interner, &[self_ty.cast(&Interner)]); | ||
| for bound in bounds.iter(&Interner) { | ||
| // NOTE(skip_binders): the extracted types are rebound by the returned `FnPointer` | ||
| match bound.skip_binders() { | ||
| WhereClause::AliasEq(eq) => match &eq.alias { | ||
| AliasTy::Projection(projection) => { | ||
| let assoc_data = self.db.associated_ty_data(projection.associated_ty_id); | ||
| if !fn_traits.contains(&assoc_data.trait_id) { | ||
| return None; | ||
| } | ||
| if let WhereClause::AliasEq(AliasEq { alias: AliasTy::Projection(projection), ty }) = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably intentional
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For what reason? To me collapsing the matches seems better personally
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto: here, a complicated pattern comes before the expression to match. |
||
| bound.skip_binders() | ||
| { | ||
| let assoc_data = self.db.associated_ty_data(projection.associated_ty_id); | ||
| if !fn_traits.contains(&assoc_data.trait_id) { | ||
| return None; | ||
| } | ||
|
|
||
| // Skip `Self`, get the type argument. | ||
| let arg = projection.substitution.as_slice(&Interner).get(1)?; | ||
| if let Some(subst) = arg.ty(&Interner)?.as_tuple() { | ||
| let generic_args = subst.as_slice(&Interner); | ||
| let mut sig_tys = Vec::new(); | ||
| for arg in generic_args { | ||
| sig_tys.push(arg.ty(&Interner)?.clone()); | ||
| } | ||
| sig_tys.push(eq.ty.clone()); | ||
|
|
||
| cov_mark::hit!(dyn_fn_param_informs_call_site_closure_signature); | ||
| return Some(FnPointer { | ||
| num_binders: bound.len(&Interner), | ||
| sig: FnSig { | ||
| abi: (), | ||
| safety: chalk_ir::Safety::Safe, | ||
| variadic: false, | ||
| }, | ||
| substitution: FnSubst(Substitution::from_iter(&Interner, sig_tys)), | ||
| }); | ||
| } | ||
| // Skip `Self`, get the type argument. | ||
| let arg = projection.substitution.as_slice(&Interner).get(1)?; | ||
| if let Some(subst) = arg.ty(&Interner)?.as_tuple() { | ||
| let generic_args = subst.as_slice(&Interner); | ||
| let mut sig_tys = Vec::new(); | ||
| for arg in generic_args { | ||
| sig_tys.push(arg.ty(&Interner)?.clone()); | ||
| } | ||
| AliasTy::Opaque(_) => {} | ||
| }, | ||
| _ => {} | ||
| sig_tys.push(ty.clone()); | ||
|
|
||
| cov_mark::hit!(dyn_fn_param_informs_call_site_closure_signature); | ||
| return Some(FnPointer { | ||
| num_binders: bound.len(&Interner), | ||
| sig: FnSig { abi: (), safety: chalk_ir::Safety::Safe, variadic: false }, | ||
| substitution: FnSubst(Substitution::from_iter(&Interner, sig_tys)), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine, it makes sense, but the default branch was a bit weird (we might want to know if new message types are added).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we won't know that though because due to non exhaustiveness we have to use a wildcard no matter what
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's was an "ugh" for the old code, not for your change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstream added non-exhaustive, that's why we need this here. Last weekend I almost got to prototyping miniserde-based cargo data to have more control over this :-)