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.

Sign up for GitHub

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

Fix diagnostic for cross crate private tuple struct constructors #85068

Merged
merged 2 commits into from
May 13, 2021
Merged

Fix diagnostic for cross crate private tuple struct constructors #85068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57fda67
Fix diagnostic for matching/creating x-crate re-exported tuple struct…
luqmana May 8, 2021
89300cd
Add tests.
luqmana May 8, 2021
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
13 changes: 12 additions & 1 deletion compiler/rustc_metadata/src/rmeta/decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use rustc_middle::middle::exported_symbols::{ExportedSymbol, SymbolExportLevel};
use rustc_middle::mir::interpret::{AllocDecodingSession, AllocDecodingState};
use rustc_middle::mir::{self, Body, Promoted};
use rustc_middle::ty::codec::TyDecoder;
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_middle::ty::{self, Ty, TyCtxt, Visibility};
use rustc_serialize::{opaque, Decodable, Decoder};
use rustc_session::Session;
use rustc_span::hygiene::ExpnDataDecodeMode;
Expand Down Expand Up @@ -1305,6 +1305,17 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
.collect()
}

fn get_struct_field_visibilities(&self, id: DefIndex) -> Vec<Visibility> {
self.root
.tables
.children
.get(self, id)
.unwrap_or_else(Lazy::empty)
.decode(self)
.map(|field_index| self.get_visibility(field_index))
.collect()
}

fn get_inherent_implementations_for_type(
&self,
tcx: TyCtxt<'tcx>,
Expand Down
18 changes: 16 additions & 2 deletions compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_ast::expand::allocator::AllocatorKind;
use rustc_data_structures::stable_map::FxHashMap;
use rustc_data_structures::svh::Svh;
use rustc_hir as hir;
use rustc_hir::def::DefKind;
use rustc_hir::def::{CtorKind, DefKind};
use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::definitions::{DefKey, DefPath, DefPathHash};
use rustc_middle::hir::exports::Export;
Expand All @@ -17,7 +17,7 @@ use rustc_middle::middle::cstore::{CrateSource, CrateStore, EncodedMetadata};
use rustc_middle::middle::exported_symbols::ExportedSymbol;
use rustc_middle::middle::stability::DeprecationEntry;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, TyCtxt};
use rustc_middle::ty::{self, TyCtxt, Visibility};
use rustc_session::utils::NativeLibKind;
use rustc_session::{CrateDisambiguator, Session};
use rustc_span::source_map::{Span, Spanned};
Expand Down Expand Up @@ -392,6 +392,20 @@ impl CStore {
self.get_crate_data(def.krate).get_struct_field_names(def.index, sess)
}

pub fn struct_field_visibilities_untracked(&self, def: DefId) -> Vec<Visibility> {
self.get_crate_data(def.krate).get_struct_field_visibilities(def.index)
}

pub fn ctor_def_id_and_kind_untracked(&self, def: DefId) -> Option<(DefId, CtorKind)> {
self.get_crate_data(def.krate).get_ctor_def_id(def.index).map(|ctor_def_id| {
(ctor_def_id, self.get_crate_data(def.krate).get_ctor_kind(def.index))
})
}

pub fn visibility_untracked(&self, def: DefId) -> Visibility {
self.get_crate_data(def.krate).get_visibility(def.index)
}

pub fn item_children_untracked(
&self,
def_id: DefId,
Expand Down
21 changes: 14 additions & 7 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,20 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
// Record some extra data for better diagnostics.
let cstore = self.r.cstore();
match res {
Res::Def(DefKind::Struct | DefKind::Union, def_id) => {
Res::Def(DefKind::Struct, def_id) => {
let field_names = cstore.struct_field_names_untracked(def_id, self.r.session);
let ctor = cstore.ctor_def_id_and_kind_untracked(def_id);
if let Some((ctor_def_id, ctor_kind)) = ctor {
let ctor_res = Res::Def(DefKind::Ctor(CtorOf::Struct, ctor_kind), ctor_def_id);
let ctor_vis = cstore.visibility_untracked(ctor_def_id);
let field_visibilities = cstore.struct_field_visibilities_untracked(def_id);
self.r
.struct_constructors
.insert(def_id, (ctor_res, ctor_vis, field_visibilities));
}
self.insert_field_names(def_id, field_names);
}
Res::Def(DefKind::Union, def_id) => {
let field_names = cstore.struct_field_names_untracked(def_id, self.r.session);
self.insert_field_names(def_id, field_names);
}
Expand All @@ -1007,12 +1020,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
self.r.has_self.insert(def_id);
}
}
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, vec![]));
}
}
Comment on lines -1010 to -1015
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this block removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This match case would only get hit for non-re-exported structs' constructors which is why we would miss the cross-crate case to begin with. So for a given newtype constructor we would sometimes hit both the DefKind::Ctor and DefKind::Struct arms or only hit the DefKind::Struct arm. It is the latter case where we didn't have the good diagnostics.

callback here collects the Exports which end up processed by the method build_reduced_graph_for_external_crate_res above:

rust/compiler/rustc_metadata/src/rmeta/decoder.rs

Lines 1084 to 1095 in 2bafe96

callback(Export { res, ident, vis, span });
// For non-re-export structs and variants add their constructors to children.
// Re-export lists automatically contain constructors when necessary.
match kind {
DefKind::Struct => {
if let Some(ctor_def_id) = self.get_ctor_def_id(child_index) {
let ctor_kind = self.get_ctor_kind(child_index);
let ctor_res =
Res::Def(DefKind::Ctor(CtorOf::Struct, ctor_kind), ctor_def_id);
let vis = self.get_visibility(ctor_def_id.index);
callback(Export { res: ctor_res, vis, ident, span });
}

So my PR just handles collecting struct_constructors for diagnostics always in the DefKind::Struct arm instead of relying on the Ctor arm which isn't always present.

_ => {}
}
}
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/issues/auxiliary/issue-75907.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,15 @@ pub struct Bar(pub u8, u8, u8);
pub fn make_bar() -> Bar {
Bar(1, 12, 10)
}

mod inner {
pub struct Foo(u8, pub u8, u8);

impl Foo {
pub fn new() -> Foo {
Foo(1, 12, 10)
}
}
}

pub use inner::Foo;
5 changes: 4 additions & 1 deletion src/test/ui/issues/issue-75907_b.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@

extern crate issue_75907 as a;

use a::{make_bar, Bar};
use a::{make_bar, Bar, Foo};

fn main() {
let Bar(x, y, z) = make_bar();
//~^ ERROR cannot match against a tuple struct which contains private fields

let Foo(x, y, z) = Foo::new();
//~^ ERROR cannot match against a tuple struct which contains private fields
}
26 changes: 24 additions & 2 deletions src/test/ui/issues/issue-75907_b.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,30 @@ error[E0532]: cannot match against a tuple struct which contains private fields
--> $DIR/issue-75907_b.rs:9:9
|
LL | let Bar(x, y, z) = make_bar();
| ^^^ constructor is not visible here due to private fields
| ^^^
|
note: constructor is not visible here due to private fields
--> $DIR/issue-75907_b.rs:9:16
|
LL | let Bar(x, y, z) = make_bar();
| ^ ^ private field
| |
| private field
Comment on lines +5 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

While looking at this again, I think we could make this slightly different, but it is not introduced by you:

Suggested change
| ^^^
|
note: constructor is not visible here due to private fields
--> $DIR/issue-75907_b.rs:9:16
|
LL | let Bar(x, y, z) = make_bar();
| ^ ^ private field
| |
| private field
| ^^^ - - private field
| | |
| | private field
| constructor not visible here due to private fields


error[E0532]: cannot match against a tuple struct which contains private fields
--> $DIR/issue-75907_b.rs:12:9
|
LL | let Foo(x, y, z) = Foo::new();
| ^^^
|
note: constructor is not visible here due to private fields
--> $DIR/issue-75907_b.rs:12:13
|
LL | let Foo(x, y, z) = Foo::new();
| ^ ^ private field
| |
| private field

error: aborting due to previous error
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0532`.
2 changes: 1 addition & 1 deletion src/test/ui/rfc-2008-non-exhaustive/struct.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0423]: cannot initialize a tuple struct which contains private fields
--> $DIR/struct.rs:20:14
|
LL | let ts = TupleStruct(640, 480);
| ^^^^^^^^^^^ constructor is not visible here due to private fields
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems at first sight that the removed block might be responsible for the removal of this span_label.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason we actually had this span_label before was because this was one of the cases handled by the DefKind::Ctor arm I removed above which you can see just always passed in an empty vec for the field visibilities. Now that we pass in the correct value there, not printing this message is actually correct since none of the fields are actually private! The reason for the error is because of #[non_exhaustive] which unfortunately isn't handled very well right now. I can hopefully get to improving the non-exhaustive errors and diagnostics.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case we should also change the error message.

I would like us to keep the constructor is not visible here due to private fields label regardless. If you look at the other output it is missing as well. Don't get me wrong, the new note is fantastic, but missing primary labels are a potential problem: some people have trained themselves to not read the main message, in other cases tools that display tooltips in users' code rely on the primary label instead of the message because it is usually better in context. I made a mockup on the other test output of what I'd like to see in the future. I'd be inclined to merge this PR if you don't intend on continuing work, but I'd like a ticket open for the follow up tweaks.

| ^^^^^^^^^^^

error[E0423]: expected value, found struct `UnitStruct`
--> $DIR/struct.rs:29:14
Expand Down