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

Properly handle postfix inc/dec in standalone and subexpr scenarios #104875

Merged
merged 3 commits into from
Dec 14, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 16 additions & 41 deletions compiler/rustc_parse/src/parser/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,6 @@ enum IsStandalone {
Standalone,
/// It's a subexpression, i.e., *not* standalone.
Subexpr,
/// It's maybe standalone; we're not sure.
Maybe,
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -213,14 +211,8 @@ impl MultiSugg {
err.multipart_suggestion(&self.msg, self.patches, self.applicability);
}

/// Overrides individual messages and applicabilities.
fn emit_many(
err: &mut Diagnostic,
msg: &str,
applicability: Applicability,
suggestions: impl Iterator<Item = Self>,
) {
err.multipart_suggestions(msg, suggestions.map(|s| s.patches), applicability);
fn emit_verbose(self, err: &mut Diagnostic) {
err.multipart_suggestion_verbose(&self.msg, self.patches, self.applicability);
}
}

Expand Down Expand Up @@ -1272,21 +1264,20 @@ impl<'a> Parser<'a> {
let standalone =
if prev_is_semi { IsStandalone::Standalone } else { IsStandalone::Subexpr };
let kind = IncDecRecovery { standalone, op: IncOrDec::Inc, fixity: UnaryFixity::Pre };

self.recover_from_inc_dec(operand_expr, kind, op_span)
}

pub(super) fn recover_from_postfix_increment(
&mut self,
operand_expr: P<Expr>,
op_span: Span,
prev_is_semi: bool,
) -> PResult<'a, P<Expr>> {
let kind = IncDecRecovery {
standalone: IsStandalone::Maybe,
standalone: if prev_is_semi { IsStandalone::Standalone } else { IsStandalone::Subexpr },
op: IncOrDec::Inc,
fixity: UnaryFixity::Post,
};

self.recover_from_inc_dec(operand_expr, kind, op_span)
}

Expand Down Expand Up @@ -1314,35 +1305,20 @@ impl<'a> Parser<'a> {
UnaryFixity::Post => (base.span.shrink_to_lo(), op_span),
};

let Ok(base_src) = self.span_to_snippet(base.span)
else { return help_base_case(err, base) };
chenyukang marked this conversation as resolved.
Show resolved Hide resolved
match kind.standalone {
IsStandalone::Standalone => self.inc_dec_standalone_suggest(kind, spans).emit(&mut err),
IsStandalone::Subexpr => {
let Ok(base_src) = self.span_to_snippet(base.span)
else { return help_base_case(err, base) };
match kind.fixity {
UnaryFixity::Pre => {
self.prefix_inc_dec_suggest(base_src, kind, spans).emit(&mut err)
}
UnaryFixity::Post => {
self.postfix_inc_dec_suggest(base_src, kind, spans).emit(&mut err)
}
}
}
IsStandalone::Maybe => {
let Ok(base_src) = self.span_to_snippet(base.span)
else { return help_base_case(err, base) };
let sugg1 = match kind.fixity {
UnaryFixity::Pre => self.prefix_inc_dec_suggest(base_src, kind, spans),
UnaryFixity::Post => self.postfix_inc_dec_suggest(base_src, kind, spans),
};
let sugg2 = self.inc_dec_standalone_suggest(kind, spans);
MultiSugg::emit_many(
&mut err,
"use `+= 1` instead",
Applicability::Unspecified,
[sugg1, sugg2].into_iter(),
)
IsStandalone::Standalone => {
self.inc_dec_standalone_suggest(kind, spans).emit_verbose(&mut err)
}
IsStandalone::Subexpr => match kind.fixity {
UnaryFixity::Pre => {
self.prefix_inc_dec_suggest(base_src, kind, spans).emit(&mut err)
}
UnaryFixity::Post => {
self.postfix_inc_dec_suggest(base_src, kind, spans).emit(&mut err)
}
},
}
Err(err)
}
Expand Down Expand Up @@ -1392,7 +1368,6 @@ impl<'a> Parser<'a> {
}

patches.push((post_span, format!(" {}= 1", kind.op.chr())));

MultiSugg {
msg: format!("use `{}= 1` instead", kind.op.chr()),
patches,
Expand Down
10 changes: 9 additions & 1 deletion compiler/rustc_parse/src/parser/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,15 @@ impl<'a> Parser<'a> {
let op_span = self.prev_token.span.to(self.token.span);
// Eat the second `+`
self.bump();
lhs = self.recover_from_postfix_increment(lhs, op_span)?;
let prev_is_semi = {
if let Ok(prev_code) = self.sess.source_map().span_to_prev_source(lhs.span) &&
prev_code.trim_end().ends_with(";") {
true
} else {
false
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a hack, but fair given where it lives. I would like to do a perf run of an alternative approach: modify lhs to hold a starts_statement: bool field. I think that it might hit memory a bit, but that would be far less brittle to refactors breaking this heuristic. Would you have time to try doing that as part of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

You means add a field with boolean type to LhsExpr type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the code.
I don't think there is a perf regerssion here, we are at error report codepath,
but you may have a double-check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, maybe cost a little bit more memory.

lhs = self.recover_from_postfix_increment(lhs, op_span, prev_is_semi)?;
continue;
}

Expand Down
63 changes: 63 additions & 0 deletions src/test/ui/parser/increment-notfixed.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// run-rustfix

struct Foo {
bar: Bar,
}

struct Bar {
qux: i32,
}

pub fn post_regular() {
let mut i = 0;
i += 1; //~ ERROR Rust has no postfix increment operator
println!("{}", i);
}

pub fn post_while() {
let mut i = 0;
while { let tmp = i; i += 1; tmp } < 5 {
//~^ ERROR Rust has no postfix increment operator
println!("{}", i);
}
}

pub fn post_regular_tmp() {
let mut tmp = 0;
tmp += 1; //~ ERROR Rust has no postfix increment operator
println!("{}", tmp);
}

pub fn post_while_tmp() {
let mut tmp = 0;
while { let tmp_ = tmp; tmp += 1; tmp_ } < 5 {
//~^ ERROR Rust has no postfix increment operator
println!("{}", tmp);
}
}

pub fn post_field() {
let mut foo = Foo { bar: Bar { qux: 0 } };
foo.bar.qux += 1;
//~^ ERROR Rust has no postfix increment operator
println!("{}", foo.bar.qux);
}

pub fn post_field_tmp() {
struct S {
tmp: i32
}
let mut s = S { tmp: 0 };
s.tmp += 1;
//~^ ERROR Rust has no postfix increment operator
println!("{}", s.tmp);
}

pub fn pre_field() {
let mut foo = Foo { bar: Bar { qux: 0 } };
foo.bar.qux += 1;
//~^ ERROR Rust has no prefix increment operator
println!("{}", foo.bar.qux);
}

fn main() {}
8 changes: 5 additions & 3 deletions src/test/ui/parser/increment-notfixed.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// run-rustfix
chenyukang marked this conversation as resolved.
Show resolved Hide resolved

struct Foo {
bar: Bar,
}
Expand Down Expand Up @@ -35,7 +37,7 @@ pub fn post_while_tmp() {
}

pub fn post_field() {
let foo = Foo { bar: Bar { qux: 0 } };
let mut foo = Foo { bar: Bar { qux: 0 } };
foo.bar.qux++;
//~^ ERROR Rust has no postfix increment operator
println!("{}", foo.bar.qux);
Expand All @@ -45,14 +47,14 @@ pub fn post_field_tmp() {
struct S {
tmp: i32
}
let s = S { tmp: 0 };
let mut s = S { tmp: 0 };
s.tmp++;
//~^ ERROR Rust has no postfix increment operator
println!("{}", s.tmp);
}

pub fn pre_field() {
let foo = Foo { bar: Bar { qux: 0 } };
let mut foo = Foo { bar: Bar { qux: 0 } };
++foo.bar.qux;
//~^ ERROR Rust has no prefix increment operator
println!("{}", foo.bar.qux);
Expand Down
26 changes: 7 additions & 19 deletions src/test/ui/parser/increment-notfixed.stderr
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
error: Rust has no postfix increment operator
--> $DIR/increment-notfixed.rs:11:6
--> $DIR/increment-notfixed.rs:13:6
|
LL | i++;
| ^^ not a valid postfix operator
|
help: use `+= 1` instead
|
LL | { let tmp = i; i += 1; tmp };
| +++++++++++ ~~~~~~~~~~~~~~~
LL | i += 1;
| ~~~~

error: Rust has no postfix increment operator
--> $DIR/increment-notfixed.rs:17:12
--> $DIR/increment-notfixed.rs:19:12
|
LL | while i++ < 5 {
| ----- ^^ not a valid postfix operator
Expand All @@ -23,24 +21,20 @@ help: use `+= 1` instead
|
LL | while { let tmp = i; i += 1; tmp } < 5 {
| +++++++++++ ~~~~~~~~~~~~~~~
LL | while i += 1 < 5 {
| ~~~~

error: Rust has no postfix increment operator
--> $DIR/increment-notfixed.rs:25:8
--> $DIR/increment-notfixed.rs:27:8
|
LL | tmp++;
| ^^ not a valid postfix operator
|
help: use `+= 1` instead
|
LL | { let tmp_ = tmp; tmp += 1; tmp_ };
| ++++++++++++ ~~~~~~~~~~~~~~~~~~
LL | tmp += 1;
| ~~~~

error: Rust has no postfix increment operator
--> $DIR/increment-notfixed.rs:31:14
--> $DIR/increment-notfixed.rs:33:14
|
LL | while tmp++ < 5 {
| ----- ^^ not a valid postfix operator
Expand All @@ -51,37 +45,31 @@ help: use `+= 1` instead
|
LL | while { let tmp_ = tmp; tmp += 1; tmp_ } < 5 {
| ++++++++++++ ~~~~~~~~~~~~~~~~~~
LL | while tmp += 1 < 5 {
| ~~~~

error: Rust has no postfix increment operator
--> $DIR/increment-notfixed.rs:39:16
--> $DIR/increment-notfixed.rs:41:16
|
LL | foo.bar.qux++;
| ^^ not a valid postfix operator
|
help: use `+= 1` instead
|
LL | { let tmp = foo.bar.qux; foo.bar.qux += 1; tmp };
| +++++++++++ ~~~~~~~~~~~~~~~~~~~~~~~~~
LL | foo.bar.qux += 1;
| ~~~~

error: Rust has no postfix increment operator
--> $DIR/increment-notfixed.rs:49:10
--> $DIR/increment-notfixed.rs:51:10
|
LL | s.tmp++;
| ^^ not a valid postfix operator
|
help: use `+= 1` instead
|
LL | { let tmp = s.tmp; s.tmp += 1; tmp };
| +++++++++++ ~~~~~~~~~~~~~~~~~~~
LL | s.tmp += 1;
| ~~~~

error: Rust has no prefix increment operator
--> $DIR/increment-notfixed.rs:56:5
--> $DIR/increment-notfixed.rs:58:5
|
LL | ++foo.bar.qux;
| ^^ not a valid prefix operator
Expand Down
30 changes: 30 additions & 0 deletions src/test/ui/parser/issue-104867-inc-dec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
struct S {
x: i32,
}

fn test1() {
let mut i = 0;
i++; //~ ERROR Rust has no postfix increment operator
}

fn test2() {
let s = S { x: 0 };
s.x++; //~ ERROR Rust has no postfix increment operator
}

fn test3() {
let mut i = 0;
if i++ == 1 {} //~ ERROR Rust has no postfix increment operator
}

fn test4() {
let mut i = 0;
++i; //~ ERROR Rust has no prefix increment operator
}

fn test5() {
let mut i = 0;
if ++i == 1 { } //~ ERROR Rust has no prefix increment operator
}

chenyukang marked this conversation as resolved.
Show resolved Hide resolved
fn main() {}
58 changes: 58 additions & 0 deletions src/test/ui/parser/issue-104867-inc-dec.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
error: Rust has no postfix increment operator
--> $DIR/issue-104867-inc-dec.rs:7:6
|
LL | i++;
| ^^ not a valid postfix operator
|
help: use `+= 1` instead
|
LL | i += 1;
| ~~~~

error: Rust has no postfix increment operator
--> $DIR/issue-104867-inc-dec.rs:12:8
|
LL | s.x++;
| ^^ not a valid postfix operator
|
help: use `+= 1` instead
|
LL | s.x += 1;
| ~~~~

error: Rust has no postfix increment operator
--> $DIR/issue-104867-inc-dec.rs:17:9
|
LL | if i++ == 1 {}
| ^^ not a valid postfix operator
|
help: use `+= 1` instead
|
LL | if { let tmp = i; i += 1; tmp } == 1 {}
| +++++++++++ ~~~~~~~~~~~~~~~

error: Rust has no prefix increment operator
--> $DIR/issue-104867-inc-dec.rs:22:5
|
LL | ++i;
| ^^ not a valid prefix operator
|
help: use `+= 1` instead
|
LL - ++i;
LL + i += 1;
|

error: Rust has no prefix increment operator
--> $DIR/issue-104867-inc-dec.rs:27:8
|
LL | if ++i == 1 { }
| ^^ not a valid prefix operator
|
help: use `+= 1` instead
|
LL | if { i += 1; i } == 1 { }
| ~ +++++++++

error: aborting due to 5 previous errors