Auto merge of rust-lang#13875 - Veykril:private-field-diag, r=Veykril

Diagnose private assoc item accesses

Author: bors

crates/hir-def/src/expr.rs

@@ -36,6 +36,13 @@ pub(crate) fn dummy_expr_id() -> ExprId {
 
 pub type PatId = Idx<Pat>;
 
+#[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,

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.
 /// <https://doc.rust-lang.org/reference/patterns.html#binding-modes>
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -209,6 +202,7 @@ pub(crate) type InferResult<T> = Result<InferOk<T>, 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 },
 }

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 => (

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(

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<Ty>,
     db: &dyn HirDatabase,
+    ty: &Canonical<Ty>,
     env: Arc<TraitEnvironment>,
     traits_in_scope: &FxHashSet<TraitId>,
     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,

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<AstPtr<ast::RecordExprField>>,
 }
 
+#[derive(Debug)]
+pub struct PrivateAssocItem {
+    pub expr_or_pat:
+        InFile<Either<AstPtr<ast::Expr>, Either<AstPtr<ast::Pat>, AstPtr<ast::SelfParam>>>>,
+    pub item: AssocItem,
+}
+
 #[derive(Debug)]
 pub struct PrivateField {
     pub expr: InFile<AstPtr<ast::Expr>>,

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() {

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();
+}
+"#,
+        );
+    }
+}

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(

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),