bjoernager/rust
bjoernager/rust
1
Fork 0
Code Issues Pull requests Activity

Auto merge of #76499 - guswynn:priv_des, r=petrochenkov

Browse source 
Give better diagnostic when using a private tuple struct constructor

Fixes #75907

Some notes:
1. This required some deep changes, including removing a Copy impl for PatKind. If some tests fail, I would still appreciate review on the overall approach
2. this only works with basic patterns (no wildcards for example), and fails if there is any problems getting the visibility of the fields (i am not sure what the failure that can happen in resolve_visibility_speculative, but we check the length of our fields in both cases against each other, so if anything goes wrong, we fall back to the worse error. This could be extended to more patterns
3. this does not yet deal with #75906, but I believe it will be similar
4. let me know if you want more tests
5. doesn't yet at the suggestion that `@yoshuawuyts` suggested at the end of their issue, but that could be added relatively easily (i believe)
This commit is contained in:
bors 2020-09-11 20:01:31 +00:00
commit bc57bd8c7e
9 changed files with 152 additions and 29 deletions
Download patch file Download diff file Expand all files Collapse all files

17
compiler/rustc_resolve/src/build_reduced_graph.rs
View file

@ -796,23 +796,26 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
vis
};
let mut ret_fields = Vec::with_capacity(vdata.fields().len());
for field in vdata.fields() {
// NOTE: The field may be an expansion placeholder, but expansion sets
// correct visibilities for unnamed field placeholders specifically, so the
// constructor visibility should still be determined correctly.
if let Ok(field_vis) = self.resolve_visibility_speculative(&field.vis, true)
{
if ctor_vis.is_at_least(field_vis, &*self.r) {
ctor_vis = field_vis;
}
let field_vis = self
.resolve_visibility_speculative(&field.vis, true)
.unwrap_or(ty::Visibility::Public);
if ctor_vis.is_at_least(field_vis, &*self.r) {
ctor_vis = field_vis;
}
ret_fields.push(field_vis);
}
let ctor_res = Res::Def(
DefKind::Ctor(CtorOf::Struct, CtorKind::from_ast(vdata)),
self.r.local_def_id(ctor_node_id).to_def_id(),
);
self.r.define(parent, ident, ValueNS, (ctor_res, ctor_vis, sp, expansion));
self.r.struct_constructors.insert(def_id, (ctor_res, ctor_vis));
self.r.struct_constructors.insert(def_id, (ctor_res, ctor_vis, ret_fields));
}
}
@ -964,7 +967,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
Res::Def(DefKind::Ctor(CtorOf::Struct, ..), def_id) => {
let parent = cstore.def_key(def_id).parent;
if let Some(struct_def_id) = parent.map(|index| DefId { index, ..def_id }) {
self.r.struct_constructors.insert(struct_def_id, (res, vis));
self.r.struct_constructors.insert(struct_def_id, (res, vis, vec![]));
}
}
_ => {}

30
compiler/rustc_resolve/src/late.rs
View file

@ -188,7 +188,7 @@ crate enum PathSource<'a> {
// Paths in struct expressions and patterns `Path { .. }`.
Struct,
// Paths in tuple struct patterns `Path(..)`.
TupleStruct(Span),
TupleStruct(Span, &'a [Span]),
// `m::A::B` in `<T as m::A>::B::C`.
TraitItem(Namespace),
}
@ -197,7 +197,7 @@ impl<'a> PathSource<'a> {
fn namespace(self) -> Namespace {
match self {
PathSource::Type | PathSource::Trait(_) | PathSource::Struct => TypeNS,
PathSource::Expr(..) | PathSource::Pat | PathSource::TupleStruct(_) => ValueNS,
PathSource::Expr(..) | PathSource::Pat | PathSource::TupleStruct(..) => ValueNS,
PathSource::TraitItem(ns) => ns,
}
}
@ -208,7 +208,7 @@ impl<'a> PathSource<'a> {
| PathSource::Expr(..)
| PathSource::Pat
| PathSource::Struct
| PathSource::TupleStruct(_) => true,
| PathSource::TupleStruct(..) => true,
PathSource::Trait(_) | PathSource::TraitItem(..) => false,
}
}
@ -219,7 +219,7 @@ impl<'a> PathSource<'a> {
PathSource::Trait(_) => "trait",
PathSource::Pat => "unit struct, unit variant or constant",
PathSource::Struct => "struct, variant or union type",
PathSource::TupleStruct(_) => "tuple struct or tuple variant",
PathSource::TupleStruct(..) => "tuple struct or tuple variant",
PathSource::TraitItem(ns) => match ns {
TypeNS => "associated type",
ValueNS => "method or associated constant",
@ -305,7 +305,7 @@ impl<'a> PathSource<'a> {
| Res::SelfCtor(..) => true,
_ => false,
},
PathSource::TupleStruct(_) => match res {
PathSource::TupleStruct(..) => match res {
Res::Def(DefKind::Ctor(_, CtorKind::Fn), _) | Res::SelfCtor(..) => true,
_ => false,
},
@ -340,8 +340,8 @@ impl<'a> PathSource<'a> {
(PathSource::Struct, false) => error_code!(E0422),
(PathSource::Expr(..), true) => error_code!(E0423),
(PathSource::Expr(..), false) => error_code!(E0425),
(PathSource::Pat | PathSource::TupleStruct(_), true) => error_code!(E0532),
(PathSource::Pat | PathSource::TupleStruct(_), false) => error_code!(E0531),
(PathSource::Pat | PathSource::TupleStruct(..), true) => error_code!(E0532),
(PathSource::Pat | PathSource::TupleStruct(..), false) => error_code!(E0531),
(PathSource::TraitItem(..), true) => error_code!(E0575),
(PathSource::TraitItem(..), false) => error_code!(E0576),
}
@ -411,7 +411,7 @@ struct LateResolutionVisitor<'a, 'b, 'ast> {
}
/// Walks the whole crate in DFS order, visiting each item, resolving names as it goes.
impl<'a, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
fn visit_item(&mut self, item: &'ast Item) {
let prev = replace(&mut self.diagnostic_metadata.current_item, Some(item));
// Always report errors in items we just entered.
@ -659,7 +659,7 @@ impl<'a, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
}
}
impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
fn new(resolver: &'b mut Resolver<'a>) -> LateResolutionVisitor<'a, 'b, 'ast> {
// During late resolution we only track the module component of the parent scope,
// although it may be useful to track other components as well for diagnostics.
@ -1539,8 +1539,16 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
.unwrap_or_else(|| self.fresh_binding(ident, pat.id, pat_src, bindings));
self.r.record_partial_res(pat.id, PartialRes::new(res));
}
PatKind::TupleStruct(ref path, ..) => {
self.smart_resolve_path(pat.id, None, path, PathSource::TupleStruct(pat.span));
PatKind::TupleStruct(ref path, ref sub_patterns) => {
self.smart_resolve_path(
pat.id,
None,
path,
PathSource::TupleStruct(
pat.span,
self.r.arenas.alloc_pattern_spans(sub_patterns.iter().map(|p| p.span)),
),
);
}
PatKind::Path(ref qself, ref path) => {
self.smart_resolve_path(pat.id, qself.as_ref(), path, PathSource::Pat);

55
compiler/rustc_resolve/src/late/diagnostics.rs
View file

@ -90,7 +90,7 @@ fn import_candidate_to_enum_paths(suggestion: &ImportSuggestion) -> (String, Str
(variant_path_string, enum_path_string)
}
impl<'a> LateResolutionVisitor<'a, '_, '_> {
impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
fn def_span(&self, def_id: DefId) -> Option<Span> {
match def_id.krate {
LOCAL_CRATE => self.r.opt_span(def_id),
@ -623,12 +623,12 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
);
}
}
PathSource::Expr(_) | PathSource::TupleStruct(_) | PathSource::Pat => {
PathSource::Expr(_) | PathSource::TupleStruct(..) | PathSource::Pat => {
let span = match &source {
PathSource::Expr(Some(Expr {
span, kind: ExprKind::Call(_, _), ..
}))
| PathSource::TupleStruct(span) => {
| PathSource::TupleStruct(span, _) => {
// We want the main underline to cover the suggested code as well for
// cleaner output.
err.set_span(*span);
@ -640,7 +640,7 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
err.span_label(span, &format!("`{}` defined here", path_str));
}
let (tail, descr, applicability) = match source {
PathSource::Pat | PathSource::TupleStruct(_) => {
PathSource::Pat | PathSource::TupleStruct(..) => {
("", "pattern", Applicability::MachineApplicable)
}
_ => (": val", "literal", Applicability::HasPlaceholders),
@ -705,7 +705,7 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
}
(
Res::Def(DefKind::Enum, def_id),
PathSource::TupleStruct(_) | PathSource::Expr(..),
PathSource::TupleStruct(..) | PathSource::Expr(..),
) => {
if self
.diagnostic_metadata
@ -745,15 +745,50 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
}
}
(Res::Def(DefKind::Struct, def_id), _) if ns == ValueNS => {
if let Some((ctor_def, ctor_vis)) = self.r.struct_constructors.get(&def_id).cloned()
if let Some((ctor_def, ctor_vis, fields)) =
self.r.struct_constructors.get(&def_id).cloned()
{
let accessible_ctor =
self.r.is_accessible_from(ctor_vis, self.parent_scope.module);
if is_expected(ctor_def) && !accessible_ctor {
err.span_label(
span,
"constructor is not visible here due to private fields".to_string(),
);
let mut better_diag = false;
if let PathSource::TupleStruct(_, pattern_spans) = source {
if pattern_spans.len() > 0 && fields.len() == pattern_spans.len() {
let non_visible_spans: Vec<Span> = fields
.iter()
.zip(pattern_spans.iter())
.filter_map(|(vis, span)| {
match self
.r
.is_accessible_from(*vis, self.parent_scope.module)
{
true => None,
false => Some(*span),
}
})
.collect();
// Extra check to be sure
if non_visible_spans.len() > 0 {
let mut m: rustc_span::MultiSpan =
non_visible_spans.clone().into();
non_visible_spans.into_iter().for_each(|s| {
m.push_span_label(s, "private field".to_string())
});
err.span_note(
m,
"constructor is not visible here due to private fields",
);
better_diag = true;
}
}
}
if !better_diag {
err.span_label(
span,
"constructor is not visible here due to private fields".to_string(),
);
}
}
} else {
bad_struct_syntax_suggestion(def_id);

7
compiler/rustc_resolve/src/lib.rs
View file

@ -1005,7 +1005,8 @@ pub struct Resolver<'a> {
/// Table for mapping struct IDs into struct constructor IDs,
/// it's not used during normal resolution, only for better error reporting.
struct_constructors: DefIdMap<(Res, ty::Visibility)>,
/// Also includes of list of each fields visibility
struct_constructors: DefIdMap<(Res, ty::Visibility, Vec<ty::Visibility>)>,
/// Features enabled for this crate.
active_features: FxHashSet<Symbol>,
@ -1042,6 +1043,7 @@ pub struct ResolverArenas<'a> {
name_resolutions: TypedArena<RefCell<NameResolution<'a>>>,
macro_rules_bindings: TypedArena<MacroRulesBinding<'a>>,
ast_paths: TypedArena<ast::Path>,
pattern_spans: TypedArena<Span>,
}
impl<'a> ResolverArenas<'a> {
@ -1073,6 +1075,9 @@ impl<'a> ResolverArenas<'a> {
fn alloc_ast_paths(&'a self, paths: &[ast::Path]) -> &'a [ast::Path] {
self.ast_paths.alloc_from_iter(paths.iter().cloned())
}
fn alloc_pattern_spans(&'a self, spans: impl Iterator<Item = Span>) -> &'a [Span] {
self.pattern_spans.alloc_from_iter(spans)
}
}
impl<'a> AsMut<Resolver<'a>> for Resolver<'a> {

5
src/test/ui/issues/auxiliary/issue-75907.rs Normal file
View file

@ -0,0 +1,5 @@
pub struct Bar(pub u8, u8, u8);
pub fn make_bar() -> Bar {
Bar(1, 12, 10)
}

18
src/test/ui/issues/issue-75907.rs Normal file
View file

@ -0,0 +1,18 @@
// Test for for diagnostic improvement issue #75907
mod foo {
pub(crate) struct Foo(u8);
pub(crate) struct Bar(pub u8, u8, Foo);
pub(crate) fn make_bar() -> Bar {
Bar(1, 12, Foo(10))
}
}
use foo::{make_bar, Bar, Foo};
fn main() {
let Bar(x, y, Foo(z)) = make_bar();
//~^ ERROR expected tuple struct
//~| ERROR expected tuple struct
}

29
src/test/ui/issues/issue-75907.stderr Normal file
View file

@ -0,0 +1,29 @@
error[E0532]: expected tuple struct or tuple variant, found struct `Bar`
--> $DIR/issue-75907.rs:15:9
|
LL | let Bar(x, y, Foo(z)) = make_bar();
| ^^^
|
note: constructor is not visible here due to private fields
--> $DIR/issue-75907.rs:15:16
|
LL | let Bar(x, y, Foo(z)) = make_bar();
| ^ ^^^^^^ private field
| |
| private field
error[E0532]: expected tuple struct or tuple variant, found struct `Foo`
--> $DIR/issue-75907.rs:15:19
|
LL | let Bar(x, y, Foo(z)) = make_bar();
| ^^^
|
note: constructor is not visible here due to private fields
--> $DIR/issue-75907.rs:15:23
|
LL | let Bar(x, y, Foo(z)) = make_bar();
| ^ private field
error: aborting due to 2 previous errors
For more information about this error, try `rustc --explain E0532`.

11
src/test/ui/issues/issue-75907_b.rs Normal file
View file

@ -0,0 +1,11 @@
// Test for for diagnostic improvement issue #75907, extern crate
// aux-build:issue-75907.rs
extern crate issue_75907 as a;
use a::{make_bar, Bar};
fn main() {
let Bar(x, y, z) = make_bar();
//~^ ERROR expected tuple struct
}

9
src/test/ui/issues/issue-75907_b.stderr Normal file
View file

@ -0,0 +1,9 @@
error[E0532]: expected tuple struct or tuple variant, found struct `Bar`
--> $DIR/issue-75907_b.rs:9:9
|
LL | let Bar(x, y, z) = make_bar();
| ^^^ constructor is not visible here due to private fields
error: aborting due to previous error
For more information about this error, try `rustc --explain E0532`.