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

[diagnostics] Attempting to initialize a private field in a public tuple struct yields confusing error message #75906

Closed
yoshuawuyts opened this issue Aug 25, 2020 · 3 comments · Fixed by #78401
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-visibility Area: Visibility / privacy. D-confusing Diagnostics: Confusing error or lint that should be reworked.

Comments

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Aug 25, 2020

This is a constructor counterpart to #75907

Overview

Errors for private fields in public non-tuple structs are pretty good. Given the following code:

mod foo {
    pub struct Bar { field: u8 }
}

use foo::Bar;

fn main() {
    let x = Bar { field: 12 };
}

The following error is returned:

error[E0451]: field `field` of struct `foo::Bar` is private
 --> src/main.rs:9:19
  |
9 |     let x = Bar { field: 12 };
  |                   ^^^^^^^^^ private field

A hint could be useful here, but overall this is good enough to understand. However when we convert this to a tuple struct the errors get more confusing:

mod foo {
    pub struct Bar(u8);
}

use foo::Bar;

fn main() {
    let x = Bar(12);
}

This yields the following error:

error[E0423]: expected function, tuple struct or tuple variant, found struct `Bar`
 --> src/main.rs:8:13
  |
8 |     let x = Bar(12);
  |             ^^^ constructor is not visible here due to private fields

The message of expected function, tuple struct or tuple variant, found struct Bar is not helpful here. It's a bit confusing what the fix is: how do we make the constructor visible? I had to look up how to declare visibility for inner fields in tuple structs.

Proposed solution

The error message could probably use some work. Something closer to the text of non-tuple structs would be clearer:

field `0` of struct `foo::Bar` is private

Or perhaps an even better direction would be:

cannot initialize a tuple struct which contains private fields

The main code block seems useful. But we could go even further and provide a hint on how to fix this if the struct is defined inside the same project:

error[E0423]: cannot initialize a tuple struct which contains private fields
 --> src/main.rs:8:13
  |
8 |     let x = Bar(12);
  |             ^^^ constructor is not visible here due to private fields
  |             |
  |             help: declare the struct's fields as public: `struct Bar(pub(crate) u8);`

Conclusion

I found a location where our errors aren't as good as they could be, and figured there may be a way we can use that to not just improve the error message -- but actually teach people about visibility modifiers in tuple structs.

I hope this is helpful. Thanks!

@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. A-visibility Area: Visibility / privacy. labels Aug 25, 2020
@tesuji
Copy link
Contributor

tesuji commented Aug 25, 2020

Just fyi, the second code snippet is the same as the first one: it doesn't use tuple struct at all.

@yoshuawuyts
Copy link
Member Author

@lzutao thanks; fixed!

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 11, 2020
Give better diagnostic when using a private tuple struct constructor

Fixes rust-lang#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 rust-lang#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)
@guswynn
Copy link
Contributor

guswynn commented Sep 11, 2020

turns out tuple structs a bit different compared to structs (#76494 has some details), this one is going to be move involved than the other issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-visibility Area: Visibility / privacy. D-confusing Diagnostics: Confusing error or lint that should be reworked.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants