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

Confusing error message on syntax error in code in doc comments #6109

Closed
feds01 opened this issue Mar 10, 2024 · 17 comments
Closed

Confusing error message on syntax error in code in doc comments #6109

feds01 opened this issue Mar 10, 2024 · 17 comments
Labels
a-comments good first issue Issues up for grabs, also good candidates for new rustfmt contributors needs-test only-with-option requires a non-default option value to reproduce

Comments

@feds01
Copy link

feds01 commented Mar 10, 2024

When enabling format_code_in_doc_comments, the formatter reports that there is a syntax issue in a code block. However the syntax issue source location seems to be confusing/ambigious:

Running rustfmt on the following code snippet produces a confusing error message:

/// Some doc comment with code snippet:
///```
/// '\u{1F}
/// ```
pub struct Code {}

fn main() {}

The error that is produced:

error[E0762]: unterminated character literal
 --> <stdin>:2:5
  |
2 |     '\u{1F}
  |     ^^^^^^^

This is not explicitly a bug, but nonetheless is confusing as it does not indicate the true origin of the issue.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 11, 2024

Thanks for the report. Just want to understand how you're calling rustfmt. Are you running rustfmt directly on a file or is it your editor that's invoking rustfmt?

@ytmimi
Copy link
Contributor

ytmimi commented Mar 11, 2024

Linking the tracking issue format_code_in_doc_comments #3348

@ytmimi ytmimi added the only-with-option requires a non-default option value to reproduce label Mar 11, 2024
@feds01
Copy link
Author

feds01 commented Mar 14, 2024

Calling through cargo fmt in the project workspace - hence why the error pops up and its unclear where the source of the error is.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 19, 2024

@feds01 by the way, what version of rustfmt are you using?

@ytmimi
Copy link
Contributor

ytmimi commented Mar 19, 2024

@davidtwco I'm assuming this might have been caused by sharing the SilentEmitter between rustc and rustfmt in rust-lang/rust#121301. I believe this is coming up because rustc's SilentEmitter is defined to still emit Fatal diagnostics, while rustfmt's SilentEmitter emits nothing. I'm able to reproduce the issue using nightly rustfmt, but not when building rustfmt from source, presumably because we haven't pulled these changes back into rustfmt yet.

Do you think it's possible to modify rustc's SilentEmitter to conditionally ignore Fatal diagnostics as well? For rustfmt I think that's the behavior that we'd want.

@feds01
Copy link
Author

feds01 commented Mar 19, 2024

@feds01 by the way, what version of rustfmt are you using?

$ cargo --version --verbose

cargo 1.78.0-nightly (a4c63fe53 2024-03-06)
release: 1.78.0-nightly
commit-hash: a4c63fe5388beaa09e5f91196c86addab0a03580
commit-date: 2024-03-06
host: aarch64-apple-darwin
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.1.2 (sys:0.4.72 curl-8.6.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 14.1.1 [64-bit]
cargo fmt --version
rustfmt 1.7.0-nightly (2d24fe59 2024-03-09)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 24, 2024
…diagnostic, r=davidtwco

conditionally ignore fatal diagnostic in the SilentEmitter

This change is primarily meant to allow rustfmt to ignore all diagnostics when using the `SilentEmitter`. Back in rust-lang#121301 the `SilentEmitter` was shared between rustc and rustfmt. This changed rustfmt's behavior from ignoring all diagnostic to emitting fatal diagnostics, which lead to rust-lang/rustfmt#6109.

These changes allow rustfmt to maintain its previous behaviour when using the `SilentEmitter`, while allowing rustc code to still emit fatal diagnostics.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 24, 2024
…diagnostic, r=davidtwco

conditionally ignore fatal diagnostic in the SilentEmitter

This change is primarily meant to allow rustfmt to ignore all diagnostics when using the `SilentEmitter`. Back in rust-lang#121301 the `SilentEmitter` was shared between rustc and rustfmt. This changed rustfmt's behavior from ignoring all diagnostic to emitting fatal diagnostics, which lead to rust-lang/rustfmt#6109.

These changes allow rustfmt to maintain its previous behaviour when using the `SilentEmitter`, while allowing rustc code to still emit fatal diagnostics.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 24, 2024
Rollup merge of rust-lang#122737 - ytmimi:conditionally_ignore_fatal_diagnostic, r=davidtwco

conditionally ignore fatal diagnostic in the SilentEmitter

This change is primarily meant to allow rustfmt to ignore all diagnostics when using the `SilentEmitter`. Back in rust-lang#121301 the `SilentEmitter` was shared between rustc and rustfmt. This changed rustfmt's behavior from ignoring all diagnostic to emitting fatal diagnostics, which lead to rust-lang/rustfmt#6109.

These changes allow rustfmt to maintain its previous behaviour when using the `SilentEmitter`, while allowing rustc code to still emit fatal diagnostics.
RenjiSann pushed a commit to RenjiSann/rust that referenced this issue Mar 25, 2024
…diagnostic, r=davidtwco

conditionally ignore fatal diagnostic in the SilentEmitter

This change is primarily meant to allow rustfmt to ignore all diagnostics when using the `SilentEmitter`. Back in rust-lang#121301 the `SilentEmitter` was shared between rustc and rustfmt. This changed rustfmt's behavior from ignoring all diagnostic to emitting fatal diagnostics, which lead to rust-lang/rustfmt#6109.

These changes allow rustfmt to maintain its previous behaviour when using the `SilentEmitter`, while allowing rustc code to still emit fatal diagnostics.
@ytmimi
Copy link
Contributor

ytmimi commented Mar 25, 2024

I just tested with the nightly-2024-03-25 toolchain (rustfmt 1.7.0-nightly (0824b300 2024-03-24)) and this issue should be resolved.

@feds01 when you have a moment could you also confirm this:

Steps to confirm:

  • rustup install nightly-2024-03-25
  • rustfmt nightly-2024-03-25 --config=format_code_in_doc_comments=true /path/to/test/file.rs

@ytmimi
Copy link
Contributor

ytmimi commented Mar 25, 2024

Before closing this issue I think it would be great if we could add a test case to prevent a regression

@ytmimi ytmimi added good first issue Issues up for grabs, also good candidates for new rustfmt contributors needs-test labels Mar 25, 2024
@feds01
Copy link
Author

feds01 commented Mar 25, 2024

It is now not reporting any issues with the file, is the desired behaviour?

@ytmimi
Copy link
Contributor

ytmimi commented Mar 25, 2024

@feds01 In this case yes. The previous behavior was for rustfmt to ignore the diagnostic message produced by the rustc parser, however there were some internal changes made in rust-lang/rust that changed that behavior. rust-lang/rust#122737 restored rustfmts previous behavior of ignoring parser errors when formatting code in doc comments.

github-actions bot pushed a commit to ytmimi/rustfmt that referenced this issue Apr 8, 2024
…c, r=davidtwco

conditionally ignore fatal diagnostic in the SilentEmitter

This change is primarily meant to allow rustfmt to ignore all diagnostics when using the `SilentEmitter`. Back in #121301 the `SilentEmitter` was shared between rustc and rustfmt. This changed rustfmt's behavior from ignoring all diagnostic to emitting fatal diagnostics, which lead to rust-lang#6109.

These changes allow rustfmt to maintain its previous behaviour when using the `SilentEmitter`, while allowing rustc code to still emit fatal diagnostics.
@giordan12
Copy link
Contributor

Hi, I'm planning to help with this as my first contribution. From looking at the contributing.md doc I understand that I would need to create single target file with the code mentioned at the top of this issue?

@ytmimi
Copy link
Contributor

ytmimi commented Jul 5, 2024

@giordan12 that's great. I appreciate you wanting to take this one one. Yes, you can add a test case in the target directory, and in addition to that I think a new standalone test case should be added in https://github.com/rust-lang/rustfmt/blob/master/tests/rustfmt/main.rs to validate that no errors are emitted to stdout or stderr.

@giordan12
Copy link
Contributor

@ytmimi appreciate the guidance, just created the PR. Let me know if there is something to add (I'm expecting there will be :) )

@ytmimi
Copy link
Contributor

ytmimi commented Jul 8, 2024

closing now that #6232 is merged

@ytmimi ytmimi closed this as completed Jul 8, 2024
@feds01
Copy link
Author

feds01 commented Jul 8, 2024

Thanks for this guys ♥️ Let me know if I can also help with anything, I'd love to contribute!

@giordan12
Copy link
Contributor

giordan12 commented Jul 10, 2024

@ytmimi I was wondering if it's worth making it clear in Contributing.md that besides creating source and target files an actual test is needed? The current text makes it look like it's enough to make the test files IMO

@ytmimi
Copy link
Contributor

ytmimi commented Jul 10, 2024

I don't think it's necessary. It's mostly this specific case since I wanted to add a test to ensure nothing was being output to stdout and stderr. Typically a source and target file are all that's required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments good first issue Issues up for grabs, also good candidates for new rustfmt contributors needs-test only-with-option requires a non-default option value to reproduce
Projects
None yet
Development

No branches or pull requests

3 participants