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.

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

Improve message when attempting to instantiate tuple structs with private fields #65153

Merged
merged 2 commits into from Oct 10, 2019

Conversation

@da-x
Copy link
Member

da-x commented Oct 6, 2019

Fixes #58017, fixes #39703.

error[E0603]: tuple struct `Error` is private
  --> main.rs:22:16
   |
2  |     pub struct Error(usize, pub usize, usize);
   |                      -----             ----- field is private
   |                      |
   |                      field is private
...
22 |     let x = a::Error(3, 1, 2);
   |                ^^^^^
   |
   = note: a tuple struct constructor is private if any of its fields is private
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 6, 2019

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 6, 2019

@rust-highfive rust-highfive assigned petrochenkov and unassigned eddyb Oct 6, 2019
@da-x

This comment has been minimized.

Copy link
Member Author

da-x commented Oct 7, 2019

Should perhaps make the top line "tuple struct constructor Error is private". The type itself is not.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 8, 2019

Should perhaps make the top line "tuple struct constructor Error is private".

AFAIR, we just didn't want to say "constructor" in error messages because it wasn't an official enough term.
It may be time to change that, I agree that mentioning "constructor" would be clearer.
You can change "tuple/unit struct/variant" to "tuple/unit struct/variant constructor" in librustc/hir/def.rs in a separate PR if you are interested in this.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 8, 2019

Is it really necessary to point to the specific private field?
Perhaps we can point to all fields and make the "a tuple struct constructor is private if any of its fields is private" note more prominent, e.g. by turning it into a label?
(Cases with >1 fields in a tuple struct are rare, and the fields are usually all private if one of them is private.)

I would be great to avoid adding more state to Resolver which is already an uncontrollably growing garbage dump.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 8, 2019

We already have the field_names field in Resolver, it can be used for keeping the field spans as well.

@da-x

This comment has been minimized.

Copy link
Member Author

da-x commented Oct 8, 2019

It's not quite simple - even if we add to field_names, it wouldn't be useful considering our constructor's ctor_def_id because that map is keyed by the DefId of the type itself (used in LateResolutionVisitor::lookup_assoc_candidate). See that field_vis uses the constructor's DefId. Siding that, there's struct_constructors, however we actually need the inverse of it to reach our theoretically expanded field_names. Maybe adding a new map is unavoidable?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 8, 2019

even if we add to field_names, it wouldn't be useful considering our constructor's ctor_def_id because that map is keyed by the DefId of the type itself

The type's DefId can be obtained from the constructor's DefId using self.parent(ctor_def_id) (assuming self is Resolver).

@da-x

This comment has been minimized.

Copy link
Member Author

da-x commented Oct 8, 2019

Is this result good? I'm not sure yet how to get the span for just the fields. I've got the span for the struct itself.

error[E0603]: tuple struct `Error` is private
  --> main.rs:22:16
   |
2  |     pub struct Error(usize, pub usize, usize);
   |     ------------------------------------------ a tuple struct constructor is private if any of its fields is private
...
22 |     let x = a::Error(3, 1, 2);
   |                ^^^^^
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 8, 2019

how to get the span for just the fields

first_field_span.to(last_field_span) can be used for obtaining a span enclosing all fields.

da-x added a commit to da-x/rust that referenced this pull request Oct 9, 2019
- Expand original `field_names` field instead of adding one.
- Turn the original note into a label refering to the fields.

error[E0603]: tuple struct `Error` is private
  --> main.rs:22:16
   |
2  |     pub struct Error(usize, pub usize, usize);
   |                      ----------------------- a tuple struct constructor is private if any of its fields is private
...
22 |     let x = a::Error(3, 1, 2);
@da-x

This comment has been minimized.

Copy link
Member Author

da-x commented Oct 9, 2019

One limitation I noticed is that it doesn't work well when the types are in an external crate. It wasn't trivial to work with the changes in librustc_metadata to export individual field spans, or to export the span for the whole fields, and to connect that with the changes to librustc_resolve. Burned much time on that without getting a good result, so I left it out.

Another limitation is that going to the parent DefId in the case of the variant constructor is not leading to the correct DefId. Not sure how to solve that one either.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 9, 2019

One limitation I noticed is that it doesn't work well when the types are in an external crate.

The link below shows how to do that.

Another limitation is that going to the parent DefId in the case of the variant constructor is not leading to the correct DefId

The note is irrelevant to variants (they cannot have private fields) and shouldn't be reported for them.

What I wanted is a minimal patch addressing the issue and not spreading to other code not related to this specific message (i.e. not span manipulations in build_reduced_graph, etc):
https://github.com/petrochenkov/rust/tree/privctor2

@da-x

This comment has been minimized.

Copy link
Member Author

da-x commented Oct 9, 2019

Your version looks much better - didn't know of Spanned, respan, and that you can pass session at build_reduced_graph_for_external_crate_res. Should I cherry pick your changes or you open a separate PR?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 9, 2019

@da-x

Should I cherry pick your changes or you open a separate PR?

At your discretion.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 9, 2019

Oh crap, committed with a work email.
I'll re-create the branch soon.
UPD: Done.

@da-x da-x force-pushed the da-x:issue-58017 branch from 8615d2d to 48f8bed Oct 9, 2019
@da-x

This comment has been minimized.

Copy link
Member Author

da-x commented Oct 9, 2019

The changes are ready.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 9, 2019

@bors r+

Sorry for stealing the work.
(You can still do the "tuple struct constructor" bit though.)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

📌 Commit 48f8bed has been approved by petrochenkov

@da-x

This comment has been minimized.

Copy link
Member Author

da-x commented Oct 9, 2019

@petrochenkov No problem - it had been educational anyway. And yes, I have the other suggested change queued up after this one is merged :)

da-x added a commit to da-x/rust that referenced this pull request Oct 9, 2019
One reason is that constructors can be private while their types can be
public.

Idea credit to @petrochenkov, discussed at rust-lang#65153
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 10, 2019

⌛️ Testing commit 48f8bed with merge 898f36c...

bors added a commit that referenced this pull request Oct 10, 2019
Improve message when attempting to instantiate tuple structs with private fields

Fixes #58017, fixes #39703.

```
error[E0603]: tuple struct `Error` is private
  --> main.rs:22:16
   |
2  |     pub struct Error(usize, pub usize, usize);
   |                      -----             ----- field is private
   |                      |
   |                      field is private
...
22 |     let x = a::Error(3, 1, 2);
   |                ^^^^^
   |
   = note: a tuple struct constructor is private if any of its fields is private
```
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 10, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 898f36c to master...

@bors bors added the merged-by-bors label Oct 10, 2019
@bors bors merged commit 48f8bed into rust-lang:master Oct 10, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191009.28 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
da-x added a commit to da-x/rust that referenced this pull request Oct 11, 2019
The constructor is private, not the type.

Idea credit to @petrochenkov, discussed at rust-lang#65153
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
…nkov

resolve: fix error title regarding private constructors

One reason is that constructors can be private while their types can be
public.

Idea credit to @petrochenkov, discussed at rust-lang#65153
choller added a commit to choller/rust that referenced this pull request Oct 17, 2019
The constructor is private, not the type.

Idea credit to @petrochenkov, discussed at rust-lang#65153
XiangQingW added a commit to XiangQingW/rust that referenced this pull request Oct 21, 2019
The constructor is private, not the type.

Idea credit to @petrochenkov, discussed at rust-lang#65153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.