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

doctest: Reset errors before dropping the parse session #81040

Merged
merged 2 commits into from
Jan 16, 2021

Conversation

osa1
Copy link
Contributor

@osa1 osa1 commented Jan 15, 2021

The first parse is to collect whether the code contains macros, has
main, and uses other crates. In that pass we ignore errors as those
will be reported when the test file is actually built.

For that we need to reset errors in the Diagnostic otherwise when
dropping it unhandled errors will be reported as compiler bugs.

Fixes #80992

@rust-highfive
Copy link
Collaborator

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2021
@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

I'll be adding a test if I can figure out how ...

@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

I added a test but it doesn't pass even though I added a stdout file with (I think) right contents. Details here: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Unable.20to.20add.20failing.20rustdoc.20test

@jyn514 jyn514 added A-doctests Area: Documentation tests, run by rustdoc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2021
The first parse is to collect whether the code contains macros, has
`main`, and uses other crates. In that pass we ignore errors as those
will be reported when the test file is actually built.

For that we need to reset errors in the `Diagnostic` otherwise when
dropping it unhandled errors will be reported as compiler bugs.

Fixes rust-lang#80992
@osa1
Copy link
Contributor Author

osa1 commented Jan 16, 2021

I added a regression test. The test is a pass-check test and doesn't compare the error message with expected one, but it fails on master branch (panics) so it should be good enough.

@jyn514 this is now ready for reviews. Thanks for the help!

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2021
@jyn514
Copy link
Member

jyn514 commented Jan 16, 2021

r=me with CI passing. Thanks for the fix!

@jyn514
Copy link
Member

jyn514 commented Jan 16, 2021

err - actually I don't think this tests anything, won't ICEs be marked as compile_fail still? I think you need to match against the error output after all.

@osa1
Copy link
Contributor Author

osa1 commented Jan 16, 2021

@jyn514 If I keep the test, but revert the fix, then the test fails with this:

Check compiletest suite=rustdoc-ui mode=ui (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 1 test
F
failures:

---- [ui] rustdoc-ui/issue-80992.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 101
command: "/home/omer/rust/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustdoc" "/home/omer/rust/rust/src/test/rustdoc-ui/issue-80992.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "-o" "/home/omer/rust/rust/build/x86_64-unknown-linux-gnu/test/rustdoc-ui/issue-80992" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/home/omer/rust/rust/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--test" "-L" "/home/omer/rust/rust/build/x86_64-unknown-linux-gnu/test/rustdoc-ui/issue-80992/auxiliary"
stdout:
------------------------------------------

running 1 test
test src/test/rustdoc-ui/issue-80992.rs - test (line 7) ... FAILED

failures:

---- src/test/rustdoc-ui/issue-80992.rs - test (line 7) stdout ----
thread 'src/test/rustdoc-ui/issue-80992.rs - test (line 7)' panicked at 'no errors encountered even though `delay_span_bug` issued', compiler/rustc_errors/src/lib.rs:974:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

query stack during panic:
end of query stack


failures:
    src/test/rustdoc-ui/issue-80992.rs - test (line 7)

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s


------------------------------------------
stderr:
------------------------------------------
error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.51.0-dev running on x86_64-unknown-linux-gnu

note: compiler flags: -Z threads=1 -Z ui-testing -Z deduplicate-diagnostics=no -Z emit-future-incompat-report -Z unstable-options -C debuginfo=0


------------------------------------------



failures:
    [ui] rustdoc-ui/issue-80992.rs

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 95 filtered out; finished in 0.04s

Some tests failed in compiletest suite=rustdoc-ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu

Is this not good enough?

@jyn514
Copy link
Member

jyn514 commented Jan 16, 2021

Oh interesting, I didn't realize rustdoc treated ICEs different from compile errors. Yes, that's good enough.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2021

📌 Commit 0ef5557 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2021
@osa1
Copy link
Contributor Author

osa1 commented Jan 16, 2021

won't ICEs be marked as compile_fail still

But the test is compile_pass, not compile_fail. Did you misunderstand the test maybe?

@osa1
Copy link
Contributor Author

osa1 commented Jan 16, 2021

So my understanding is if a rustdoc compilation fails with ICE, then that's not considered a success for "compile_fail". I'd expect an ICE to never be considered good for compile_fail tests as I'm think the compiler should never panic. (instead fail with a good error message when things go wrong)

@jyn514
Copy link
Member

jyn514 commented Jan 16, 2021

But the test is compile_pass, not compile_fail. Did you misunderstand the test maybe?

The compiletest test is // check-pass. The rustdoc test is compile_fail. compiletest is checking that rustdoc successfully marks compile errors as failed.

So my understanding is if a rustdoc compilation fails with ICE, then that's not considered a success for "compile_fail".

Make sense to me :)

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 16, 2021
doctest: Reset errors before dropping the parse session

The first parse is to collect whether the code contains macros, has
`main`, and uses other crates. In that pass we ignore errors as those
will be reported when the test file is actually built.

For that we need to reset errors in the `Diagnostic` otherwise when
dropping it unhandled errors will be reported as compiler bugs.

Fixes rust-lang#80992
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
doctest: Reset errors before dropping the parse session

The first parse is to collect whether the code contains macros, has
`main`, and uses other crates. In that pass we ignore errors as those
will be reported when the test file is actually built.

For that we need to reset errors in the `Diagnostic` otherwise when
dropping it unhandled errors will be reported as compiler bugs.

Fixes rust-lang#80992
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2021
Rollup of 17 pull requests

Successful merges:

 - rust-lang#78455 (Introduce {Ref, RefMut}::try_map for optional projections in RefCell)
 - rust-lang#80144 (Remove giant badge in README)
 - rust-lang#80614 (Explain why borrows can't be held across yield point in async blocks)
 - rust-lang#80670 (TrustedRandomAaccess specialization composes incorrectly for nested iter::Zips)
 - rust-lang#80681 (Clarify what the effects of a 'logic error' are)
 - rust-lang#80764 (Re-stabilize Weak::as_ptr and friends for unsized T)
 - rust-lang#80901 (Make `x.py --color always` apply to logging too)
 - rust-lang#80902 (Add a regression test for rust-lang#76281)
 - rust-lang#80941 (Do not suggest invalid code in pattern with loop)
 - rust-lang#80968 (Stabilize the poll_map feature)
 - rust-lang#80971 (Put all feature gate tests under `feature-gates/`)
 - rust-lang#81021 (Remove doctree::Import)
 - rust-lang#81040 (doctest: Reset errors before dropping the parse session)
 - rust-lang#81060 (Add a regression test for rust-lang#50041)
 - rust-lang#81065 (codegen_cranelift: Fix redundant semicolon warn)
 - rust-lang#81069 (Add sample code for Rc::new_cyclic)
 - rust-lang#81081 (Add test for rust-lang#34792)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9df8dcb into rust-lang:master Jan 16, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doctests Area: Documentation tests, run by rustdoc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE given unclosed angle-bracket *only* in doctest
5 participants