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

Incorrect suggestion for function parameter of type str #82820

Closed
camelid opened this issue Mar 6, 2021 · 7 comments · Fixed by #84728
Closed

Incorrect suggestion for function parameter of type str #82820

camelid opened this issue Mar 6, 2021 · 7 comments · Fixed by #84728
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@camelid
Copy link
Member

camelid commented Mar 6, 2021

fn foo(bar: str) {}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> src/lib.rs:1:8
  |
1 | fn foo(bar: str) {}
  |        ^^^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
help: function arguments must have a statically known size, borrowed types always have a known size
  |
1 | fn foo(&bar: str) {}
  |        ^

error: aborting due to previous error

Applying the suggestion leads to yet more errors. The new errors do suggest using &str, but they still suggest invalid code:

error[E0308]: mismatched types
 --> src/lib.rs:1:8
  |
1 | fn foo(&bar: str) {}
  |        ^^^^-----
  |        |     |
  |        |     expected due to this
  |        expected `str`, found reference
  |        help: did you mean `bar`: `&str`
  |
  = note:   expected type `str`
          found reference `&_`

error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> src/lib.rs:1:8
  |
1 | fn foo(&bar: str) {}
  |        ^^^^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
help: function arguments must have a statically known size, borrowed types always have a known size
  |
1 | fn foo(&bar: &str) {}
  |              ^

error: aborting due to 2 previous errors

After applying the new suggestions, I get:

error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> src/lib.rs:1:9
  |
1 | fn foo(&bar: &str) {}
  |         ^^^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
  = note: all local variables must have a statically known size
  = help: unsized locals are gated as an unstable feature

error: aborting due to previous error

This seems likely to send new users in circles.

@camelid camelid 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. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 6, 2021
@camelid
Copy link
Member Author

camelid commented Mar 6, 2021

Assigning P-medium and removing I-prioritize as discussed in the prioritization working group.

@camelid camelid added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 6, 2021
@JohnTitor JohnTitor added the C-bug Category: This is a bug. label Mar 6, 2021
@apiraino
Copy link
Contributor

apiraino commented Mar 6, 2021

I've bisected to this nightly range:

+nightly-2020-07-12-x86_64-unknown-linux-gnu 346aec9b0 => old error msg
+nightly-2020-07-26-x86_64-unknown-linux-gnu d6953df14 => new error message

(I cannot bisect further because in between of that range I cannot install nightlies 🤷‍♂️ )

@hellow554
Copy link
Contributor

Funny enough, when using _ as variable name, the suggestion will get displayed correctly:

fn f(a: str) {}
fn g(_: str) {}
   Compiling playground v0.0.1 (/playground)
error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> src/lib.rs:1:6
  |
1 | fn f(a: str) {}
  |      ^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
help: function arguments must have a statically known size, borrowed types always have a known size
  |
1 | fn f(&a: str) {}
  |      ^

error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> src/lib.rs:2:6
  |
2 | fn g(_: str) {}
  |      ^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
help: function arguments must have a statically known size, borrowed types always have a known size
  |
2 | fn g(_: &str) {}

@hellow554
Copy link
Contributor

hellow554 commented Apr 28, 2021

This seems to be fixed on nighlty, but not on beta...

somewhere between 5a4ab26...9d9c2c9

Fixed in 9d9c2c9

Fixed by #84313

So this can be closed @camelid 🎉

@hellow554
Copy link
Contributor

hellow554 commented Apr 28, 2021

But, as a sidenote: How could this ever be commited to master? I mean, it's obviosly wrong so say &a: str :/

I hope, that in the future the review process can be improved to detect such errors early on.

The error was introduced in #78152, where the test has been adapted to match the wrong syntax, but nobody has seen this :( Let's hope in the future it will get noticed right away 😊

@camelid
Copy link
Member Author

camelid commented Apr 29, 2021

The error was introduced in #78152, where the test has been adapted to match the wrong syntax, but nobody has seen this :( Let's hope in the future it will get noticed right away 😊

It looks like that PR caused a bunch of small diagnostics changes, so the reviewers probably just missed that bug.

@camelid
Copy link
Member Author

camelid commented Apr 29, 2021

Assigning myself for adding a test. @rustbot claim

@camelid camelid added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 29, 2021
camelid added a commit to camelid/rust that referenced this issue May 6, 2021
This is a regression test for rust-lang#82820.

This test case is included in more general tests, but I think the error
regressed because there were a bunch of other diagnostic changes in the
test that obscured this regression.

Hopefully, having a test specific to the suggestion, and running rustfix
for the test, will prevent this error from regressing in the future.
@bors bors closed this as completed in 3a0d6be May 7, 2021
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-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants