26

Github Fix diagnostic for cross crate private tuple struct constructors by luqma...

 3 years ago
source link: https://github.com/rust-lang/rust/pull/85068
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Copy link

Contributor

estebank left a comment

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?

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

estebank 23 days ago

Contributor

Why is this block removed?

luqmana 23 days ago

Author

Member

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:

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.

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

estebank 23 days ago

Contributor

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

luqmana 23 days ago

Author

Member

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.

estebank 22 days ago

Contributor

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.


report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK