-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Suggest pub
when public
is used
#99706
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
Conversation
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Could you add a UI test to ensure your change works as expected? There's a guide for it: https://rustc-dev-guide.rust-lang.org/tests/adding.html Also, it'd be great if you could squash commits not to show up unrelated changes in Git history. |
I've already squashed, adding UI tests 👍🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with a test case
Added tests and squashed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, would appreciate if you could move the test (and fix the other nits if you're so inclined) and then I'll r=me :)
// Checks what happens when `public` is used instead of the correct, `pub` | ||
// edition:2018 | ||
public struct MyStruct; | ||
//~^ ERROR 3:8: 3:14: expected one of `!` or `::`, found keyword `struct` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//~^ ERROR 3:8: 3:14: expected one of `!` or `::`, found keyword `struct` | |
//~^ ERROR expected one of `!` or `::`, found keyword `struct` |
nit: you don't need to include the line/column numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test failed when I didn't include the line/column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely shouldn't - almost every test is written with error annotations that don't include the line/column numbers. What error are you seeing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the line/column makes it fail like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you just need to --bless
the test again. The important part of that error is where it says "The actual fixed differed from the expected fixed" - that just means that the output it gets from running rustfix
on the test is different from what it expects the output to be (this happens when you first add // run-rustfix
, or when the test changes such that the output is genuinely different, such as if it the output had the comment with line numbers in it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/rust-lang/rust/runs/7518469562?check_suite_focus=true
This also happens in CI, blessing the test again in a minute
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Could you rebase instead of merging? See our no merge policy :) |
[I have accidentally messed up and mixed my commit history of this branch with something else, closing and re-opening with a new PR, sorry] |
Add diagnostic when using public instead of pub Forwarding from rust-lang#99706 I accidentally broke something(??) in git and the commits in that PR are absolutely not what I did in that branch Anyways, this is the PR for this now. Adding tests again in a minute. cc `@davidtwco`
Fixes #99653