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

Fix suggestion when shorthand self has erroneous type #122161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

compiler-errors
Copy link
Member

Fixes #122086

r? estebank

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2024
… r=workingjubilee

Bless tidy issues order

The order is not right now because of rust-lang#121248 (comment)

from rust-lang#122161 (comment)

r? `@workingjubilee`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Rollup merge of rust-lang#122175 - chenyukang:yukang-fix-tidy-issues, r=workingjubilee

Bless tidy issues order

The order is not right now because of rust-lang#121248 (comment)

from rust-lang#122161 (comment)

r? `@workingjubilee`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 9, 2024
…gjubilee

Bless tidy issues order

The order is not right now because of rust-lang/rust#121248 (comment)

from rust-lang/rust#122161 (comment)

r? `@workingjubilee`
@compiler-errors
Copy link
Member Author

Seems like @estebank is busy, so

r? compiler

@rustbot rustbot assigned michaelwoerister and unassigned estebank Mar 19, 2024
@compiler-errors
Copy link
Member Author

r? @fmease

@rustbot rustbot assigned fmease and unassigned michaelwoerister Mar 19, 2024
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like it! I have some smol nitpicks.

@@ -2787,6 +2814,7 @@ impl<'a> Parser<'a> {
let eself_lo = self.token.span;
let (eself, eself_ident, eself_hi) = match self.token.uninterpolate().kind {
token::BinOp(token::And) => {
let lo = self.token.span;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's just eself_lo a few lines further up.

let eself_ident = expect_self_ident(this);

// Recover `: Type` after a qualified self
if this.may_recover() && this.check_noexpect(&token::Colon) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also eat_noexpect instead of check + manual bump

@@ -2355,6 +2355,19 @@ pub enum SelfKind {
Explicit(P<Ty>, Mutability),
}

impl SelfKind {
pub fn as_suggestion(&self) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn as_suggestion(&self) -> String {
pub fn to_suggestion(&self) -> String {

Minor style nit: The Rust API guidelines recommend to_ for “expensive” (alloc) borrowed→owned conversions. as_ is for cheap ones.

https://rust-lang.github.io/api-guidelines/naming.html#c-conv

impl SelfKind {
pub fn as_suggestion(&self) -> String {
match self {
SelfKind::Value(mutbl) => mutbl.prefix_str().to_string(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutbl == Mutability::Mut is also unreachable here similar to the explicit case

SelfKind::Region(None, mutbl) => mutbl.ref_prefix_str().to_string(),
SelfKind::Region(Some(lt), mutbl) => format!("&{lt} {}", mutbl.prefix_str()),
SelfKind::Explicit(_, _) => {
unreachable!("if we had an explicit self, we wouldn't be here")
Copy link
Member

@fmease fmease Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to mark this as unreachable but I think the method name is a bit too nondescript, ppl might accidentally reach for it not knowing the internal invariants (I know, it's super easy to find out in this case but I hope you get my point). Maybe the method name could be made more specific? 🤔

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2024
@bors
Copy link
Contributor

bors commented Apr 23, 2024

☔ The latest upstream changes (presumably #124277) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

self param with ampersand (&) and explicit type yields noisy parsing diagnostics which could be more helpful
8 participants