Skip to content

Commit

Permalink
avoid overlapping privacy suggestion for single nested imports
Browse files Browse the repository at this point in the history
  • Loading branch information
bvanjoi committed Feb 16, 2024
1 parent ae9d7b0 commit 3c7465b
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 45 deletions.
69 changes: 45 additions & 24 deletions compiler/rustc_resolve/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {

pub(crate) fn lint_if_path_starts_with_module(
&mut self,
finalize: Option<Finalize>,
finalize: Option<Finalize<'_>>,
path: &[Segment],
second_binding: Option<NameBinding<'_>>,
) {
Expand Down Expand Up @@ -1694,7 +1694,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}

fn report_privacy_error(&mut self, privacy_error: &PrivacyError<'a>) {
let PrivacyError { ident, binding, outermost_res, parent_scope, dedup_span } =
let PrivacyError { ident, binding, outermost_res, parent_scope, imported, dedup_span } =
*privacy_error;

let res = binding.res();
Expand Down Expand Up @@ -1733,7 +1733,11 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
&import_suggestions,
Instead::Yes,
FoundUse::Yes,
DiagnosticMode::Import,
DiagnosticMode::Import {
single_nested: imported.is_some_and(|imported| {
matches!(imported.kind, ImportKind::Single { nested: true, .. })
}),
},
vec![],
"",
);
Expand Down Expand Up @@ -2659,7 +2663,10 @@ pub(crate) enum DiagnosticMode {
/// The binding is part of a pattern
Pattern,
/// The binding is part of a use statement
Import,
Import {
/// Is the format `use a::{b,c}`
single_nested: bool,
},
}

pub(crate) fn import_candidates(
Expand All @@ -2684,6 +2691,8 @@ pub(crate) fn import_candidates(
);
}

type PathString<'a> = (String, &'a str, Option<DefId>, &'a Option<String>, bool);

/// When an entity with a given name is not available in scope, we search for
/// entities with that name in all crates. This method allows outputting the
/// results of this search in a programmer-friendly way. If any entities are
Expand All @@ -2704,10 +2713,8 @@ fn show_candidates(
return false;
}

let mut accessible_path_strings: Vec<(String, &str, Option<DefId>, &Option<String>, bool)> =
Vec::new();
let mut inaccessible_path_strings: Vec<(String, &str, Option<DefId>, &Option<String>, bool)> =
Vec::new();
let mut accessible_path_strings: Vec<PathString<'_>> = Vec::new();
let mut inaccessible_path_strings: Vec<PathString<'_>> = Vec::new();

candidates.iter().for_each(|c| {
if c.accessible {
Expand Down Expand Up @@ -2769,6 +2776,15 @@ fn show_candidates(
err.note(note.clone());
}

let append_candidates = |msg: &mut String, accessible_path_strings: Vec<PathString<'_>>| {
msg.push(':');

for candidate in accessible_path_strings {
msg.push('\n');
msg.push_str(&candidate.0);
}
};

if let Some(span) = use_placement_span {
let (add_use, trailing) = match mode {
DiagnosticMode::Pattern => {
Expand All @@ -2780,7 +2796,7 @@ fn show_candidates(
);
return true;
}
DiagnosticMode::Import => ("", ""),
DiagnosticMode::Import { .. } => ("", ""),
DiagnosticMode::Normal => ("use ", ";\n"),
};
for candidate in &mut accessible_path_strings {
Expand All @@ -2797,13 +2813,22 @@ fn show_candidates(
format!("{add_use}{}{append}{trailing}{additional_newline}", &candidate.0);
}

err.span_suggestions_with_style(
span,
msg,
accessible_path_strings.into_iter().map(|a| a.0),
Applicability::MaybeIncorrect,
SuggestionStyle::ShowAlways,
);
match mode {
DiagnosticMode::Import { single_nested: true, .. } => {
append_candidates(&mut msg, accessible_path_strings);
err.span_help(span, msg);
}
_ => {
err.span_suggestions_with_style(
span,
msg,
accessible_path_strings.into_iter().map(|a| a.0),
Applicability::MaybeIncorrect,
SuggestionStyle::ShowAlways,
);
}
}

if let [first, .., last] = &path[..] {
let sp = first.ident.span.until(last.ident.span);
// Our suggestion is empty, so make sure the span is not empty (or we'd ICE).
Expand All @@ -2818,17 +2843,13 @@ fn show_candidates(
}
}
} else {
msg.push(':');

for candidate in accessible_path_strings {
msg.push('\n');
msg.push_str(&candidate.0);
}

append_candidates(&mut msg, accessible_path_strings);
err.help(msg);
}
true
} else if !(inaccessible_path_strings.is_empty() || matches!(mode, DiagnosticMode::Import)) {
} else if !(inaccessible_path_strings.is_empty()
|| matches!(mode, DiagnosticMode::Import { .. }))
{
let prefix = if let DiagnosticMode::Pattern = mode {
"you might have meant to match on "
} else {
Expand Down
19 changes: 10 additions & 9 deletions compiler/rustc_resolve/src/ident.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
mut ident: Ident,
ns: Namespace,
parent_scope: &ParentScope<'a>,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
ribs: &[Rib<'a>],
ignore_binding: Option<NameBinding<'a>>,
) -> Option<LexicalScopeBinding<'a>> {
Expand Down Expand Up @@ -370,7 +370,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
orig_ident: Ident,
scope_set: ScopeSet<'a>,
parent_scope: &ParentScope<'a>,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
force: bool,
ignore_binding: Option<NameBinding<'a>>,
) -> Result<NameBinding<'a>, Determinacy> {
Expand Down Expand Up @@ -711,7 +711,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
ident: Ident,
ns: Namespace,
parent_scope: &ParentScope<'a>,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
ignore_binding: Option<NameBinding<'a>>,
) -> Result<NameBinding<'a>, Determinacy> {
self.resolve_ident_in_module_ext(module, ident, ns, parent_scope, finalize, ignore_binding)
Expand All @@ -725,7 +725,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
mut ident: Ident,
ns: Namespace,
parent_scope: &ParentScope<'a>,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
ignore_binding: Option<NameBinding<'a>>,
) -> Result<NameBinding<'a>, (Determinacy, Weak)> {
let tmp_parent_scope;
Expand Down Expand Up @@ -763,7 +763,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
ident: Ident,
ns: Namespace,
parent_scope: &ParentScope<'a>,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
ignore_binding: Option<NameBinding<'a>>,
) -> Result<NameBinding<'a>, Determinacy> {
self.resolve_ident_in_module_unadjusted_ext(
Expand All @@ -788,7 +788,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
ns: Namespace,
parent_scope: &ParentScope<'a>,
restricted_shadowing: bool,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
// This binding should be ignored during in-module resolution, so that we don't get
// "self-confirming" import resolutions during import validation and checking.
ignore_binding: Option<NameBinding<'a>>,
Expand Down Expand Up @@ -857,7 +857,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
.into_iter()
.find_map(|binding| if binding == ignore_binding { None } else { binding });

if let Some(Finalize { path_span, report_private, .. }) = finalize {
if let Some(Finalize { path_span, report_private, imported, .. }) = finalize {
let Some(binding) = binding else {
return Err((Determined, Weak::No));
};
Expand All @@ -870,6 +870,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
dedup_span: path_span,
outermost_res: None,
parent_scope: *parent_scope,
imported,
});
} else {
return Err((Determined, Weak::No));
Expand Down Expand Up @@ -1332,7 +1333,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
path: &[Segment],
opt_ns: Option<Namespace>, // `None` indicates a module path in import
parent_scope: &ParentScope<'a>,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
ignore_binding: Option<NameBinding<'a>>,
) -> PathResult<'a> {
self.resolve_path_with_ribs(path, opt_ns, parent_scope, finalize, None, ignore_binding)
Expand All @@ -1343,7 +1344,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
path: &[Segment],
opt_ns: Option<Namespace>, // `None` indicates a module path in import
parent_scope: &ParentScope<'a>,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
ribs: Option<&PerNS<Vec<Rib<'a>>>>,
ignore_binding: Option<NameBinding<'a>>,
) -> PathResult<'a> {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_resolve/src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
&mut diag,
Some(err.span),
candidates,
DiagnosticMode::Import,
DiagnosticMode::Import { single_nested: false },
(source != target)
.then(|| format!(" as {target}"))
.as_deref()
Expand Down Expand Up @@ -857,7 +857,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
_ => None,
};
let prev_ambiguity_errors_len = self.ambiguity_errors.len();
let finalize = Finalize::with_root_span(import.root_id, import.span, import.root_span);
let finalize = Finalize::with_imported(import);

// We'll provide more context to the privacy errors later, up to `len`.
let privacy_errors_len = self.privacy_errors.len();
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1324,7 +1324,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
&mut self,
ident: Ident,
ns: Namespace,
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
ignore_binding: Option<NameBinding<'a>>,
) -> Option<LexicalScopeBinding<'a>> {
self.r.resolve_ident_in_lexical_scope(
Expand All @@ -1341,7 +1341,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
&mut self,
path: &[Segment],
opt_ns: Option<Namespace>, // `None` indicates a module path in import
finalize: Option<Finalize>,
finalize: Option<Finalize<'a>>,
) -> PathResult<'a> {
self.r.resolve_path_with_ribs(
path,
Expand Down Expand Up @@ -3725,7 +3725,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
qself: &Option<P<QSelf>>,
path: &[Segment],
source: PathSource<'ast>,
finalize: Finalize,
finalize: Finalize<'a>,
record_partial_res: RecordPartialRes,
) -> PartialRes {
let ns = source.namespace();
Expand Down Expand Up @@ -3982,7 +3982,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
primary_ns: Namespace,
span: Span,
defer_to_typeck: bool,
finalize: Finalize,
finalize: Finalize<'a>,
) -> Result<Option<PartialRes>, Spanned<ResolutionError<'a>>> {
let mut fin_res = None;

Expand Down Expand Up @@ -4024,7 +4024,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
qself: &Option<P<QSelf>>,
path: &[Segment],
ns: Namespace,
finalize: Finalize,
finalize: Finalize<'a>,
) -> Result<Option<PartialRes>, Spanned<ResolutionError<'a>>> {
debug!(
"resolve_qpath(qself={:?}, path={:?}, ns={:?}, finalize={:?})",
Expand Down
22 changes: 17 additions & 5 deletions compiler/rustc_resolve/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ struct PrivacyError<'a> {
dedup_span: Span,
outermost_res: Option<(Res, Ident)>,
parent_scope: ParentScope<'a>,
imported: Option<Import<'a>>,
}

#[derive(Debug)]
Expand Down Expand Up @@ -2164,7 +2165,7 @@ fn module_to_string(module: Module<'_>) -> Option<String> {
}

#[derive(Copy, Clone, Debug)]
struct Finalize {
struct Finalize<'a> {
/// Node ID for linting.
node_id: NodeId,
/// Span of the whole path or some its characteristic fragment.
Expand All @@ -2176,15 +2177,26 @@ struct Finalize {
/// Whether to report privacy errors or silently return "no resolution" for them,
/// similarly to speculative resolution.
report_private: bool,
imported: Option<Import<'a>>,
}

impl Finalize {
fn new(node_id: NodeId, path_span: Span) -> Finalize {
impl<'a> Finalize<'a> {
fn new(node_id: NodeId, path_span: Span) -> Finalize<'a> {
Finalize::with_root_span(node_id, path_span, path_span)
}

fn with_root_span(node_id: NodeId, path_span: Span, root_span: Span) -> Finalize {
Finalize { node_id, path_span, root_span, report_private: true }
fn with_root_span(node_id: NodeId, path_span: Span, root_span: Span) -> Finalize<'a> {
Finalize { node_id, path_span, root_span, report_private: true, imported: None }
}

fn with_imported(imported: Import<'a>) -> Finalize<'a> {
Finalize {
node_id: imported.root_id,
path_span: imported.span,
root_span: imported.root_span,
report_private: true,
imported: Some(imported),
}
}
}

Expand Down
14 changes: 14 additions & 0 deletions tests/ui/imports/issue-114884.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
mod mod1 {
pub trait TraitA {}
}

mod mod2 {
mod sub_mod {
use super::super::mod1::TraitA;
}
}

use mod2::{sub_mod::TraitA};
//~^ ERROR: module `sub_mod` is private

fn main() {}
21 changes: 21 additions & 0 deletions tests/ui/imports/issue-114884.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error[E0603]: module `sub_mod` is private
--> $DIR/issue-114884.rs:11:12
|
LL | use mod2::{sub_mod::TraitA};
| ^^^^^^^ private module
|
help: consider importing this trait instead:
mod1::TraitA
--> $DIR/issue-114884.rs:11:12
|
LL | use mod2::{sub_mod::TraitA};
| ^^^^^^^^^^^^^^^
note: the module `sub_mod` is defined here
--> $DIR/issue-114884.rs:6:5
|
LL | mod sub_mod {
| ^^^^^^^^^^^

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0603`.

0 comments on commit 3c7465b

Please sign in to comment.