Skip to content
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

feat: Diagnose private assoc item accesses #13875

Merged
merged 1 commit into from Jan 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions crates/hir-def/src/expr.rs
Expand Up @@ -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,
Expand Down
12 changes: 3 additions & 9 deletions crates/hir-ty/src/infer.rs
Expand Up @@ -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},
Expand All @@ -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,
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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 },
}
Expand Down
10 changes: 8 additions & 2 deletions crates/hir-ty/src/infer/expr.rs
Expand Up @@ -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 => (
Expand Down
14 changes: 9 additions & 5 deletions crates/hir-ty/src/infer/path.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions crates/hir-ty/src/method_resolution.rs
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
10 changes: 9 additions & 1 deletion crates/hir/src/diagnostics.rs
Expand Up @@ -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,)*) => {
Expand Down Expand Up @@ -41,6 +41,7 @@ diagnostics![
MissingMatchArms,
MissingUnsafe,
NoSuchField,
PrivateAssocItem,
PrivateField,
ReplaceFilterMapNextWithFindMap,
TypeMismatch,
Expand Down Expand Up @@ -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>>,
Expand Down
23 changes: 19 additions & 4 deletions crates/hir/src/lib.rs
Expand Up @@ -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,
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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() {
Expand Down
124 changes: 124 additions & 0 deletions 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();
}
"#,
);
}
}
15 changes: 14 additions & 1 deletion crates/ide-diagnostics/src/handlers/private_field.rs
Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ide-diagnostics/src/lib.rs
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand Down