From eee7de0225603387064fa63768f782b66b7e6018 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 1 Jan 2023 13:24:48 +0100 Subject: [PATCH] Diagnose private assoc item accesses --- crates/hir-def/src/expr.rs | 7 + crates/hir-ty/src/infer.rs | 12 +- crates/hir-ty/src/infer/expr.rs | 10 +- crates/hir-ty/src/infer/path.rs | 14 +- crates/hir-ty/src/method_resolution.rs | 8 +- crates/hir/src/diagnostics.rs | 10 +- crates/hir/src/lib.rs | 23 +++- .../src/handlers/private_assoc_item.rs | 124 ++++++++++++++++++ .../src/handlers/private_field.rs | 15 ++- crates/ide-diagnostics/src/lib.rs | 2 + 10 files changed, 199 insertions(+), 26 deletions(-) create mode 100644 crates/ide-diagnostics/src/handlers/private_assoc_item.rs diff --git a/crates/hir-def/src/expr.rs b/crates/hir-def/src/expr.rs index 3066213ace8..7b656942119 100644 --- a/crates/hir-def/src/expr.rs +++ b/crates/hir-def/src/expr.rs @@ -36,6 +36,13 @@ pub(crate) fn dummy_expr_id() -> ExprId { pub type PatId = Idx; +#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] +pub enum ExprOrPatId { + ExprId(ExprId), + PatId(PatId), +} +stdx::impl_from!(ExprId, PatId for ExprOrPatId); + #[derive(Debug, Clone, Eq, PartialEq)] pub struct Label { pub name: Name, diff --git a/crates/hir-ty/src/infer.rs b/crates/hir-ty/src/infer.rs index e38c2154413..7b54886d53f 100644 --- a/crates/hir-ty/src/infer.rs +++ b/crates/hir-ty/src/infer.rs @@ -21,7 +21,7 @@ use hir_def::{ body::Body, builtin_type::{BuiltinInt, BuiltinType, BuiltinUint}, data::{ConstData, StaticData}, - expr::{BindingAnnotation, ExprId, PatId}, + expr::{BindingAnnotation, ExprId, ExprOrPatId, PatId}, lang_item::LangItemTarget, layout::Integer, path::{path, Path}, @@ -34,7 +34,7 @@ use hir_expand::name::{name, Name}; use itertools::Either; use la_arena::ArenaMap; use rustc_hash::FxHashMap; -use stdx::{always, impl_from}; +use stdx::always; use crate::{ db::HirDatabase, fold_tys, fold_tys_and_consts, infer::coerce::CoerceMany, @@ -120,13 +120,6 @@ pub(crate) fn normalize(db: &dyn HirDatabase, owner: DefWithBodyId, ty: Ty) -> T table.resolve_completely(ty_with_vars) } -#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] -enum ExprOrPatId { - ExprId(ExprId), - PatId(PatId), -} -impl_from!(ExprId, PatId for ExprOrPatId); - /// Binding modes inferred for patterns. /// #[derive(Copy, Clone, Debug, Eq, PartialEq)] @@ -209,6 +202,7 @@ pub(crate) type InferResult = Result, TypeError>; pub enum InferenceDiagnostic { NoSuchField { expr: ExprId }, PrivateField { expr: ExprId, field: FieldId }, + PrivateAssocItem { id: ExprOrPatId, item: AssocItemId }, BreakOutsideOfLoop { expr: ExprId, is_break: bool }, MismatchedArgCount { call_expr: ExprId, expected: usize, found: usize }, } diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs index 4881e7c8fc7..a5dd0206764 100644 --- a/crates/hir-ty/src/infer/expr.rs +++ b/crates/hir-ty/src/infer/expr.rs @@ -1142,20 +1142,26 @@ impl<'a> InferenceContext<'a> { let traits_in_scope = self.resolver.traits_in_scope(self.db.upcast()); let resolved = method_resolution::lookup_method( - &canonicalized_receiver.value, self.db, + &canonicalized_receiver.value, self.trait_env.clone(), &traits_in_scope, VisibleFromModule::Filter(self.resolver.module()), method_name, ); let (receiver_ty, method_ty, substs) = match resolved { - Some((adjust, func)) => { + Some((adjust, func, visible)) => { let (ty, adjustments) = adjust.apply(&mut self.table, receiver_ty); let generics = generics(self.db.upcast(), func.into()); let substs = self.substs_for_method_call(generics, generic_args); self.write_expr_adj(receiver, adjustments); self.write_method_resolution(tgt_expr, func, substs.clone()); + if !visible { + self.push_diagnostic(InferenceDiagnostic::PrivateAssocItem { + id: tgt_expr.into(), + item: func.into(), + }) + } (ty, self.db.value_ty(func.into()), substs) } None => ( diff --git a/crates/hir-ty/src/infer/path.rs b/crates/hir-ty/src/infer/path.rs index dc645f840ee..8bd17c0f39f 100644 --- a/crates/hir-ty/src/infer/path.rs +++ b/crates/hir-ty/src/infer/path.rs @@ -14,7 +14,8 @@ use crate::{ consteval, method_resolution::{self, VisibleFromModule}, utils::generics, - Interner, Substitution, TraitRefExt, Ty, TyBuilder, TyExt, TyKind, ValueTyDefId, + InferenceDiagnostic, Interner, Substitution, TraitRefExt, Ty, TyBuilder, TyExt, TyKind, + ValueTyDefId, }; use super::{ExprOrPatId, InferenceContext, TraitRef}; @@ -279,20 +280,23 @@ impl<'a> InferenceContext<'a> { }; if visible { - Some((def, item, Some(substs))) + Some((def, item, Some(substs), true)) } else { if not_visible.is_none() { - not_visible = Some((def, item, Some(substs))); + not_visible = Some((def, item, Some(substs), false)); } None } }, ); let res = res.or(not_visible); - if let Some((_, item, Some(ref substs))) = res { + if let Some((_, item, Some(ref substs), visible)) = res { self.write_assoc_resolution(id, item, substs.clone()); + if !visible { + self.push_diagnostic(InferenceDiagnostic::PrivateAssocItem { id, item }) + } } - res.map(|(def, _, substs)| (def, substs)) + res.map(|(def, _, substs, _)| (def, substs)) } fn resolve_enum_variant_on_ty( diff --git a/crates/hir-ty/src/method_resolution.rs b/crates/hir-ty/src/method_resolution.rs index 2d162bc38e4..2f5fa3083c7 100644 --- a/crates/hir-ty/src/method_resolution.rs +++ b/crates/hir-ty/src/method_resolution.rs @@ -488,13 +488,13 @@ pub fn lang_names_for_bin_op(op: syntax::ast::BinaryOp) -> Option<(Name, Name)> /// Look up the method with the given name. pub(crate) fn lookup_method( - ty: &Canonical, db: &dyn HirDatabase, + ty: &Canonical, env: Arc, traits_in_scope: &FxHashSet, visible_from_module: VisibleFromModule, name: &Name, -) -> Option<(ReceiverAdjustments, FunctionId)> { +) -> Option<(ReceiverAdjustments, FunctionId, bool)> { let mut not_visible = None; let res = iterate_method_candidates( ty, @@ -505,9 +505,9 @@ pub(crate) fn lookup_method( Some(name), LookupMode::MethodCall, |adjustments, f, visible| match f { - AssocItemId::FunctionId(f) if visible => Some((adjustments, f)), + AssocItemId::FunctionId(f) if visible => Some((adjustments, f, true)), AssocItemId::FunctionId(f) if not_visible.is_none() => { - not_visible = Some((adjustments, f)); + not_visible = Some((adjustments, f, false)); None } _ => None, diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index baeb525effe..54d43fa8dc7 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -10,7 +10,7 @@ use hir_def::path::ModPath; use hir_expand::{name::Name, HirFileId, InFile}; use syntax::{ast, AstPtr, SyntaxNodePtr, TextRange}; -use crate::{Field, MacroKind, Type}; +use crate::{AssocItem, Field, MacroKind, Type}; macro_rules! diagnostics { ($($diag:ident,)*) => { @@ -41,6 +41,7 @@ diagnostics![ MissingMatchArms, MissingUnsafe, NoSuchField, + PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, @@ -122,6 +123,13 @@ pub struct NoSuchField { pub field: InFile>, } +#[derive(Debug)] +pub struct PrivateAssocItem { + pub expr_or_pat: + InFile, Either, AstPtr>>>, + pub item: AssocItem, +} + #[derive(Debug)] pub struct PrivateField { pub expr: InFile>, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index e8e623e7d61..86fd45e8246 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -41,7 +41,7 @@ use either::Either; use hir_def::{ adt::VariantData, body::{BodyDiagnostic, SyntheticSyntax}, - expr::{BindingAnnotation, LabelId, Pat, PatId}, + expr::{BindingAnnotation, ExprOrPatId, LabelId, Pat, PatId}, generics::{TypeOrConstParamData, TypeParamProvenance}, item_tree::ItemTreeNode, lang_item::LangItemTarget, @@ -85,9 +85,10 @@ pub use crate::{ diagnostics::{ AnyDiagnostic, BreakOutsideOfLoop, InactiveCode, IncorrectCase, InvalidDeriveTarget, MacroError, MalformedDerive, MismatchedArgCount, MissingFields, MissingMatchArms, - MissingUnsafe, NoSuchField, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, - UnimplementedBuiltinMacro, UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, - UnresolvedModule, UnresolvedProcMacro, + MissingUnsafe, NoSuchField, PrivateAssocItem, PrivateField, + ReplaceFilterMapNextWithFindMap, TypeMismatch, UnimplementedBuiltinMacro, + UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, UnresolvedModule, + UnresolvedProcMacro, }, has_source::HasSource, semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo, VisibleTraits}, @@ -1358,6 +1359,20 @@ impl DefWithBody { let field = field.into(); acc.push(PrivateField { expr, field }.into()) } + &hir_ty::InferenceDiagnostic::PrivateAssocItem { id, item } => { + let expr_or_pat = match id { + ExprOrPatId::ExprId(expr) => source_map + .expr_syntax(expr) + .expect("unexpected synthetic") + .map(Either::Left), + ExprOrPatId::PatId(pat) => source_map + .pat_syntax(pat) + .expect("unexpected synthetic") + .map(Either::Right), + }; + let item = item.into(); + acc.push(PrivateAssocItem { expr_or_pat, item }.into()) + } } } for (expr, mismatch) in infer.expr_type_mismatches() { diff --git a/crates/ide-diagnostics/src/handlers/private_assoc_item.rs b/crates/ide-diagnostics/src/handlers/private_assoc_item.rs new file mode 100644 index 00000000000..b363a516dd1 --- /dev/null +++ b/crates/ide-diagnostics/src/handlers/private_assoc_item.rs @@ -0,0 +1,124 @@ +use either::Either; + +use crate::{Diagnostic, DiagnosticsContext}; + +// Diagnostic: private-assoc-item +// +// This diagnostic is triggered if the referenced associated item is not visible from the current +// module. +pub(crate) fn private_assoc_item( + ctx: &DiagnosticsContext<'_>, + d: &hir::PrivateAssocItem, +) -> Diagnostic { + // FIXME: add quickfix + let name = match d.item.name(ctx.sema.db) { + Some(name) => format!("`{}` ", name), + None => String::new(), + }; + Diagnostic::new( + "private-assoc-item", + format!( + "{} {}is private", + match d.item { + hir::AssocItem::Function(_) => "function", + hir::AssocItem::Const(_) => "const", + hir::AssocItem::TypeAlias(_) => "type alias", + }, + name, + ), + ctx.sema + .diagnostics_display_range(d.expr_or_pat.clone().map(|it| match it { + Either::Left(it) => it.into(), + Either::Right(it) => match it { + Either::Left(it) => it.into(), + Either::Right(it) => it.into(), + }, + })) + .range, + ) +} + +#[cfg(test)] +mod tests { + use crate::tests::check_diagnostics; + + #[test] + fn private_method() { + check_diagnostics( + r#" +mod module { + pub struct Struct; + impl Struct { + fn method(&self) {} + } +} +fn main(s: module::Struct) { + s.method(); + //^^^^^^^^^^ error: function `method` is private +} +"#, + ); + } + + #[test] + fn private_func() { + check_diagnostics( + r#" +mod module { + pub struct Struct; + impl Struct { + fn func() {} + } +} +fn main() { + module::Struct::func(); + //^^^^^^^^^^^^^^^^^^^^ error: function `func` is private +} +"#, + ); + } + + #[test] + fn private_const() { + check_diagnostics( + r#" +mod module { + pub struct Struct; + impl Struct { + const CONST: u32 = 0; + } +} +fn main() { + module::Struct::CONST; + //^^^^^^^^^^^^^^^^^^^^^ error: const `CONST` is private +} +"#, + ); + } + + #[test] + fn private_but_shadowed_in_deref() { + check_diagnostics( + r#" +//- minicore: deref +mod module { + pub struct Struct { field: Inner } + pub struct Inner; + impl core::ops::Deref for Struct { + type Target = Inner; + fn deref(&self) -> &Inner { &self.field } + } + impl Struct { + fn method(&self) {} + } + impl Inner { + pub fn method(&self) {} + } +} +fn main(s: module::Struct) { + s.method(); +} +"#, + ); + } +} diff --git a/crates/ide-diagnostics/src/handlers/private_field.rs b/crates/ide-diagnostics/src/handlers/private_field.rs index 3db5eca07bf..e630ae36866 100644 --- a/crates/ide-diagnostics/src/handlers/private_field.rs +++ b/crates/ide-diagnostics/src/handlers/private_field.rs @@ -2,7 +2,7 @@ use crate::{Diagnostic, DiagnosticsContext}; // Diagnostic: private-field // -// This diagnostic is triggered if created structure does not have field provided in record. +// This diagnostic is triggered if the accessed field is not visible from the current module. pub(crate) fn private_field(ctx: &DiagnosticsContext<'_>, d: &hir::PrivateField) -> Diagnostic { // FIXME: add quickfix Diagnostic::new( @@ -33,6 +33,19 @@ fn main(s: module::Struct) { ); } + #[test] + fn private_tuple_field() { + check_diagnostics( + r#" +mod module { pub struct Struct(u32); } +fn main(s: module::Struct) { + s.0; + //^^^ error: field `0` of `Struct` is private +} +"#, + ); + } + #[test] fn private_but_shadowed_in_deref() { check_diagnostics( diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index 91555e01b9c..b0231b87540 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -37,6 +37,7 @@ mod handlers { pub(crate) mod missing_match_arms; pub(crate) mod missing_unsafe; pub(crate) mod no_such_field; + pub(crate) mod private_assoc_item; pub(crate) mod private_field; pub(crate) mod replace_filter_map_next_with_find_map; pub(crate) mod type_mismatch; @@ -255,6 +256,7 @@ pub fn diagnostics( AnyDiagnostic::MissingMatchArms(d) => handlers::missing_match_arms::missing_match_arms(&ctx, &d), AnyDiagnostic::MissingUnsafe(d) => handlers::missing_unsafe::missing_unsafe(&ctx, &d), AnyDiagnostic::NoSuchField(d) => handlers::no_such_field::no_such_field(&ctx, &d), + 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::TypeMismatch(d) => handlers::type_mismatch::type_mismatch(&ctx, &d),