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 Json Test Suite: Avoid using #![no_core] #117487

Closed
aDotInTheVoid opened this issue Nov 1, 2023 · 1 comment · Fixed by #117679
Closed

Rustdoc Json Test Suite: Avoid using #![no_core] #117487

aDotInTheVoid opened this issue Nov 1, 2023 · 1 comment · Fixed by #117679
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc 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

@aDotInTheVoid
Copy link
Member

Many tests in tests/rustdoc-json use #![feature(no_core)] #![no_core]. This significantly reduces the size of the generates JSON, and makes the tests easier to write.

However it also means the tests now rely on the unstable (and undocumented) contract between core and rustc. This means that unreleated rustc changes can cause these tests to fail, as they usually only contain the subset of lang-items needed to make the tests pass.

We don't want rustdoc-json tests to create unnessessary work for people working on rustc, who may not be framiliar with how the tests work, and deffinatly don't want to be interupted by our flakey tests.

Eg: https://github.com/rust-lang/rust/pull/117213/files#diff-9e25fc2f875153412739ef3cdf1267fc82c6f2743f0d7ee8dba65671d24cb3caR4-R7

Therefor, we should remove usages of #![no_core] in tests/rustdoc-json. Some of them will be unavoidable if we're trying to test core specific behaviour (eg inherent impls on primitives), but we should limit #![no_core] to cases where it's strictly nessessary.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 1, 2023
@aDotInTheVoid aDotInTheVoid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-testsuite Area: The testsuite used to check the correctness of rustc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-rustdoc-json Area: Rustdoc JSON backend and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 1, 2023
@GuillaumeGomez
Copy link
Member

We could at least keep #![no_std] though. Still not as no_core but better than not having it.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 8, 2023
…eGomez

tests/rustdoc-json: Avoid needless use of `no_core` and `lang_items`

See rust-lang#117487 for motivation.

I've split it into three commits, depending on how much work it was to remove `#![no_core]`. The first is entirely mechanical, the second makes no logical changes but couldn't be done with find+replace, and the third required rewriting assertions no not depend on having `#![no_core]`. All of the interesting changes for review are in the third commit, so I recommend reviewing commit-by-commit.

After this, 3 tests still use `#![no_core]`:

- `./tests/rustdoc-json/primitives/primitive_impls.rs`. Uses impls on primitives, so needs to simulate core
- `./tests/rustdoc-json/primitives/local_primitive.rs`: Uses `rustc_doc_primitive`, so needs to simulate core
- `./tests/rustdoc-json/impls/auto.rs`: Uses auto traits, so needs to simulate core

But after this change, we only rely on the core-rustc boundary in tests that deliberately test those interactions.

r? `@GuillaumeGomez`

Fixes rust-lang#117487
@bors bors closed this as completed in 5d00a5d Nov 8, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 8, 2023
Rollup merge of rust-lang#117679 - aDotInTheVoid:yes-core, r=GuillaumeGomez

tests/rustdoc-json: Avoid needless use of `no_core` and `lang_items`

See rust-lang#117487 for motivation.

I've split it into three commits, depending on how much work it was to remove `#![no_core]`. The first is entirely mechanical, the second makes no logical changes but couldn't be done with find+replace, and the third required rewriting assertions no not depend on having `#![no_core]`. All of the interesting changes for review are in the third commit, so I recommend reviewing commit-by-commit.

After this, 3 tests still use `#![no_core]`:

- `./tests/rustdoc-json/primitives/primitive_impls.rs`. Uses impls on primitives, so needs to simulate core
- `./tests/rustdoc-json/primitives/local_primitive.rs`: Uses `rustc_doc_primitive`, so needs to simulate core
- `./tests/rustdoc-json/impls/auto.rs`: Uses auto traits, so needs to simulate core

But after this change, we only rely on the core-rustc boundary in tests that deliberately test those interactions.

r? ``@GuillaumeGomez``

Fixes rust-lang#117487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc 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

Successfully merging a pull request may close this issue.

3 participants