Tweak `self` arg not as first argument of a method diagnostic by estebank · Pull Request #61087 · rust-lang/rust

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

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

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

Uh oh!

Conversation

estebank commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 23, 2019

Uh oh!

estebank commented May 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Centril commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Centril May 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

estebank commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Centril left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Centril May 25, 2019

Choose a reason for hiding this comment

Uh oh!

estebank May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Centril May 25, 2019

Choose a reason for hiding this comment

Uh oh!

estebank commented May 25, 2019

Uh oh!

bors commented May 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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