-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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: Replaces fn main search and extern crate search with proper parsing during doctests. #54861
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2f3bb83
to
08214de
Compare
This comment has been minimized.
This comment has been minimized.
Thanks for your PR! You should hear from someone from @QuietMisdreavus or someone else on the Rustdoc team soon. |
fd79240
to
08214de
Compare
Wo, nice one! |
Should hopefully have time either towards the end of the week or the weekend to get this PR fixed up and ready to go 👌 |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ping from triage @QuietMisdreavus: This PR requires your review. |
Looks like the parsing errors are still being printed, though it's not causing rustdoc to fail:
However, it looks like what actually caused the failure was the error index generator? There's a new batch of errors at the end of the travis log:
I need to look closer to see why the error-index generator is running into problems, but that's at least the source of the current travis failure. |
Odd that the errors are still being printed, I didn't experience that doing a local doc build. But yeah, those hard errors are what have caused me to get kind of stuck :( I tried a bunch of different things locally, from how I was handling the errors, to trying different ways of cancelling -- all to no avail. If you have any hints on the cause I should hopefully have time this weekend to fix it up and then maybe, just maybe, the PR will finally build 😄 |
The reason the error-index generator is running through your code is because it uses rustdoc's Markdown parsing code to output HTML, and the Markdown parsing uses the test generator to create playground links for code samples. I also looked into libsyntax, and i think i know why it's still emitting the errors: the parser code doesn't bubble up every error it creates. For example, here's the spot in the parser that corresponds to the first error from the error-index: rust/src/libsyntax/parse/parser.rs Lines 5363 to 5366 in de9666f
Since the parser considers it an error but wants to continue parsing the function to emit any other errors, it immediately emits the error and carries on. To truly silence errors from the parser, it looks like we'll need to go through this parsing code and figure out a way to silence them, but only for this one location. I also found why it failed entirely: any errors that come out of the conversion to a TokenStream (done during the creation of the Parser) that are immediately thrown as a fatal error. This includes the "unexpected close delimiter" at the end of the error index's errors. I'm having trouble finding the specific example that is causing the issue, though, since so many samples are supposed to fail compiling. There's also the possibility that something about the |
Update: I found it! I had to wire The comment on the crate line absorbs the opening delimiter for the module that the test defines, because there was no line break between them, even though that line break existed in the original. I'm rebuilding now, but you should add That just leaves the errors being printed. I'm investigating using an error emitter that sends information to an |
f679313
to
0fe6aae
Compare
I wound up writing the fix in myself, so i'm going to swap in a new reviewer: I'd also like someone from @rust-lang/compiler to take a look since i'm adding a few new methods in libsyntax and i want to make sure i'm not stepping on anyone's toes. |
@@ -174,14 +174,25 @@ pub fn parse_stream_from_source_str(name: FileName, source: String, sess: &Parse | |||
source_file_to_stream(sess, sess.source_map().new_source_file(name, source), override_span) | |||
} | |||
|
|||
// Create a new parser from a source string | |||
/// Create a new parser from a source string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use maybe_new_parser_from_source_str
and emit the buffered errors? That would also allow removing source_file_to_parser
, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How deep do you want me to go? Should i reimplement each existing function in terms of its new maybe_
variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have the time, that'd be great. I just want to minimize code duplication. Whenever I add a fallible version of an existing method, I rewrite the callees if under a dozen, or I rewrite the infallible version to call the fallible version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've got the change written locally, i'll push once i finish my current build.
@@ -175,6 +175,16 @@ impl<'a> StringReader<'a> { | |||
self.fatal_errs.clear(); | |||
} | |||
|
|||
pub fn buffer_fatal_errors(&mut self) -> Vec<Diagnostic> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this was the only API addition you needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the new_or_buffered_errs
constructor. I needed a way to "handle" the errors without prematurely emitting them (in this case, to cancel them), and this felt like a natural way to get the content of self.fatal_errs
without just exposing the Vec directly. Since both new
and new_without_err
emitted the errors directly, i added this instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against it, I'm surprised at how little you needed to add :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! I just needed a version that wouldn't immediately crash the process if it ran into a problem in the first token. One thing i will ask is whether the current implementation with buffer()
is appropriate, since plain Diagnostics don't have the destructor bomb that full DiagnosticBuilder
s have. (Since they need a Handler
to be emitted, they don't have the same ability to forcibly print themselves.) The alternative would be to swap out the Vec entirely and return the full DiagnosticBuilder, and since the ParseSess
isn't local the lifetimes would work out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment has been minimized.
This comment has been minimized.
c56b193
to
c08659c
Compare
Whoops, wrote |
It seems to have failed again with the same error
|
That message about But the panic message... that passes locally, so i have no idea what it's running into! |
c08659c
to
014c8c4
Compare
Oh hey, my bluff about just forcing travis to try again seems to have worked! CI is green now. |
@bors r+ |
📌 Commit 014c8c4 has been approved by |
☀️ Test successful - status-appveyor, status-travis |
Fixes #21299.
Fixes #33731.
Let me know if there's any additional changes you'd like made!