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

Cleanup: HIR ty lowering: Consolidate the places that do assoc item probing & access checking #125978

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions compiler/rustc_hir_analysis/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ hir_analysis_assoc_item_constraints_not_allowed_here =
associated item constraints are not allowed here
.label = associated item constraint not allowed here

hir_analysis_assoc_item_is_private = {$kind} `{$name}` is private
.label = private {$kind}
.defined_here_label = the {$kind} is defined here

hir_analysis_assoc_item_not_found = associated {$assoc_kind} `{$assoc_name}` not found for `{$ty_param_name}`

hir_analysis_assoc_item_not_found_found_in_other_trait_label = there is {$identically_named ->
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ pub struct AssocKindMismatchWrapInBracesSugg {
pub hi: Span,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_assoc_item_is_private, code = E0624)]
pub struct AssocItemIsPrivate {
#[primary_span]
#[label]
pub span: Span,
pub kind: &'static str,
pub name: Ident,
#[label(hir_analysis_defined_here_label)]
pub defined_here_label: Span,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_assoc_item_not_found, code = E0220)]
pub struct AssocItemNotFound<'a> {
Expand Down
50 changes: 17 additions & 33 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,30 +292,15 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
)?
};

let (assoc_ident, def_scope) =
tcx.adjust_ident_and_get_scope(constraint.ident, candidate.def_id(), hir_ref_id);

// We have already adjusted the item name above, so compare with `.normalize_to_macros_2_0()`
// instead of calling `filter_by_name_and_kind` which would needlessly normalize the
// `assoc_ident` again and again.
let assoc_item = tcx
.associated_items(candidate.def_id())
.filter_by_name_unhygienic(assoc_ident.name)
.find(|i| i.kind == assoc_kind && i.ident(tcx).normalize_to_macros_2_0() == assoc_ident)
.expect("missing associated item");

if !assoc_item.visibility(tcx).is_accessible_from(def_scope, tcx) {
let reported = tcx
.dcx()
.struct_span_err(
constraint.span,
format!("{} `{}` is private", assoc_item.kind, constraint.ident),
)
.with_span_label(constraint.span, format!("private {}", assoc_item.kind))
.emit();
self.set_tainted_by_errors(reported);
}
tcx.check_stability(assoc_item.def_id, Some(hir_ref_id), constraint.span, None);
let assoc_item = self
.probe_assoc_item(
constraint.ident,
assoc_kind,
hir_ref_id,
constraint.span,
candidate.def_id(),
)
.expect("failed to find associated item");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entertaining the idea of moving this unwrap into probe_assoc_item for further dedup (2 occurrences) and rename the function to sth. like expect_and_check_assoc_item (that name is gross tho; suggestions welcome).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind that name, feel free to do this in a follow-up.


duplicates
.entry(assoc_item.def_id)
Expand Down Expand Up @@ -406,10 +391,9 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
// Create the generic arguments for the associated type or constant by joining the
// parent arguments (the arguments of the trait) and the own arguments (the ones of
// the associated item itself) and construct an alias type using them.
let alias_ty = candidate.map_bound(|trait_ref| {
let ident = Ident::new(assoc_item.name, constraint.ident.span);
let alias_term = candidate.map_bound(|trait_ref| {
let item_segment = hir::PathSegment {
ident,
ident: constraint.ident,
hir_id: constraint.hir_id,
res: Res::Err,
args: Some(constraint.gen_args),
Expand All @@ -428,15 +412,15 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
});

// Provide the resolved type of the associated constant to `type_of(AnonConst)`.
if let hir::AssocItemConstraintKind::Equality { term: hir::Term::Const(anon_const) } =
constraint.kind
{
let ty = alias_ty.map_bound(|ty| tcx.type_of(ty.def_id).instantiate(tcx, ty.args));
let ty = check_assoc_const_binding_type(tcx, assoc_ident, ty, constraint.hir_id);
if let Some(anon_const) = constraint.ct() {
davidtwco marked this conversation as resolved.
Show resolved Hide resolved
let ty = alias_term
.map_bound(|alias| tcx.type_of(alias.def_id).instantiate(tcx, alias.args));
let ty =
check_assoc_const_binding_type(tcx, constraint.ident, ty, constraint.hir_id);
tcx.feed_anon_const_type(anon_const.def_id, ty::EarlyBinder::bind(ty));
}

alias_ty
alias_term
};

match constraint.kind {
Expand Down
91 changes: 58 additions & 33 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1166,8 +1166,10 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
};

let trait_did = bound.def_id();
let assoc_ty_did = self.probe_assoc_ty(assoc_ident, hir_ref_id, span, trait_did).unwrap();
let ty = self.lower_assoc_ty(span, assoc_ty_did, assoc_segment, bound);
let assoc_ty = self
.probe_assoc_item(assoc_ident, ty::AssocKind::Type, hir_ref_id, span, trait_did)
.expect("failed to find associated type");
let ty = self.lower_assoc_ty(span, assoc_ty.def_id, assoc_segment, bound);

if let Some(variant_def_id) = variant_resolution {
tcx.node_span_lint(AMBIGUOUS_ASSOCIATED_ITEMS, hir_ref_id, span, |lint| {
Expand All @@ -1183,7 +1185,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
};

could_refer_to(DefKind::Variant, variant_def_id, "");
could_refer_to(DefKind::AssocTy, assoc_ty_did, " also");
could_refer_to(DefKind::AssocTy, assoc_ty.def_id, " also");

lint.span_suggestion(
span,
Expand All @@ -1193,7 +1195,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
);
});
}
Ok((ty, DefKind::AssocTy, assoc_ty_did))
Ok((ty, DefKind::AssocTy, assoc_ty.def_id))
}

fn probe_inherent_assoc_ty(
Expand All @@ -1220,7 +1222,11 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
let candidates: Vec<_> = tcx
.inherent_impls(adt_did)?
.iter()
.filter_map(|&impl_| Some((impl_, self.probe_assoc_ty_unchecked(name, block, impl_)?)))
.filter_map(|&impl_| {
let (item, scope) =
self.probe_assoc_item_unchecked(name, ty::AssocKind::Type, block, impl_)?;
Some((impl_, (item.def_id, scope)))
})
.collect();

if candidates.is_empty() {
Expand Down Expand Up @@ -1264,7 +1270,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
},
)?;

self.check_assoc_ty(assoc_item, name, def_scope, block, span);
self.check_assoc_item(assoc_item, name, def_scope, block, span);

// FIXME(fmease): Currently creating throwaway `parent_args` to please
// `lower_generic_args_of_assoc_item`. Modify the latter instead (or sth. similar) to
Expand Down Expand Up @@ -1351,50 +1357,69 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
}
}

fn probe_assoc_ty(&self, name: Ident, block: HirId, span: Span, scope: DefId) -> Option<DefId> {
let (item, def_scope) = self.probe_assoc_ty_unchecked(name, block, scope)?;
self.check_assoc_ty(item, name, def_scope, block, span);
/// Given name and kind search for the assoc item in the provided scope and check if it's accessible[^1].
///
/// [^1]: I.e., accessible in the provided scope wrt. visibility and stability.
fn probe_assoc_item(
&self,
ident: Ident,
kind: ty::AssocKind,
block: HirId,
span: Span,
scope: DefId,
) -> Option<ty::AssocItem> {
let (item, scope) = self.probe_assoc_item_unchecked(ident, kind, block, scope)?;
self.check_assoc_item(item.def_id, ident, scope, block, span);
Some(item)
}

fn probe_assoc_ty_unchecked(
/// Given name and kind search for the assoc item in the provided scope
/// *without* checking if it's accessible[^1].
///
/// [^1]: I.e., accessible in the provided scope wrt. visibility and stability.
fn probe_assoc_item_unchecked(
&self,
name: Ident,
ident: Ident,
kind: ty::AssocKind,
block: HirId,
scope: DefId,
) -> Option<(DefId, DefId)> {
) -> Option<(ty::AssocItem, /*scope*/ DefId)> {
let tcx = self.tcx();
let (ident, def_scope) = tcx.adjust_ident_and_get_scope(name, scope, block);

let (ident, def_scope) = tcx.adjust_ident_and_get_scope(ident, scope, block);
// We have already adjusted the item name above, so compare with `.normalize_to_macros_2_0()`
// instead of calling `filter_by_name_and_kind` which would needlessly normalize the
// `ident` again and again.
let item = tcx.associated_items(scope).in_definition_order().find(|i| {
i.kind.namespace() == Namespace::TypeNS
&& i.ident(tcx).normalize_to_macros_2_0() == ident
})?;
let item = tcx
.associated_items(scope)
.filter_by_name_unhygienic(ident.name)
.find(|i| i.kind == kind && i.ident(tcx).normalize_to_macros_2_0() == ident)?;

Some((item.def_id, def_scope))
Some((*item, def_scope))
}

fn check_assoc_ty(&self, item: DefId, name: Ident, def_scope: DefId, block: HirId, span: Span) {
/// Check if the given assoc item is accessible in the provided scope wrt. visibility and stability.
fn check_assoc_item(
&self,
item_def_id: DefId,
ident: Ident,
scope: DefId,
block: HirId,
span: Span,
) {
let tcx = self.tcx();
let kind = DefKind::AssocTy;

if !tcx.visibility(item).is_accessible_from(def_scope, tcx) {
let kind = tcx.def_kind_descr(kind, item);
let msg = format!("{kind} `{name}` is private");
let def_span = tcx.def_span(item);
let reported = tcx
.dcx()
.struct_span_err(span, msg)
.with_code(E0624)
.with_span_label(span, format!("private {kind}"))
.with_span_label(def_span, format!("{kind} defined here"))
.emit();

if !tcx.visibility(item_def_id).is_accessible_from(scope, tcx) {
let reported = tcx.dcx().emit_err(crate::errors::AssocItemIsPrivate {
span,
kind: tcx.def_descr(item_def_id),
name: ident,
defined_here_label: tcx.def_span(item_def_id),
});
self.set_tainted_by_errors(reported);
}
tcx.check_stability(item, Some(block), span, None);

tcx.check_stability(item_def_id, Some(block), span, None);
}

fn probe_traits_that_match_assoc_ty(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0624]: associated type `P` is private
--> $DIR/assoc-inherent-private.rs:10:10
|
LL | type P = ();
| ------ associated type defined here
| ------ the associated type is defined here
...
LL | type U = m::T::P;
| ^^^^^^^ private associated type
Expand All @@ -11,7 +11,7 @@ error[E0624]: associated type `P` is private
--> $DIR/assoc-inherent-private.rs:21:10
|
LL | pub(super) type P = bool;
| ----------------- associated type defined here
| ----------------- the associated type is defined here
...
LL | type V = n::n::T::P;
| ^^^^^^^^^^ private associated type
Expand Down
7 changes: 5 additions & 2 deletions tests/ui/traits/item-privacy.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -195,14 +195,17 @@ error[E0624]: associated type `A` is private
--> $DIR/item-privacy.rs:119:12
|
LL | type A = u8;
| ------ associated type defined here
| ------ the associated type is defined here
...
LL | let _: T::A;
| ^^^^ private associated type

error: associated type `A` is private
error[E0624]: associated type `A` is private
--> $DIR/item-privacy.rs:128:9
|
LL | type A = u8;
| ------ the associated type is defined here
...
LL | A = u8,
| ^^^^^^ private associated type

Expand Down
Loading