Fix diagnostic for cross crate private tuple struct constructors #85068

luqmana · 2021-05-08T08:25:48Z

There was already some limited support for certain cross-crate scenarios but that didn't handle a tuple struct rexported from an inner module for example (e.g. the NonZero* types as seen in #85049).

➜  cat bug.rs
fn main() {
    let _x = std::num::NonZeroU32(12);
    let n = std::num::NonZeroU32::new(1).unwrap();
    match n {
        std::num::NonZeroU32(i) => {},
    }
}

Before:

➜  rustc +nightly bug.rs
error[E0423]: expected function, tuple struct or tuple variant, found struct `std::num::NonZeroU32`
   --> bug.rs:2:14
    |
2   |       let _x = std::num::NonZeroU32(12);
    |                ^^^^^^^^^^^^^^^^^^^^^^^^ help: use struct literal syntax instead: `std::num::NonZeroU32 { 0: val }`
    | 
   ::: /home/luqman/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/nonzero.rs:148:1
[snip]
error[E0532]: expected tuple struct or tuple variant, found struct `std::num::NonZeroU32`
   --> bug.rs:5:9
    |
5   |           std::num::NonZeroU32(i) => {},
    |           ^^^^^^^^^^^^^^^^^^^^^^^ help: use struct pattern syntax instead: `std::num::NonZeroU32 { 0 }`
    | 
   ::: /home/luqman/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/nonzero.rs:148:1
[snip]

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0423, E0532.
For more information about an error, try `rustc --explain E0423`.

After:

➜  /rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc bug.rs
error[E0423]: cannot initialize a tuple struct which contains private fields
   --> bug.rs:2:14
    |
2   |     let _x = std::num::NonZeroU32(12);
    |              ^^^^^^^^^^^^^^^^^^^^
    |
note: constructor is not visible here due to private fields
   --> /rust/library/core/src/num/nonzero.rs:148:1
[snip]
error[E0532]: cannot match against a tuple struct which contains private fields
 --> bug.rs:5:9
  |
5 |         std::num::NonZeroU32(i) => {},
  |         ^^^^^^^^^^^^^^^^^^^^
  |
note: constructor is not visible here due to private fields
 --> bug.rs:5:30
  |
5 |         std::num::NonZeroU32(i) => {},
  |                              ^ private field

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0423, E0532.
For more information about an error, try `rustc --explain E0423`.

One question is if we should only collect the needed info for the cross-crate case after encountering an error instead of always doing it. Perf run perhaps to gauge the impact.

…s with private fields. The more helpful diagnostic already existed but wasn't working if the struct in question was a re-export from a different crate.

rust-highfive · 2021-05-08T08:25:51Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

estebank

I have one nitpick but I like it! Could you take a look to see if you can easily recover the span label that we are now missing in the last test?

estebank · 2021-05-11T17:54:01Z

compiler/rustc_resolve/src/build_reduced_graph.rs

-            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![]));
-                }
-            }


Why is this block removed?

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.

estebank · 2021-05-11T17:54:29Z

src/test/ui/rfc-2008-non-exhaustive/struct.stderr

@@ -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


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

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.

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.

estebank · 2021-05-12T15:19:30Z

src/test/ui/rfc-2008-non-exhaustive/struct.stderr

@@ -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


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.

estebank · 2021-05-12T15:22:23Z

src/test/ui/issues/issue-75907_b.stderr

+   |         ^^^
+   |
+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


estebank · 2021-05-12T15:22:36Z

@bors r+

bors · 2021-05-12T15:22:37Z

📌 Commit 89300cd has been approved by estebank

…laumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#85068 (Fix diagnostic for cross crate private tuple struct constructors) - rust-lang#85175 (Rustdoc cleanup) - rust-lang#85177 (add BITS associated constant to core::num::Wrapping) - rust-lang#85240 (Don't suggest adding `'static` lifetime to arguments) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

luqmana added 2 commits May 8, 2021 00:33

Fix diagnostic for matching/creating x-crate re-exported tuple struct…

57fda67

…s with private fields. The more helpful diagnostic already existed but wasn't working if the struct in question was a re-export from a different crate.

Add tests.

89300cd

rust-highfive assigned estebank May 8, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2021

estebank approved these changes May 11, 2021

View reviewed changes

estebank reviewed May 12, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2021

GuillaumeGomez mentioned this pull request May 13, 2021

Rollup of 4 pull requests #85258

Merged

bors merged commit 3db335b into rust-lang:master May 13, 2021

rustbot added this to the 1.54.0 milestone May 13, 2021

luqmana deleted the 78708-xcrate-diag branch May 13, 2021 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix diagnostic for cross crate private tuple struct constructors #85068

Fix diagnostic for cross crate private tuple struct constructors #85068

luqmana commented May 8, 2021 •

edited

Loading

rust-highfive commented May 8, 2021

estebank left a comment

estebank May 11, 2021

luqmana May 11, 2021

estebank May 11, 2021

luqmana May 11, 2021

estebank May 12, 2021

estebank May 12, 2021

estebank May 12, 2021

estebank commented May 12, 2021

bors commented May 12, 2021

	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 });
	}

Fix diagnostic for cross crate private tuple struct constructors #85068

Fix diagnostic for cross crate private tuple struct constructors #85068

Conversation

luqmana commented May 8, 2021 • edited Loading

rust-highfive commented May 8, 2021

estebank left a comment

Choose a reason for hiding this comment

estebank May 11, 2021

Choose a reason for hiding this comment

luqmana May 11, 2021

Choose a reason for hiding this comment

estebank May 11, 2021

Choose a reason for hiding this comment

luqmana May 11, 2021

Choose a reason for hiding this comment

estebank May 12, 2021

Choose a reason for hiding this comment

estebank May 12, 2021

Choose a reason for hiding this comment

estebank May 12, 2021

Choose a reason for hiding this comment

estebank commented May 12, 2021

bors commented May 12, 2021

luqmana commented May 8, 2021 •

edited

Loading