From e844784d8db048079db95e5b8b954eb7228e6047 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 14 Nov 2023 10:08:19 +0100 Subject: [PATCH 1/4] Simplify --- crates/hir-def/src/body/pretty.rs | 2 +- crates/hir-def/src/data.rs | 6 +- crates/hir-def/src/generics.rs | 2 +- crates/hir-def/src/item_tree/lower.rs | 13 +--- crates/hir-ty/src/db.rs | 26 ++++---- crates/hir-ty/src/lower.rs | 92 ++++++++++++++------------- crates/hir-ty/src/tests/traits.rs | 56 ++++++++-------- crates/hir-ty/src/traits.rs | 4 +- crates/hir/src/display.rs | 2 +- crates/ide-diagnostics/src/lib.rs | 6 +- crates/test-utils/src/minicore.rs | 11 +++- 11 files changed, 110 insertions(+), 110 deletions(-) diff --git a/crates/hir-def/src/body/pretty.rs b/crates/hir-def/src/body/pretty.rs index fad4d7a4da..6ecf1c20d6 100644 --- a/crates/hir-def/src/body/pretty.rs +++ b/crates/hir-def/src/body/pretty.rs @@ -54,7 +54,7 @@ pub(super) fn print_body_hir(db: &dyn DefDatabase, body: &Body, owner: DefWithBo let mut p = Printer { db, body, buf: header, indent_level: 0, needs_indent: false }; if let DefWithBodyId::FunctionId(it) = owner { p.buf.push('('); - body.params.iter().zip(&db.function_data(it).params).for_each(|(¶m, ty)| { + body.params.iter().zip(db.function_data(it).params.iter()).for_each(|(¶m, ty)| { p.print_pat(param); p.buf.push(':'); p.print_type_ref(ty); diff --git a/crates/hir-def/src/data.rs b/crates/hir-def/src/data.rs index 718f241cf7..93fd741aff 100644 --- a/crates/hir-def/src/data.rs +++ b/crates/hir-def/src/data.rs @@ -34,7 +34,7 @@ use crate::{ #[derive(Debug, Clone, PartialEq, Eq)] pub struct FunctionData { pub name: Name, - pub params: Vec>, + pub params: Box<[Interned]>, pub ret_type: Interned, pub attrs: Attrs, pub visibility: RawVisibility, @@ -177,7 +177,7 @@ pub struct TypeAliasData { pub rustc_has_incoherent_inherent_impls: bool, pub rustc_allow_incoherent_impl: bool, /// Bounds restricting the type alias itself (eg. `type Ty: Bound;` in a trait or impl). - pub bounds: Vec>, + pub bounds: Box<[Interned]>, } impl TypeAliasData { @@ -210,7 +210,7 @@ impl TypeAliasData { is_extern: matches!(loc.container, ItemContainerId::ExternBlockId(_)), rustc_has_incoherent_inherent_impls, rustc_allow_incoherent_impl, - bounds: typ.bounds.to_vec(), + bounds: typ.bounds.clone(), }) } } diff --git a/crates/hir-def/src/generics.rs b/crates/hir-def/src/generics.rs index 1e2535a8a9..fac90e6630 100644 --- a/crates/hir-def/src/generics.rs +++ b/crates/hir-def/src/generics.rs @@ -227,7 +227,7 @@ impl GenericParams { let mut expander = Lazy::new(|| { (module.def_map(db), Expander::new(db, loc.source(db).file_id, module)) }); - for param in &func_data.params { + for param in func_data.params.iter() { generic_params.fill_implicit_impl_trait_args(db, &mut expander, param); } diff --git a/crates/hir-def/src/item_tree/lower.rs b/crates/hir-def/src/item_tree/lower.rs index c0a880a64b..30b649d2f3 100644 --- a/crates/hir-def/src/item_tree/lower.rs +++ b/crates/hir-def/src/item_tree/lower.rs @@ -396,14 +396,7 @@ impl<'a> Ctx<'a> { let bounds = self.lower_type_bounds(type_alias); let generic_params = self.lower_generic_params(HasImplicitSelf::No, type_alias); let ast_id = self.source_ast_id_map.ast_id(type_alias); - let res = TypeAlias { - name, - visibility, - bounds: bounds.into_boxed_slice(), - generic_params, - type_ref, - ast_id, - }; + let res = TypeAlias { name, visibility, bounds, generic_params, type_ref, ast_id }; Some(id(self.data().type_aliases.alloc(res))) } @@ -637,13 +630,13 @@ impl<'a> Ctx<'a> { Interned::new(generics) } - fn lower_type_bounds(&mut self, node: &dyn ast::HasTypeBounds) -> Vec> { + fn lower_type_bounds(&mut self, node: &dyn ast::HasTypeBounds) -> Box<[Interned]> { match node.type_bound_list() { Some(bound_list) => bound_list .bounds() .map(|it| Interned::new(TypeBound::from_ast(&self.body_ctx, it))) .collect(), - None => Vec::new(), + None => Box::default(), } } diff --git a/crates/hir-ty/src/db.rs b/crates/hir-ty/src/db.rs index 9c96b5ab8d..410bcbf035 100644 --- a/crates/hir-ty/src/db.rs +++ b/crates/hir-ty/src/db.rs @@ -20,8 +20,8 @@ use crate::{ method_resolution::{InherentImpls, TraitImpls, TyFingerprint}, mir::{BorrowckResult, MirBody, MirLowerError}, Binders, CallableDefId, ClosureId, Const, FnDefId, GenericArg, ImplTraitId, InferenceResult, - Interner, PolyFnSig, QuantifiedWhereClause, ReturnTypeImplTraits, Substitution, TraitRef, Ty, - TyDefId, ValueTyDefId, + Interner, PolyFnSig, QuantifiedWhereClause, ReturnTypeImplTraits, Substitution, + TraitEnvironment, TraitRef, Ty, TyDefId, ValueTyDefId, }; use hir_expand::name::Name; @@ -47,7 +47,7 @@ pub trait HirDatabase: DefDatabase + Upcast { &self, def: DefWithBodyId, subst: Substitution, - env: Arc, + env: Arc, ) -> Result, MirLowerError>; #[salsa::invoke(crate::mir::monomorphized_mir_body_for_closure_query)] @@ -55,7 +55,7 @@ pub trait HirDatabase: DefDatabase + Upcast { &self, def: ClosureId, subst: Substitution, - env: Arc, + env: Arc, ) -> Result, MirLowerError>; #[salsa::invoke(crate::mir::borrowck_query)] @@ -81,7 +81,7 @@ pub trait HirDatabase: DefDatabase + Upcast { &self, def: GeneralConstId, subst: Substitution, - trait_env: Option>, + trait_env: Option>, ) -> Result; #[salsa::invoke(crate::consteval::const_eval_static_query)] @@ -104,16 +104,12 @@ pub trait HirDatabase: DefDatabase + Upcast { &self, def: AdtId, subst: Substitution, - env: Arc, + env: Arc, ) -> Result, LayoutError>; #[salsa::invoke(crate::layout::layout_of_ty_query)] #[salsa::cycle(crate::layout::layout_of_ty_recover)] - fn layout_of_ty( - &self, - ty: Ty, - env: Arc, - ) -> Result, LayoutError>; + fn layout_of_ty(&self, ty: Ty, env: Arc) -> Result, LayoutError>; #[salsa::invoke(crate::layout::target_data_layout_query)] fn target_data_layout(&self, krate: CrateId) -> Option>; @@ -121,7 +117,7 @@ pub trait HirDatabase: DefDatabase + Upcast { #[salsa::invoke(crate::method_resolution::lookup_impl_method_query)] fn lookup_impl_method( &self, - env: Arc, + env: Arc, func: FunctionId, fn_subst: Substitution, ) -> (FunctionId, Substitution); @@ -149,10 +145,10 @@ pub trait HirDatabase: DefDatabase + Upcast { #[salsa::invoke(crate::lower::trait_environment_for_body_query)] #[salsa::transparent] - fn trait_environment_for_body(&self, def: DefWithBodyId) -> Arc; + fn trait_environment_for_body(&self, def: DefWithBodyId) -> Arc; #[salsa::invoke(crate::lower::trait_environment_query)] - fn trait_environment(&self, def: GenericDefId) -> Arc; + fn trait_environment(&self, def: GenericDefId) -> Arc; #[salsa::invoke(crate::lower::generic_defaults_query)] #[salsa::cycle(crate::lower::generic_defaults_recover)] @@ -249,7 +245,7 @@ pub trait HirDatabase: DefDatabase + Upcast { fn normalize_projection( &self, projection: crate::ProjectionTy, - env: Arc, + env: Arc, ) -> Ty; #[salsa::invoke(trait_solve_wait)] diff --git a/crates/hir-ty/src/lower.rs b/crates/hir-ty/src/lower.rs index 9a61f15359..c8a85b4a9f 100644 --- a/crates/hir-ty/src/lower.rs +++ b/crates/hir-ty/src/lower.rs @@ -1383,51 +1383,50 @@ pub(crate) fn generic_predicates_for_param_query( let ctx = TyLoweringContext::new(db, &resolver, def.into()) .with_type_param_mode(ParamLoweringMode::Variable); let generics = generics(db.upcast(), def); + + // we have to filter out all other predicates *first*, before attempting to lower them + let predicate = |pred: &&_| match pred { + WherePredicate::ForLifetime { target, bound, .. } + | WherePredicate::TypeBound { target, bound, .. } => { + let invalid_target = match target { + WherePredicateTypeTarget::TypeRef(type_ref) => { + ctx.lower_ty_only_param(type_ref) != Some(param_id) + } + &WherePredicateTypeTarget::TypeOrConstParam(local_id) => { + let target_id = TypeOrConstParamId { parent: def, local_id }; + target_id != param_id + } + }; + if invalid_target { + return false; + } + + match &**bound { + TypeBound::ForLifetime(_, path) | TypeBound::Path(path, _) => { + // Only lower the bound if the trait could possibly define the associated + // type we're looking for. + + let Some(assoc_name) = &assoc_name else { return true }; + let Some(TypeNs::TraitId(tr)) = + resolver.resolve_path_in_type_ns_fully(db.upcast(), path) + else { + return false; + }; + + all_super_traits(db.upcast(), tr).iter().any(|tr| { + db.trait_data(*tr).items.iter().any(|(name, item)| { + matches!(item, AssocItemId::TypeAliasId(_)) && name == assoc_name + }) + }) + } + TypeBound::Lifetime(_) | TypeBound::Error => false, + } + } + WherePredicate::Lifetime { .. } => false, + }; let mut predicates: Vec<_> = resolver .where_predicates_in_scope() - // we have to filter out all other predicates *first*, before attempting to lower them - .filter(|pred| match pred { - WherePredicate::ForLifetime { target, bound, .. } - | WherePredicate::TypeBound { target, bound, .. } => { - match target { - WherePredicateTypeTarget::TypeRef(type_ref) => { - if ctx.lower_ty_only_param(type_ref) != Some(param_id) { - return false; - } - } - &WherePredicateTypeTarget::TypeOrConstParam(local_id) => { - let target_id = TypeOrConstParamId { parent: def, local_id }; - if target_id != param_id { - return false; - } - } - }; - - match &**bound { - TypeBound::ForLifetime(_, path) | TypeBound::Path(path, _) => { - // Only lower the bound if the trait could possibly define the associated - // type we're looking for. - - let assoc_name = match &assoc_name { - Some(it) => it, - None => return true, - }; - let tr = match resolver.resolve_path_in_type_ns_fully(db.upcast(), path) { - Some(TypeNs::TraitId(tr)) => tr, - _ => return false, - }; - - all_super_traits(db.upcast(), tr).iter().any(|tr| { - db.trait_data(*tr).items.iter().any(|(name, item)| { - matches!(item, AssocItemId::TypeAliasId(_)) && name == assoc_name - }) - }) - } - TypeBound::Lifetime(_) | TypeBound::Error => false, - } - } - WherePredicate::Lifetime { .. } => false, - }) + .filter(predicate) .flat_map(|pred| { ctx.lower_where_predicate(pred, true).map(|p| make_binders(db, &generics, p)) }) @@ -1519,7 +1518,12 @@ pub(crate) fn trait_environment_query( let env = chalk_ir::Environment::new(Interner).add_clauses(Interner, clauses); - Arc::new(TraitEnvironment { krate, block: None, traits_from_clauses: traits_in_scope, env }) + Arc::new(TraitEnvironment { + krate, + block: None, + traits_from_clauses: traits_in_scope.into_boxed_slice(), + env, + }) } /// Resolve the where clause(s) of an item with generics. diff --git a/crates/hir-ty/src/tests/traits.rs b/crates/hir-ty/src/tests/traits.rs index d36b885ec1..73016d38bb 100644 --- a/crates/hir-ty/src/tests/traits.rs +++ b/crates/hir-ty/src/tests/traits.rs @@ -4439,42 +4439,42 @@ fn test(v: S) { fn associated_type_in_argument() { check( r#" - trait A { - fn m(&self) -> i32; - } +trait A { + fn m(&self) -> i32; +} - fn x(k: &::Ty) { - k.m(); - } +fn x(k: &::Ty) { + k.m(); +} - struct X; - struct Y; +struct X; +struct Y; - impl A for X { - fn m(&self) -> i32 { - 8 - } +impl A for X { + fn m(&self) -> i32 { + 8 } +} - impl A for Y { - fn m(&self) -> i32 { - 32 - } +impl A for Y { + fn m(&self) -> i32 { + 32 } +} - trait B { - type Ty: A; - } +trait B { + type Ty: A; +} - impl B for u16 { - type Ty = X; - } +impl B for u16 { + type Ty = X; +} - fn ttt() { - let inp = Y; - x::(&inp); - //^^^^ expected &X, got &Y - } - "#, +fn ttt() { + let inp = Y; + x::(&inp); + //^^^^ expected &X, got &Y +} +"#, ); } diff --git a/crates/hir-ty/src/traits.rs b/crates/hir-ty/src/traits.rs index 3c7cfbaed3..467b94a266 100644 --- a/crates/hir-ty/src/traits.rs +++ b/crates/hir-ty/src/traits.rs @@ -48,7 +48,7 @@ pub struct TraitEnvironment { pub krate: CrateId, pub block: Option, // FIXME make this a BTreeMap - pub(crate) traits_from_clauses: Vec<(Ty, TraitId)>, + pub(crate) traits_from_clauses: Box<[(Ty, TraitId)]>, pub env: chalk_ir::Environment, } @@ -57,7 +57,7 @@ impl TraitEnvironment { TraitEnvironment { krate, block: None, - traits_from_clauses: Vec::new(), + traits_from_clauses: Box::default(), env: chalk_ir::Environment::new(Interner), } } diff --git a/crates/hir/src/display.rs b/crates/hir/src/display.rs index cf3ff62fc6..5847c8a9fb 100644 --- a/crates/hir/src/display.rs +++ b/crates/hir/src/display.rs @@ -616,7 +616,7 @@ impl HirDisplay for TypeAlias { write_where_clause(def_id, f)?; if !data.bounds.is_empty() { f.write_str(": ")?; - f.write_joined(&data.bounds, " + ")?; + f.write_joined(data.bounds.iter(), " + ")?; } if let Some(ty) = &data.type_ref { f.write_str(" = ")?; diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index d8e37a848a..68d7e99b7b 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -301,9 +301,9 @@ pub fn diagnostics( ) })); - let parse = sema.parse(file_id); + let parse = parse.syntax_node(); - for node in parse.syntax().descendants() { + for node in parse.descendants() { handlers::useless_braces::useless_braces(&mut res, file_id, &node); handlers::field_shorthand::field_shorthand(&mut res, file_id, &node); handlers::json_is_not_rust::json_in_items(&sema, &mut res, file_id, &node, config); @@ -386,7 +386,7 @@ pub fn diagnostics( handle_lint_attributes( &ctx.sema, - parse.syntax(), + &parse, &mut rustc_stack, &mut clippy_stack, &mut diagnostics_of_range, diff --git a/crates/test-utils/src/minicore.rs b/crates/test-utils/src/minicore.rs index cc41d87f5d..f2ca9d82ed 100644 --- a/crates/test-utils/src/minicore.rs +++ b/crates/test-utils/src/minicore.rs @@ -44,7 +44,7 @@ //! panic: fmt //! phantom_data: //! pin: -//! pointee: +//! pointee: copy, send, sync, ord, hash, unpin //! range: //! result: //! send: sized @@ -54,6 +54,7 @@ //! sync: sized //! transmute: //! try: infallible +//! unpin: sized //! unsize: sized #![rustc_coherence_is_core] @@ -89,6 +90,11 @@ pub mod marker { pub trait Unsize {} // endregion:unsize + // region:unpin + #[lang = "unpin"] + pub auto trait Unpin {} + // endregion:unpin + // region:copy #[lang = "copy"] pub trait Copy: Clone {} @@ -387,9 +393,10 @@ pub mod ptr { // region:pointee #[lang = "pointee_trait"] + #[rustc_deny_explicit_impl(implement_via_object = false)] pub trait Pointee { #[lang = "metadata_type"] - type Metadata; + type Metadata: Copy + Send + Sync + Ord + Hash + Unpin; } // endregion:pointee // region:non_null From b74015512d06030973641ded964311cfb4e981d0 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 14 Nov 2023 13:32:04 +0100 Subject: [PATCH 2/4] Remove UserError from LayoutError --- crates/hir-ty/src/layout.rs | 57 ++++++++++++++++++++----------- crates/hir-ty/src/layout/adt.rs | 8 ++--- crates/hir-ty/src/layout/tests.rs | 7 ++-- crates/hir-ty/src/tests/traits.rs | 2 +- 4 files changed, 42 insertions(+), 32 deletions(-) diff --git a/crates/hir-ty/src/layout.rs b/crates/hir-ty/src/layout.rs index 603e58f9d4..b2591f016d 100644 --- a/crates/hir-ty/src/layout.rs +++ b/crates/hir-ty/src/layout.rs @@ -1,5 +1,7 @@ //! Compute the binary representation of a type +use std::fmt; + use chalk_ir::{AdtId, FloatTy, IntTy, TyKind, UintTy}; use hir_def::{ layout::{ @@ -26,12 +28,6 @@ pub use self::{ target::target_data_layout_query, }; -macro_rules! user_error { - ($it: expr) => { - return Err(LayoutError::UserError(format!($it).into())) - }; -} - mod adt; mod target; @@ -73,13 +69,38 @@ pub type Variants = hir_def::layout::Variants), + HasErrorConst, + HasErrorType, + HasPlaceholder, + InvalidSimdType, + NotImplemented, + RecursiveTypeWithoutIndirection, SizeOverflow, TargetLayoutNotAvailable, - HasPlaceholder, - HasErrorType, - NotImplemented, Unknown, + UserReprTooSmall, +} + +impl std::error::Error for LayoutError {} +impl fmt::Display for LayoutError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + LayoutError::HasErrorConst => write!(f, "type contains an unevaluatable const"), + LayoutError::HasErrorType => write!(f, "type contains an error"), + LayoutError::HasPlaceholder => write!(f, "type contains placeholders"), + LayoutError::InvalidSimdType => write!(f, "invalid simd type definition"), + LayoutError::NotImplemented => write!(f, "not implemented"), + LayoutError::RecursiveTypeWithoutIndirection => { + write!(f, "recursive type without indirection") + } + LayoutError::SizeOverflow => write!(f, "size overflow"), + LayoutError::TargetLayoutNotAvailable => write!(f, "target layout not available"), + LayoutError::Unknown => write!(f, "unknown"), + LayoutError::UserReprTooSmall => { + write!(f, "the `#[repr]` hint is too small to hold the discriminants of the enum") + } + } + } } struct LayoutCx<'a> { @@ -118,9 +139,7 @@ fn layout_of_simd_ty( let f0_ty = match fields.iter().next() { Some(it) => it.1.clone().substitute(Interner, subst), - None => { - user_error!("simd type with zero fields"); - } + None => return Err(LayoutError::InvalidSimdType), }; // The element type and number of elements of the SIMD vector @@ -134,7 +153,7 @@ fn layout_of_simd_ty( // Extract the number of elements from the layout of the array field: let FieldsShape::Array { count, .. } = db.layout_of_ty(f0_ty.clone(), env.clone())?.fields else { - user_error!("Array with non array layout"); + return Err(LayoutError::Unknown); }; (e_ty.clone(), count, true) @@ -146,7 +165,7 @@ fn layout_of_simd_ty( // Compute the ABI of the element type: let e_ly = db.layout_of_ty(e_ty, env.clone())?; let Abi::Scalar(e_abi) = e_ly.abi else { - user_error!("simd type with inner non scalar type"); + return Err(LayoutError::Unknown); }; // Compute the size and alignment of the vector: @@ -259,9 +278,7 @@ pub fn layout_of_ty_query( cx.univariant(dl, &fields, &ReprOptions::default(), kind).ok_or(LayoutError::Unknown)? } TyKind::Array(element, count) => { - let count = try_const_usize(db, &count).ok_or(LayoutError::UserError(Box::from( - "unevaluated or mistyped const generic parameter", - )))? as u64; + let count = try_const_usize(db, &count).ok_or(LayoutError::HasErrorConst)? as u64; let element = db.layout_of_ty(element.clone(), trait_env.clone())?; let size = element.size.checked_mul(count, dl).ok_or(LayoutError::SizeOverflow)?; @@ -352,7 +369,7 @@ pub fn layout_of_ty_query( let mut unit = layout_of_unit(&cx, dl)?; match unit.abi { Abi::Aggregate { ref mut sized } => *sized = false, - _ => user_error!("bug"), + _ => return Err(LayoutError::Unknown), } unit } @@ -418,7 +435,7 @@ pub fn layout_of_ty_recover( _: &Ty, _: &Arc, ) -> Result, LayoutError> { - user_error!("infinite sized recursive type"); + Err(LayoutError::RecursiveTypeWithoutIndirection) } fn layout_of_unit(cx: &LayoutCx<'_>, dl: &TargetDataLayout) -> Result { diff --git a/crates/hir-ty/src/layout/adt.rs b/crates/hir-ty/src/layout/adt.rs index 5e713c17cf..58a06dc643 100644 --- a/crates/hir-ty/src/layout/adt.rs +++ b/crates/hir-ty/src/layout/adt.rs @@ -145,7 +145,7 @@ pub fn layout_of_adt_recover( _: &Substitution, _: &Arc, ) -> Result, LayoutError> { - user_error!("infinite sized recursive type"); + Err(LayoutError::RecursiveTypeWithoutIndirection) } /// Finds the appropriate Integer type and signedness for the given @@ -169,11 +169,7 @@ fn repr_discr( let discr = Integer::from_attr(dl, ity); let fit = if ity.is_signed() { signed_fit } else { unsigned_fit }; if discr < fit { - return Err(LayoutError::UserError( - "Integer::repr_discr: `#[repr]` hint too small for \ - discriminant range of enum " - .into(), - )); + return Err(LayoutError::UserReprTooSmall); } return Ok((discr, ity.is_signed())); } diff --git a/crates/hir-ty/src/layout/tests.rs b/crates/hir-ty/src/layout/tests.rs index ffdbb9de93..5e3a86c80e 100644 --- a/crates/hir-ty/src/layout/tests.rs +++ b/crates/hir-ty/src/layout/tests.rs @@ -210,16 +210,13 @@ fn recursive() { struct BoxLike(*mut T); struct Goal(BoxLike); } - check_fail( - r#"struct Goal(Goal);"#, - LayoutError::UserError("infinite sized recursive type".into()), - ); + check_fail(r#"struct Goal(Goal);"#, LayoutError::RecursiveTypeWithoutIndirection); check_fail( r#" struct Foo(Foo); struct Goal(Foo); "#, - LayoutError::UserError("infinite sized recursive type".into()), + LayoutError::RecursiveTypeWithoutIndirection, ); } diff --git a/crates/hir-ty/src/tests/traits.rs b/crates/hir-ty/src/tests/traits.rs index 73016d38bb..48dd954032 100644 --- a/crates/hir-ty/src/tests/traits.rs +++ b/crates/hir-ty/src/tests/traits.rs @@ -4473,7 +4473,7 @@ impl B for u16 { fn ttt() { let inp = Y; x::(&inp); - //^^^^ expected &X, got &Y + //^^^^ expected &X, got &Y } "#, ); From 6ddccc9a6e81ebd3350568575051ac795184fa81 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 14 Nov 2023 18:09:28 +0100 Subject: [PATCH 3/4] Diagnose some orphan trait impl cases --- crates/hir-ty/src/diagnostics.rs | 6 - crates/hir-ty/src/lib.rs | 1 + crates/hir-ty/src/method_resolution.rs | 56 +++++++++ crates/hir/src/diagnostics.rs | 15 ++- crates/hir/src/lib.rs | 13 ++- .../src/handlers/trait_impl_orphan.rs | 106 ++++++++++++++++++ crates/ide-diagnostics/src/lib.rs | 8 +- crates/ide-diagnostics/src/tests.rs | 15 ++- 8 files changed, 205 insertions(+), 15 deletions(-) create mode 100644 crates/ide-diagnostics/src/handlers/trait_impl_orphan.rs diff --git a/crates/hir-ty/src/diagnostics.rs b/crates/hir-ty/src/diagnostics.rs index ef43ed5c46..c1b3619009 100644 --- a/crates/hir-ty/src/diagnostics.rs +++ b/crates/hir-ty/src/diagnostics.rs @@ -11,9 +11,3 @@ pub use crate::diagnostics::{ }, unsafe_check::{missing_unsafe, unsafe_expressions, UnsafeExpr}, }; - -#[derive(Debug, PartialEq, Eq)] -pub struct IncoherentImpl { - pub file_id: hir_expand::HirFileId, - pub impl_: syntax::AstPtr, -} diff --git a/crates/hir-ty/src/lib.rs b/crates/hir-ty/src/lib.rs index 405bb001b5..35a3a1ad8d 100644 --- a/crates/hir-ty/src/lib.rs +++ b/crates/hir-ty/src/lib.rs @@ -80,6 +80,7 @@ pub use mapping::{ lt_from_placeholder_idx, to_assoc_type_id, to_chalk_trait_id, to_foreign_def_id, to_placeholder_idx, }; +pub use method_resolution::check_orphan_rules; pub use traits::TraitEnvironment; pub use utils::{all_super_traits, is_fn_unsafe_to_call}; diff --git a/crates/hir-ty/src/method_resolution.rs b/crates/hir-ty/src/method_resolution.rs index f3a5f69b2a..87c9328336 100644 --- a/crates/hir-ty/src/method_resolution.rs +++ b/crates/hir-ty/src/method_resolution.rs @@ -862,6 +862,62 @@ fn is_inherent_impl_coherent( } } +/// Checks whether the impl satisfies the orphan rules. +/// +/// Given `impl Trait for T0`, an `impl`` is valid only if at least one of the following is true: +/// - Trait is a local trait +/// - All of +/// - At least one of the types `T0..=Tn`` must be a local type. Let `Ti`` be the first such type. +/// - No uncovered type parameters `P1..=Pn` may appear in `T0..Ti`` (excluding `Ti`) +pub fn check_orphan_rules(db: &dyn HirDatabase, impl_: ImplId) -> bool { + let substs = TyBuilder::placeholder_subst(db, impl_); + let Some(impl_trait) = db.impl_trait(impl_) else { + // not a trait impl + return true; + }; + + let local_crate = impl_.lookup(db.upcast()).container.krate(); + let is_local = |tgt_crate| tgt_crate == local_crate; + + let trait_ref = impl_trait.substitute(Interner, &substs); + let trait_id = from_chalk_trait_id(trait_ref.trait_id); + if is_local(trait_id.module(db.upcast()).krate()) { + // trait to be implemented is local + return true; + } + + let unwrap_fundamental = |ty: Ty| match ty.kind(Interner) { + TyKind::Ref(_, _, referenced) => referenced.clone(), + &TyKind::Adt(AdtId(hir_def::AdtId::StructId(s)), ref subs) => { + let struct_data = db.struct_data(s); + if struct_data.flags.contains(StructFlags::IS_FUNDAMENTAL) { + let next = subs.type_parameters(Interner).next(); + match next { + Some(ty) => ty, + None => ty, + } + } else { + ty + } + } + _ => ty, + }; + // - At least one of the types `T0..=Tn`` must be a local type. Let `Ti`` be the first such type. + let is_not_orphan = trait_ref.substitution.type_parameters(Interner).any(|ty| { + match unwrap_fundamental(ty).kind(Interner) { + &TyKind::Adt(AdtId(id), _) => is_local(id.module(db.upcast()).krate()), + TyKind::Error => true, + TyKind::Dyn(it) => it.principal().map_or(false, |trait_ref| { + is_local(from_chalk_trait_id(trait_ref.trait_id).module(db.upcast()).krate()) + }), + _ => false, + } + }); + // FIXME: param coverage + // - No uncovered type parameters `P1..=Pn` may appear in `T0..Ti`` (excluding `Ti`) + is_not_orphan +} + pub fn iterate_path_candidates( ty: &Canonical, db: &dyn HirDatabase, diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 67d3169243..3a45622675 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -3,7 +3,7 @@ //! //! This probably isn't the best way to do this -- ideally, diagnostics should //! be expressed in terms of hir types themselves. -pub use hir_ty::diagnostics::{CaseType, IncoherentImpl, IncorrectCase}; +pub use hir_ty::diagnostics::{CaseType, IncorrectCase}; use base_db::CrateId; use cfg::{CfgExpr, CfgOptions}; @@ -38,6 +38,7 @@ diagnostics![ IncorrectCase, InvalidDeriveTarget, IncoherentImpl, + TraitImplOrphan, MacroDefError, MacroError, MacroExpansionParseError, @@ -280,3 +281,15 @@ pub struct MovedOutOfRef { pub ty: Type, pub span: InFile, } + +#[derive(Debug, PartialEq, Eq)] +pub struct IncoherentImpl { + pub file_id: HirFileId, + pub impl_: AstPtr, +} + +#[derive(Debug, PartialEq, Eq)] +pub struct TraitImplOrphan { + pub file_id: HirFileId, + pub impl_: AstPtr, +} diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 6fcc02fb9d..dfb0779f10 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -60,7 +60,7 @@ use hir_def::{ }; use hir_expand::{name::name, MacroCallKind}; use hir_ty::{ - all_super_traits, autoderef, + all_super_traits, autoderef, check_orphan_rules, consteval::{try_const_usize, unknown_const_as_generic, ConstEvalError, ConstExt}, diagnostics::BodyValidationDiagnostic, known_const_to_ast, @@ -95,7 +95,7 @@ pub use crate::{ MacroExpansionParseError, MalformedDerive, MismatchedArgCount, MismatchedTupleStructPatArgCount, MissingFields, MissingMatchArms, MissingUnsafe, MovedOutOfRef, NeedMut, NoSuchField, PrivateAssocItem, PrivateField, - ReplaceFilterMapNextWithFindMap, TypeMismatch, TypedHole, UndeclaredLabel, + ReplaceFilterMapNextWithFindMap, TraitImplOrphan, TypeMismatch, TypedHole, UndeclaredLabel, UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate, UnresolvedField, UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall, UnresolvedModule, UnresolvedProcMacro, UnusedMut, UnusedVariable, @@ -624,6 +624,11 @@ impl Module { acc.push(IncoherentImpl { impl_: ast_id_map.get(node.ast_id()), file_id }.into()) } + if !impl_def.check_orphan_rules(db) { + let ast_id_map = db.ast_id_map(file_id); + acc.push(TraitImplOrphan { impl_: ast_id_map.get(node.ast_id()), file_id }.into()) + } + for item in impl_def.items(db) { let def: DefWithBody = match item { AssocItem::Function(it) => it.into(), @@ -3406,6 +3411,10 @@ impl Impl { let src = self.source(db)?; src.file_id.as_builtin_derive_attr_node(db.upcast()) } + + pub fn check_orphan_rules(self, db: &dyn HirDatabase) -> bool { + check_orphan_rules(db, self.id) + } } #[derive(Clone, PartialEq, Eq, Debug, Hash)] diff --git a/crates/ide-diagnostics/src/handlers/trait_impl_orphan.rs b/crates/ide-diagnostics/src/handlers/trait_impl_orphan.rs new file mode 100644 index 0000000000..159d87d269 --- /dev/null +++ b/crates/ide-diagnostics/src/handlers/trait_impl_orphan.rs @@ -0,0 +1,106 @@ +use hir::InFile; + +use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext}; + +// Diagnostic: trait-impl-orphan +// +// Only traits defined in the current crate can be implemented for arbitrary types +pub(crate) fn trait_impl_orphan( + ctx: &DiagnosticsContext<'_>, + d: &hir::TraitImplOrphan, +) -> Diagnostic { + Diagnostic::new_with_syntax_node_ptr( + ctx, + DiagnosticCode::RustcHardError("E0117"), + format!("only traits defined in the current crate can be implemented for arbitrary types"), + InFile::new(d.file_id, d.impl_.clone().into()), + ) + // Not yet checked for false positives + .experimental() +} + +#[cfg(test)] +mod tests { + use crate::tests::check_diagnostics; + + #[test] + fn simple() { + check_diagnostics( + r#" +//- /foo.rs crate:foo +pub trait Foo {} +//- /bar.rs crate:bar +pub struct Bar; +//- /main.rs crate:main deps:foo,bar +struct LocalType; +trait LocalTrait {} + impl foo::Foo for bar::Bar {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types +impl foo::Foo for LocalType {} +impl LocalTrait for bar::Bar {} +"#, + ); + } + + #[test] + fn generics() { + check_diagnostics( + r#" +//- /foo.rs crate:foo +pub trait Foo {} +//- /bar.rs crate:bar +pub struct Bar(T); +//- /main.rs crate:main deps:foo,bar +struct LocalType; +trait LocalTrait {} + impl foo::Foo for bar::Bar {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types + + impl foo::Foo for bar::Bar> {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types + + impl foo::Foo> for bar::Bar {} + + impl foo::Foo>> for bar::Bar> {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types +"#, + ); + } + + #[test] + fn fundamental() { + check_diagnostics( + r#" +//- /foo.rs crate:foo +pub trait Foo {} +//- /bar.rs crate:bar +pub struct Bar(T); +#[lang = "owned_box"] +#[fundamental] +pub struct Box(T); +//- /main.rs crate:main deps:foo,bar +struct LocalType; + impl foo::Foo for bar::Box {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types + impl foo::Foo for &LocalType {} + impl foo::Foo for bar::Box {} +"#, + ); + } + + #[test] + fn dyn_object() { + check_diagnostics( + r#" +//- /foo.rs crate:foo +pub trait Foo {} +//- /bar.rs crate:bar +pub struct Bar; +//- /main.rs crate:main deps:foo,bar +trait LocalTrait {} +impl foo::Foo for dyn LocalTrait {} +impl foo::Foo for Bar {} +"#, + ); + } +} diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index 68d7e99b7b..3226dd29d2 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -44,6 +44,7 @@ mod handlers { pub(crate) mod private_assoc_item; pub(crate) mod private_field; pub(crate) mod replace_filter_map_next_with_find_map; + pub(crate) mod trait_impl_orphan; pub(crate) mod typed_hole; pub(crate) mod type_mismatch; pub(crate) mod unimplemented_builtin_macro; @@ -301,9 +302,9 @@ pub fn diagnostics( ) })); - let parse = parse.syntax_node(); + let parse = sema.parse(file_id); - for node in parse.descendants() { + for node in parse.syntax().descendants() { handlers::useless_braces::useless_braces(&mut res, file_id, &node); handlers::field_shorthand::field_shorthand(&mut res, file_id, &node); handlers::json_is_not_rust::json_in_items(&sema, &mut res, file_id, &node, config); @@ -358,6 +359,7 @@ pub fn diagnostics( AnyDiagnostic::PrivateAssocItem(d) => handlers::private_assoc_item::private_assoc_item(&ctx, &d), AnyDiagnostic::PrivateField(d) => handlers::private_field::private_field(&ctx, &d), AnyDiagnostic::ReplaceFilterMapNextWithFindMap(d) => handlers::replace_filter_map_next_with_find_map::replace_filter_map_next_with_find_map(&ctx, &d), + AnyDiagnostic::TraitImplOrphan(d) => handlers::trait_impl_orphan::trait_impl_orphan(&ctx, &d), AnyDiagnostic::TypedHole(d) => handlers::typed_hole::typed_hole(&ctx, &d), AnyDiagnostic::TypeMismatch(d) => handlers::type_mismatch::type_mismatch(&ctx, &d), AnyDiagnostic::UndeclaredLabel(d) => handlers::undeclared_label::undeclared_label(&ctx, &d), @@ -386,7 +388,7 @@ pub fn diagnostics( handle_lint_attributes( &ctx.sema, - &parse, + parse.syntax(), &mut rustc_stack, &mut clippy_stack, &mut diagnostics_of_range, diff --git a/crates/ide-diagnostics/src/tests.rs b/crates/ide-diagnostics/src/tests.rs index ee0e035490..ff8f3b2686 100644 --- a/crates/ide-diagnostics/src/tests.rs +++ b/crates/ide-diagnostics/src/tests.rs @@ -5,7 +5,7 @@ use expect_test::Expect; use ide_db::{ assists::AssistResolveStrategy, base_db::{fixture::WithFixture, SourceDatabaseExt}, - RootDatabase, + LineIndexDatabase, RootDatabase, }; use stdx::trim_indent; use test_utils::{assert_eq_text, extract_annotations, MiniCore}; @@ -103,6 +103,7 @@ pub(crate) fn check_diagnostics(ra_fixture: &str) { pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixture: &str) { let (db, files) = RootDatabase::with_many_files(ra_fixture); for file_id in files { + let line_index = db.line_index(file_id); let diagnostics = super::diagnostics(&db, &config, &AssistResolveStrategy::All, file_id); let expected = extract_annotations(&db.file_text(file_id)); @@ -136,8 +137,16 @@ pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixtur } } if expected != actual { - let fneg = expected.iter().filter(|x| !actual.contains(x)).collect::>(); - let fpos = actual.iter().filter(|x| !expected.contains(x)).collect::>(); + let fneg = expected + .iter() + .filter(|x| !actual.contains(x)) + .map(|(range, s)| (line_index.line_col(range.start()), range, s)) + .collect::>(); + let fpos = actual + .iter() + .filter(|x| !expected.contains(x)) + .map(|(range, s)| (line_index.line_col(range.start()), range, s)) + .collect::>(); panic!("Diagnostic test failed.\nFalse negatives: {fneg:?}\nFalse positives: {fpos:?}"); } From d5faad1dae0b5c43b19efbfba7a268f1800b56e4 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 14 Nov 2023 18:53:17 +0100 Subject: [PATCH 4/4] Fix inlay-hint tests being invalidated by minicore chanes --- crates/hir/src/lib.rs | 4 ++++ crates/ide/src/inlay_hints.rs | 17 +++++++++++++++++ crates/ide/src/inlay_hints/chaining.rs | 20 +++++++++++--------- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index dfb0779f10..8d82e88da9 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -3403,6 +3403,10 @@ impl Impl { db.impl_data(self.id).is_negative } + pub fn is_unsafe(self, db: &dyn HirDatabase) -> bool { + db.impl_data(self.id).is_unique() + } + pub fn module(self, db: &dyn HirDatabase) -> Module { self.id.lookup(db.upcast()).container.into() } diff --git a/crates/ide/src/inlay_hints.rs b/crates/ide/src/inlay_hints.rs index a5d070fe76..24f44ca06f 100644 --- a/crates/ide/src/inlay_hints.rs +++ b/crates/ide/src/inlay_hints.rs @@ -563,6 +563,7 @@ mod tests { use hir::ClosureStyle; use itertools::Itertools; use test_utils::extract_annotations; + use text_edit::{TextRange, TextSize}; use crate::inlay_hints::{AdjustmentHints, AdjustmentHintsMode}; use crate::DiscriminantHints; @@ -629,6 +630,22 @@ mod tests { expect.assert_debug_eq(&inlay_hints) } + #[track_caller] + pub(super) fn check_expect_clear_loc( + config: InlayHintsConfig, + ra_fixture: &str, + expect: Expect, + ) { + let (analysis, file_id) = fixture::file(ra_fixture); + let mut inlay_hints = analysis.inlay_hints(&config, file_id, None).unwrap(); + inlay_hints.iter_mut().flat_map(|hint| &mut hint.label.parts).for_each(|hint| { + if let Some(loc) = &mut hint.linked_location { + loc.range = TextRange::empty(TextSize::from(0)); + } + }); + expect.assert_debug_eq(&inlay_hints) + } + /// Computes inlay hints for the fixture, applies all the provided text edits and then runs /// expect test. #[track_caller] diff --git a/crates/ide/src/inlay_hints/chaining.rs b/crates/ide/src/inlay_hints/chaining.rs index 4152e60675..c9e9a22378 100644 --- a/crates/ide/src/inlay_hints/chaining.rs +++ b/crates/ide/src/inlay_hints/chaining.rs @@ -78,7 +78,9 @@ mod tests { use expect_test::expect; use crate::{ - inlay_hints::tests::{check_expect, check_with_config, DISABLED_CONFIG, TEST_CONFIG}, + inlay_hints::tests::{ + check_expect, check_expect_clear_loc, check_with_config, DISABLED_CONFIG, TEST_CONFIG, + }, InlayHintsConfig, }; @@ -444,7 +446,7 @@ fn main() { #[test] fn shorten_iterator_chaining_hints() { - check_expect( + check_expect_clear_loc( InlayHintsConfig { chaining_hints: true, ..DISABLED_CONFIG }, r#" //- minicore: iterators @@ -484,7 +486,7 @@ fn main() { file_id: FileId( 1, ), - range: 10752..10760, + range: 0..0, }, ), tooltip: "", @@ -497,7 +499,7 @@ fn main() { file_id: FileId( 1, ), - range: 10784..10788, + range: 0..0, }, ), tooltip: "", @@ -522,7 +524,7 @@ fn main() { file_id: FileId( 1, ), - range: 10752..10760, + range: 0..0, }, ), tooltip: "", @@ -535,7 +537,7 @@ fn main() { file_id: FileId( 1, ), - range: 10784..10788, + range: 0..0, }, ), tooltip: "", @@ -560,7 +562,7 @@ fn main() { file_id: FileId( 1, ), - range: 10752..10760, + range: 0..0, }, ), tooltip: "", @@ -573,7 +575,7 @@ fn main() { file_id: FileId( 1, ), - range: 10784..10788, + range: 0..0, }, ), tooltip: "", @@ -598,7 +600,7 @@ fn main() { file_id: FileId( 0, ), - range: 24..30, + range: 0..0, }, ), tooltip: "",