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.

Sign up for GitHub

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

Jump to bottom

"associated function without body" error suggestion has incorrect span #83104

Closed
camelid opened this issue Mar 14, 2021 · 10 comments · Fixed by #95220
Closed

"associated function without body" error suggestion has incorrect span #83104

camelid opened this issue Mar 14, 2021 · 10 comments · Fixed by #95220
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@camelid
Copy link
Member

camelid commented Mar 14, 2021 •
edited by rustbot
Loading

It suggests

fn foo({ <body> }

when it should suggest

fn foo() { <body> }

or at least suggest

fn foo(){ <body> }

struct Foo;

impl Foo {
    fn foo()
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error: expected one of `->`, `;`, `where`, or `{`, found `}`
 --> src/lib.rs:5:1
  |
4 |     fn foo()
  |        ---  - expected one of `->`, `;`, `where`, or `{`
  |        |
  |        while parsing this `fn`
5 | }
  | ^ unexpected token

error: associated function in `impl` without body
 --> src/lib.rs:4:5
  |
4 |     fn foo()
  |     ^^^^^^^-
  |            |
  |            help: provide a definition for the function: `{ <body> }`

error: aborting due to 2 previous errors

error: could not compile `playground`

To learn more, run the command again with --verbose.
@camelid camelid added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Mar 14, 2021
@camelid
Copy link
Member Author

camelid commented Mar 14, 2021

Probably the "free function without body" variant has this bug as well.

@camelid
Copy link
Member Author

camelid commented Mar 14, 2021

The code at issue:

rust/compiler/rustc_ast_passes/src/ast_validation.rs

Lines 409 to 419 in acca818

fn error_item_without_body(&self, sp: Span, ctx: &str, msg: &str, sugg: &str) {
self.err_handler()
.struct_span_err(sp, msg)
.span_suggestion(
self.session.source_map().end_point(sp),
&format!("provide a definition for the {}", ctx),
sugg.to_string(),
Applicability::HasPlaceholders,
)
.emit();
}

@ThePuzzlemaker
Copy link
Contributor

I think I'll take a shot at this.
@rustbot claim

@ThePuzzlemaker
Copy link
Contributor

That was... an easy fix.
end_point to next_point
That provides this:

error: expected one of `->`, `;`, `where`, or `{`, found `}`
 --> <anon>:5:1
  |
4 |     fn foo()
  |        ---  - expected one of `->`, `;`, `where`, or `{`
  |        |
  |        while parsing this `fn`
5 | }
  | ^ unexpected token

error: associated function in `impl` without body
 --> <anon>:4:5
  |
4 |     fn foo()
  |     ^^^^^^^^- help: provide a definition for the function: `{ <body> }`

error: aborting due to 2 previous errors

which isn't entirely satisfactory imo cause the suggestion span is directly after the ) rather than a space after, but it's at least valid Rust code that can be formatted into more "properly" formatted Rust code.

@ThePuzzlemaker
Copy link
Contributor

ThePuzzlemaker commented Mar 23, 2021 •
edited
Loading

Probably the "free function without body" variant has this bug as well.

It appears that it has a similar bug:

fn foo();
error: free function without a body
 --> <anon>:1:1
  |
1 | fn foo();
  | ^^^^^^^^^- help: provide a definition for the function: `{ <body> }`

error: aborting due to previous error

Looks like this can probably be fixed similarly.

@ThePuzzlemaker
Copy link
Contributor

Oh wait, that bug was caused by me. It looks like I'll probably have to implement a better solution.

@ThePuzzlemaker
Copy link
Contributor

Okay, so it appears that this bug is triggered in inherent impls with and without a semicolon, and in free functions with a semicolon. I'll have to find some kind of way to change the span depending on if it had a semicolon or not.

@ThePuzzlemaker
Copy link
Contributor

Okay, so it looks like in 99% of cases that trigger the same code path as this error:

  • free fn
  • free const
  • free static
  • free type
  • inherent const
  • inherent fn
  • trait const
  • trait type
  • trait fn

the existing code is already correct, as in those cases a semicolon is (usually) required. Either we can modify the parser to give some kind of indication to the AST validator that an inherent fn/const/type has no semicolon at the end, or we can just use a hack to check if there's a semicolon at the end of the span

@ThePuzzlemaker
Copy link
Contributor

I'm not quite sure how this should be fixed :/

@camelid
Copy link
Member Author

camelid commented Mar 23, 2021

I'm not quite sure how this should be fixed :/

@ThePuzzlemaker I would suggest asking in #t-compiler/help since someone there can probably give advice.

@ThePuzzlemaker ThePuzzlemaker removed their assignment May 5, 2021
@notriddle notriddle mentioned this issue Mar 22, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 23, 2022
@bors
Auto merge of rust-lang#95220 - rust-lang:notriddle/ast-validation-se…
547369d 
…micolon, r=Dylan-DPC

diagnostics: do not suggest `fn foo({ <body> }`

Instead of suggesting that the body always replace the last character on the line, presuming it must be a semicolon, the parser should instead check what the last character is, and append the body if it is anything else.

Fixes rust-lang#83104
@bors bors closed this as completed in 3729b17 Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

diagnostics: do not suggest fn foo({ <body> } rust-lang/rust
2 participants
@ThePuzzlemaker @camelid