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

rustdoc: do not panic all the way when lexing a source snippet fails #33510

Closed
wants to merge 1 commit into from

Conversation

birkenfeld
Copy link
Contributor

Currently, the libsyntax lexer panics on invalid source, which makes rustdoc panic when trying to highlight it.

I assume there are efforts underway to make the lexer panic-free, but until this is done this should be an acceptable workaround.

Note that the panic is still printed like normal as "thread X panicked, run with RUST_BACKTRACE=1 ...", so I added the printout below to make it seem less like a fatal error.

I didn't touch render_inner_with_highlighting below, as it is currently unused. It returns a Result whose Err type would have to be changed if it was to support lexer errors.

Fixes: #30032

Currently, the libsyntax lexer panics on invalid source, which
makes rustdoc panic when trying to highlight it.

I assume there are efforts underway to make the lexer panic-free,
but until this is done this should be an acceptable workaround.

Note that the panic is still printed like normal as "thread X panicked,
run with RUST_BACKTRACE=1 ...", so I added the printout below to
make it seem less like a fatal error.

I didn't touch `render_inner_with_highlighting` below, as it is
currently unused.  It returns a Result whose Err type would have to
be changed if it was to support lexer errors.

Fixes: rust-lang#30032
@rust-highfive
Copy link
Collaborator

r? @steveklabnik

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

@steveklabnik
Copy link
Member

Sorry, I should not be the reviewer on this one, as it's out of my area of specialty. Maybe someone from @rust-lang/tools ?

@alexcrichton
Copy link
Member

This is unfortunately basically what catch_unwind is not intended to be used for. Would it be possible to use variuos non-panicking methods in the parser at this point? Or possibly modify the parser to add them?

@birkenfeld
Copy link
Contributor Author

See OP:

I assume there are efforts underway to make the lexer panic-free, but until this is done this should be an acceptable workaround.

@alexcrichton
Copy link
Member

Right, I'm just wondering if it's reasonable to accelerate those ephemeral efforts by implementing it as part of this PR.

@birkenfeld
Copy link
Contributor Author

I would work on this, but I don't know if someone else is already doing it/planning to do it - I know there were already a few iterations of removing panics.

@alexcrichton
Copy link
Member

Ah I don't recall too much activity working on the parser recently, so you should be good to go!

@brson
Copy link
Contributor

brson commented Jun 9, 2016

Personally I don't mind doing this. I'd even say this is exactly what catch_unwind is for: there's some external subsystem that's unreliable (the lexer), so this provides a boundary to stop that flakiness and keep the whole system up. If the lexer can be fixed easily then great, but if not I think this approach is fine.

@bors
Copy link
Contributor

bors commented Jun 16, 2016

☔ The latest upstream changes (presumably #34187) made this pull request unmergeable. Please resolve the merge conflicts.

@steveklabnik
Copy link
Member

r? @alexcrichton

@alexcrichton
Copy link
Member

Thanks for the ping @steveklabnik, I'm gonna close this out of inactivity for now, but feel free to resubmit with a rebase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants