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

update rustdoc's syntax checker to work with error-recovering lexer #63284

Closed
matklad opened this issue Aug 5, 2019 · 7 comments
Closed

update rustdoc's syntax checker to work with error-recovering lexer #63284

matklad opened this issue Aug 5, 2019 · 7 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@matklad
Copy link
Member

matklad commented Aug 5, 2019

Rustdoc processes code inside of ``` blocks in two ways:

  • First, the code is checked for lexer errors

  • Second, lexer-based syntax highlighting is done

The checking for lexer errors works by intercepting fatal errors from the lexer. However, since this was originally implemented, the lexer moved from fatal erroring to error-recovery (#63017 in particular tries to remove the last bit of fatal erroring). That means that the current approach intercepts only a fraction of lexer errors, while most of the errors are reported twice (once duing the check, once during highlighing). Here's an example:

14:30:41|~/tmp
λ cat main.rs 
/// ```
/// '...'
/// ```
pub fn foo() {}

14:31:11|~/tmp
λ rustdoc main.rs
error: character literal may only contain one codepoint
 --> <doctest>:1:1
  |
1 | '...'
  | ^^^^^
help: if you meant to write a `str` literal, use double quotes
  |
1 | "..."
  |

error: character literal may only contain one codepoint
 --> <rustdoc-highlighting>:1:1
  |
1 | '...'
  | ^^^^^
help: if you meant to write a `str` literal, use double quotes
  |
1 | "..."

I think that, to fix this, we should configure the parsing session with a custom Emitter. For code-checking pass, the emmitter should downgrade all diagnostics to warnings and set a flag if there were any diagnostics. For syntax-highlighting pass, we should use a "/dev/null" emitter which just doesn't emit anything.

cc @euclio, @GuillaumeGomez

@jonas-schievink jonas-schievink added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 5, 2019
@euclio
Copy link
Contributor

euclio commented Aug 5, 2019

cc #56885

@matklad
Copy link
Member Author

matklad commented Aug 5, 2019

hm, #56885 makes me think that there are two ways to approach this:

  • use special emitter (what issue description is proposing)
  • make all lexer errors buffered

I don't know what would be better (someone with more broad compiler knowledge needs to decide this), but I like BufferingEmitter approach better, because it can be reused anywhere, while buffering in the lexer will be special-casing the lexer.

@estebank
Copy link
Contributor

estebank commented Aug 5, 2019

An alternative would be to have the lexer and parser have a "strict" mode where they bail early when encountering any error, that way the caller can handle the Err case themselves. This would also help people using the Parser as a library.

@Centril
Copy link
Contributor

Centril commented Aug 5, 2019

@estebank That sounds like it would complicate parser.rs (which is already in a sorry state!).

@Mark-Simulacrum
Copy link
Member

A separate/custom emitter is definitely the way to go here, we should not complicate parsing code anymore than it already is.

Centril added a commit to Centril/rust that referenced this issue Nov 5, 2019
use silent emitter for rustdoc highlighting pass

Partially addresses rust-lang#63284.
Centril added a commit to Centril/rust that referenced this issue Nov 6, 2019
use silent emitter for rustdoc highlighting pass

Partially addresses rust-lang#63284.
@jyn514
Copy link
Member

jyn514 commented Dec 15, 2020

@matklad this only gives one warning on master, I think it was fixed by #76068 ?

@camelid
Copy link
Member

camelid commented Jan 29, 2021

Should this be closed?

@jyn514 jyn514 closed this as completed Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants