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

tuple struct expressions with non-visible (private) fields should bias towards suggesting methods #103215

Open
pnkfelix opened this issue Oct 18, 2022 · 2 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Oct 18, 2022

Given the following code (play):

fn main() {
    let f = std::fs::File::open("/proc/meminfo").unwrap();
    let b = std::io::BufReader(f);
    for line in b.lines() { dbg!(line); }
}

The current output is:

error[[E0423]](https://doc.rust-lang.org/nightly/error-index.html#E0423): expected function, tuple struct or tuple variant, found struct `std::io::BufReader`
 --> src/main.rs:3:13
  |
3 |     let b = std::io::BufReader(f);
  |             ^^^^^^^^^^^^^^^^^^^^^ help: use struct literal syntax instead: `std::io::BufReader { inner: val, buf: val }`

Ideally the output should look like:

3 |     let b = std::io::BufReader(f);
  |             ^^^^^^^^^^^^^^^^^^^^^ help: use constructor method instead: `std::io::BufReader::new(f)`

In other words: If a type from an external crate has non-public fields, we should not be suggesting the user attempt to use them for constructing an instance of the type. Especially in the case of tuple struct expressions, which can be a sign of an omitted constructor method name. (I'm thinking specifically of people who are coming from Java, and thus may be used to writing something like new BufReader(f) where we would write BufReader::new(f).)

A simple heuristic here could be to look for methods on the type that 1. do not take self parameter, and 2. (optional) have a matching number of arguments


This issue is similar to #66067 and #52144, except worse, because (if I understand them correctly) those two are both talking about improving the UX for incorrect tuple struct expressions that at least are also referring to tuple struct definitions.

In the scenario described here, we have a record struct definition being used as a tuple struct, which, in the presence of private fields, we should interpret as a strong hint that someone probably meant to use a method.

@pnkfelix pnkfelix added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 18, 2022
@Plecra
Copy link

Plecra commented Oct 31, 2022

Here's another instance of this issue I came across in #rust-beginners on the community discord (https://discord.com/channels/273534239310479360/273541522815713281/1036653255175979058)

use std::collections::HashMap;
struct PassportBuilder {
    fields: HashMap<String, String>,
}
let mut builder = PassportBuilder{ fields: HashMap<String, String>{} };

raising this error -

error[[E0423]](https://doc.rust-lang.org/stable/error-index.html#E0423): expected value, found struct `HashMap`
 --> src/main.rs:6:44
  |
6 | let mut builder = PassportBuilder{ fields: HashMap<String, String>{} };
  |                                            ^^^^^^^ help: use struct literal syntax instead: `HashMap { base: val }`

this one minimises to (std::collections::HashMap<String, String>{},)

@kadiwa4
Copy link
Contributor

kadiwa4 commented Feb 19, 2024

The original MCVE was fixed by #116858.

Plecra's example was not fixed, but a very slightly modified version was fixed:

use std::collections::HashMap;
struct PassportBuilder {
    fields: HashMap<String, String>,
}
let mut builder = PassportBuilder { fields: HashMap::<String, String>{} };

(note the turbofish after HashMap)

Considering that Plecra's version without the turbofish is deeply broken syntax-wise, I think it's fine to not have it emit a helpful diagnostic.

Diagnostics when not using turbofish
error: expected one of `,`, `:`, or `}`, found `>`
 --> src/main.rs:8:71
  |
8 |     let mut builder = PassportBuilder { fields: HashMap<String, String>{} };
  |                       ---------------                           ------^ expected one of `,`, `:`, or `}`
  |                       |                                         |
  |                       while parsing this struct                 while parsing this struct field
  |
help: try naming a field
  |
8 |     let mut builder = PassportBuilder { fields: HashMap<String, String: String>{} };
  |                                                                 +++++++

error[E0423]: expected value, found struct `HashMap`
   --> src/main.rs:8:49
    |
8   |     let mut builder = PassportBuilder { fields: HashMap<String, String>{} };
    |                                                 ^^^^^^^
    |
   ::: /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:213:1
    |
213 | pub struct HashMap<K, V, S = RandomState> {
    | ----------------------------------------- `HashMap` defined here

error[E0423]: expected value, found struct `String`
   --> src/main.rs:8:57
    |
8   |     let mut builder = PassportBuilder { fields: HashMap<String, String>{} };
    |                                                         ^^^^^^
    |
   ::: /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/string.rs:365:1
    |
365 | pub struct String {
    | ----------------- `String` defined here
    |
help: consider importing one of these items instead
    |
1   + use clap::error::ContextValue::String;
    |
1   + use clap_builder::error::ContextValue::String;
    |
1   + use gimli::AttributeValue::String;
    |
1   + use serde_json::Value::String;
    |
      and 5 other candidates

error[E0308]: mismatched types
 --> src/main.rs:8:49
  |
8 |     let mut builder = PassportBuilder { fields: HashMap<String, String>{} };
  |                                                 ^^^^^^^^^^^^^^ expected `HashMap<String, String>`, found `bool`
  |
  = note: expected struct `HashMap<String, String>`
               found type `bool`

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

Since tests/ui/privacy/suggest-box-new.rs has some very similar test cases, I think this can be closed.

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 T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants