Tweak `self` arg not as first argument of a method diagnostic #61087

estebank · 2019-05-23T16:47:52Z

Mention that self is only valid on "associated functions"

error: unexpected `self` argument in function
  --> $DIR/self-in-function-arg.rs:1:15
   |
LL | fn foo(x:i32, self: i32) -> i32 { self }
   |               ^^^^ not valid as function argument
   |
   = note: `self` is only valid as the first argument of an associated function

When it is a method, mention it must be first

error: unexpected `self` argument in function
  --> $DIR/trait-fn.rs:4:20
   |
LL |     fn c(foo: u32, self) {}
   |                    ^^^^ must be the first associated function argument

Move a bunch of error recovery methods to diagnostics.rs away from parser.rs.

Fix #51547. CC #60015.

rust-highfive · 2019-05-23T16:48:05Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2019-05-23T16:52:03Z

r? @Centril parser code shuffle

src/libsyntax/parse/diagnostics.rs

Centril · 2019-05-23T17:25:39Z

I'll do a more thorough review later.

@estebank Can you split the "move stuff" and "change diagnostics output" into separate commits and then a third commit for addressing review comments?

Centril · 2019-05-23T17:12:49Z

src/libsyntax/parse/diagnostics.rs

+                     .map_or(String::new(), |t| t.to_string());
+            i.enumerate().fold(b, |mut b, (i, a)| {
+                if tokens.len() > 2 && i == tokens.len() - 2 {
+                    b.push_str(", or ");


Refactor out b.push_str(connective)?

src/libsyntax/parse/diagnostics.rs

estebank · 2019-05-23T20:23:45Z

@Centril addressed all review comments but one, which I think should be part of a separate PR cleaning up all the times this one-or-more string handling has been done in the parser.

Centril

r=me with rebase and possibly a fix to the comment below

Centril · 2019-05-25T09:39:25Z

src/libsyntax/parse/diagnostics.rs

+        is_trait_item: bool,
+    ) -> PResult<'a, ast::Arg> {
+        let sp = arg.pat.span;
+        arg.ty.node = TyKind::Err;


Why not set the TyKind to what it would have been had it been in the right position?

Because we don't know wether this is any of the following:

a function with an arbitrary argument called self

a method with self not at the start

a method with self at the start and a second self

I thought it'd be safer to just mark it as TyKind::Err and avoid potential incorrect diagnostics elsewhere. I prefer to be silent on actual problems (given other errors are emitted) over being too verbose with possibly unnecessary errors.

Good insight; on the other hand, I'd say that self not at the start is probabilistically more correct than the others... but, in any case, let's land this as-is. :)

Mention that `self` is only valid on "associated functions" ``` error: unexpected `self` argument in function --> $DIR/self-in-function-arg.rs:1:15 | LL | fn foo(x:i32, self: i32) -> i32 { self } | ^^^^ not valid as function argument | = note: `self` is only valid as the first argument of an associated function ``` When it is a method, mention it must be first ``` error: unexpected `self` argument in function --> $DIR/trait-fn.rs:4:20 | LL | fn c(foo: u32, self) {} | ^^^^ must be the first associated function argument ```

Move a bunch of error recovery methods to `diagnostics.rs` away from `parser.rs`.

estebank · 2019-05-25T20:24:17Z

@bors r=Centril

bors · 2019-05-25T20:24:18Z

📌 Commit 4e68ddc has been approved by Centril

Tweak `self` arg not as first argument of a method diagnostic Mention that `self` is only valid on "associated functions" ``` error: unexpected `self` argument in function --> $DIR/self-in-function-arg.rs:1:15 | LL | fn foo(x:i32, self: i32) -> i32 { self } | ^^^^ not valid as function argument | = note: `self` is only valid as the first argument of an associated function ``` When it is a method, mention it must be first ``` error: unexpected `self` argument in function --> $DIR/trait-fn.rs:4:20 | LL | fn c(foo: u32, self) {} | ^^^^ must be the first associated function argument ``` Move a bunch of error recovery methods to `diagnostics.rs` away from `parser.rs`. Fix rust-lang#51547. CC rust-lang#60015.

@ghost

Rollup of 9 pull requests Successful merges: - #61087 (Tweak `self` arg not as first argument of a method diagnostic) - #61114 (Vec: avoid creating slices to the elements) - #61144 (Suggest borrowing for loop head on move error) - #61149 (Fix spelling in release notes) - #61161 (MaybeUninit doctest: remove unnecessary type ascription) - #61173 (Auto-derive Encode and Decode implementations of DefPathTable) - #61184 (Add additional trace statements to the const propagator) - #61189 (Turn turbo 🐟 🍨 into an error) - #61193 (Add comment to explain why we change the layout for Projection) Failed merges: r? @ghost

rust-highfive assigned pnkfelix May 23, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2019

estebank force-pushed the parsepalooza branch from 394a005 to 4dc5937 Compare May 23, 2019 16:51

rust-highfive assigned Centril and unassigned pnkfelix May 23, 2019

petrochenkov assigned pnkfelix and petrochenkov May 23, 2019

petrochenkov reviewed May 23, 2019

View reviewed changes

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

Centril suggested changes May 23, 2019

View reviewed changes

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

petrochenkov reviewed May 23, 2019

View reviewed changes

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

petrochenkov removed their assignment May 23, 2019

Centril suggested changes May 23, 2019

View reviewed changes

estebank force-pushed the parsepalooza branch from 4dc5937 to 960e8dd Compare May 23, 2019 20:14

This comment has been minimized.

Sign in to view

estebank force-pushed the parsepalooza branch 3 times, most recently from b116e3b to 0230fc3 Compare May 25, 2019 00:18

This comment has been minimized.

Sign in to view

Centril reviewed May 25, 2019

View reviewed changes

Centril 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 May 25, 2019

estebank added 3 commits May 25, 2019 12:05

Move some methods to diagnostics.rs away from parser.rs

d1364d5

Move a bunch of error recovery methods to `diagnostics.rs` away from `parser.rs`.

review comments: move back some methods and clean up wording

4e68ddc

estebank force-pushed the parsepalooza branch from 0230fc3 to 4e68ddc Compare May 25, 2019 19:21

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2019

Centril mentioned this pull request May 25, 2019

Rollup of 9 pull requests #61198

Closed

Centril mentioned this pull request May 26, 2019

Rollup of 9 pull requests #61201

Merged

bors merged commit 4e68ddc into rust-lang:master May 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak `self` arg not as first argument of a method diagnostic #61087

Tweak `self` arg not as first argument of a method diagnostic #61087

estebank commented May 23, 2019 •

edited

Loading

rust-highfive commented May 23, 2019

estebank commented May 23, 2019

Centril commented May 23, 2019 •

edited

Loading

Centril May 23, 2019

estebank commented May 23, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

Centril left a comment •

edited

Loading

Centril May 25, 2019

estebank May 25, 2019 •

edited

Loading

Centril May 25, 2019

estebank commented May 25, 2019

bors commented May 25, 2019

Tweak self arg not as first argument of a method diagnostic #61087

Tweak self arg not as first argument of a method diagnostic #61087

Conversation

estebank commented May 23, 2019 • edited Loading

rust-highfive commented May 23, 2019

estebank commented May 23, 2019

Centril commented May 23, 2019 • edited Loading

Centril May 23, 2019

Choose a reason for hiding this comment

estebank commented May 23, 2019 • edited Loading

This comment has been minimized.

This comment has been minimized.

Centril left a comment • edited Loading

Choose a reason for hiding this comment

Centril May 25, 2019

Choose a reason for hiding this comment

estebank May 25, 2019 • edited Loading

Choose a reason for hiding this comment

Centril May 25, 2019

Choose a reason for hiding this comment

estebank commented May 25, 2019

bors commented May 25, 2019

Tweak `self` arg not as first argument of a method diagnostic #61087

Tweak `self` arg not as first argument of a method diagnostic #61087

estebank commented May 23, 2019 •

edited

Loading

Centril commented May 23, 2019 •

edited

Loading

estebank commented May 23, 2019 •

edited

Loading

Centril left a comment •

edited

Loading

estebank May 25, 2019 •

edited

Loading