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

Also emit missing_docs lint with --test to fulfil expectations #130025

Merged
merged 4 commits into from
Sep 10, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Sep 6, 2024

This PR removes the "test harness" suppression of the missing_docs lint to be able to fulfil #[expect] (expectations) as it is now "relevant".

I think the goal was to maybe avoid false-positive while linting on public items under #[cfg(test)] but with effective visibility we should no longer have any false-positive.

Another possibility would be to query the lint level and only emit the lint if it's of expect level, but that is even more hacky.

Fixes #130021

try-job: x86_64-gnu-aux

@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2024
@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Sep 6, 2024

r? compiler

@rustbot rustbot assigned petrochenkov and unassigned lcnr Sep 6, 2024
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 7, 2024

📌 Commit ae661dd has been approved by petrochenkov

It is now in the queue for this repository.

@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 Sep 7, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 8, 2024
…chenkov

Also emit `missing_docs` lint with `--test` to fulfil expectations

This PR removes the "test harness" suppression of the `missing_docs` lint to be able to fulfil `#[expect]` (expectations) as it is now "relevant".

I think the goal was to maybe avoid false-positive while linting on public items under `#[cfg(test)]` but with effective visibility we should no longer have any false-positive.

Another possibility would be to query the lint level and only emit the lint if it's of expect level, but that is even more hacky.

Fixes rust-lang#130021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#129337 (rustdoc rfc#3662 changes under unstable flags)
 - rust-lang#129473 (use  `download-ci-llvm=true` in the default compiler config)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#130025 (Also emit `missing_docs` lint with `--test` to fulfil expectations)
 - rust-lang#130070 (Rename variant `AddrOfRegion` of `RegionVariableOrigin` to `BorrowRegion`)
 - rust-lang#130087 (remove 'const' from 'Option::iter')
 - rust-lang#130092 (Fixes typo in wasm32-wasip2 doc comment)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 8, 2024
…chenkov

Also emit `missing_docs` lint with `--test` to fulfil expectations

This PR removes the "test harness" suppression of the `missing_docs` lint to be able to fulfil `#[expect]` (expectations) as it is now "relevant".

I think the goal was to maybe avoid false-positive while linting on public items under `#[cfg(test)]` but with effective visibility we should no longer have any false-positive.

Another possibility would be to query the lint level and only emit the lint if it's of expect level, but that is even more hacky.

Fixes rust-lang#130021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#129473 (use  `download-ci-llvm=true` in the default compiler config)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#130025 (Also emit `missing_docs` lint with `--test` to fulfil expectations)
 - rust-lang#130048 (run-make-support: Add llvm-pdbutil)
 - rust-lang#130070 (Rename variant `AddrOfRegion` of `RegionVariableOrigin` to `BorrowRegion`)
 - rust-lang#130087 (remove 'const' from 'Option::iter')
 - rust-lang#130092 (Fixes typo in wasm32-wasip2 doc comment)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

I'm wondering if this could have the potential to break cargo tests #130109 (comment)

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 9, 2024
…chenkov

Also emit `missing_docs` lint with `--test` to fulfil expectations

This PR removes the "test harness" suppression of the `missing_docs` lint to be able to fulfil `#[expect]` (expectations) as it is now "relevant".

I think the goal was to maybe avoid false-positive while linting on public items under `#[cfg(test)]` but with effective visibility we should no longer have any false-positive.

Another possibility would be to query the lint level and only emit the lint if it's of expect level, but that is even more hacky.

Fixes rust-lang#130021
@workingjubilee
Copy link
Member

@workingjubilee
Copy link
Member

@bors r-

@bors bors added 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 9, 2024
@workingjubilee
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Sep 9, 2024

⌛ Trying commit ae661dd with merge d453327...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2024
Also emit `missing_docs` lint with `--test` to fulfil expectations

This PR removes the "test harness" suppression of the `missing_docs` lint to be able to fulfil `#[expect]` (expectations) as it is now "relevant".

I think the goal was to maybe avoid false-positive while linting on public items under `#[cfg(test)]` but with effective visibility we should no longer have any false-positive.

Another possibility would be to query the lint level and only emit the lint if it's of expect level, but that is even more hacky.

Fixes rust-lang#130021

try-job: x86_64-gnu-aux
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 9, 2024

📌 Commit 0f9cb07 has been approved by petrochenkov

It is now in the queue for this repository.

@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 Sep 9, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 10, 2024
…chenkov

Also emit `missing_docs` lint with `--test` to fulfil expectations

This PR removes the "test harness" suppression of the `missing_docs` lint to be able to fulfil `#[expect]` (expectations) as it is now "relevant".

I think the goal was to maybe avoid false-positive while linting on public items under `#[cfg(test)]` but with effective visibility we should no longer have any false-positive.

Another possibility would be to query the lint level and only emit the lint if it's of expect level, but that is even more hacky.

Fixes rust-lang#130021

try-job: x86_64-gnu-aux
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2024
…kingjubilee

Rollup of 14 pull requests

Successful merges:

 - rust-lang#128316 (Stabilize most of `io_error_more`)
 - rust-lang#129473 (use  `download-ci-llvm=true` in the default compiler config)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129778 (interpret: make typed copies lossy wrt provenance and padding)
 - rust-lang#129981 (Remove `serialized_bitcode` from `LtoModuleCodegen`.)
 - rust-lang#130025 (Also emit `missing_docs` lint with `--test` to fulfil expectations)
 - rust-lang#130040 (unify `llvm-bitcode-linker`, `wasm-component-ld` and llvm-tools logics)
 - rust-lang#130094 (Inform the solver if evaluation is concurrent)
 - rust-lang#130132 ([illumos] enable SIGSEGV handler to detect stack overflows)
 - rust-lang#130146 (bootstrap `naked_asm!` for `compiler-builtins`)
 - rust-lang#130149 (Helper function for formatting with `LifetimeSuggestionPosition`)
 - rust-lang#130152 (adapt a test for llvm 20)
 - rust-lang#130162 (bump download-ci-llvm-stamp)
 - rust-lang#130164 (move some const fn out of the const_ptr_as_ref feature)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Sep 10, 2024

⌛ Testing commit 0f9cb07 with merge 33855f8...

@bors
Copy link
Contributor

bors commented Sep 10, 2024

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 33855f8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 10, 2024
@bors bors merged commit 33855f8 into rust-lang:master Sep 10, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 10, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (33855f8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary -2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-3.0%, -2.8%] 2
All ❌✅ (primary) - - 0

Cycles

Results (secondary -4.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-6.0% [-7.9%, -3.5%] 4
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 755.942s -> 757.872s (0.26%)
Artifact size: 341.35 MiB -> 341.32 MiB (-0.01%)

@Urgau Urgau added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 10, 2024
ogoffart added a commit to ogoffart/rust that referenced this pull request Sep 11, 2024
Since rust-lang#130025, the compiler don't ignore missing_docs when compiling the tests.
But there is now a false positive warning for every `#[test]`

For example, this code
```rust
//! Crate docs

fn just_a_test() {}
```

Would emit this warning when running `cargo test`

```
warning: missing documentation for a constant
 --> src/lib.rs:5:1
  |
4 | #[test]
  | ------- in this procedural macro expansion
5 | fn just_a_test() {}
  | ^^^^^^^^^^^^^^^^^^^
```
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
Fix false positive with `missing_docs` and `#[test]`

Since rust-lang#130025, the compiler don't ignore missing_docs when compiling the tests. But there is now a false positive warning for every `#[test]`

For example, this code
```rust
//! Crate docs

fn just_a_test() {}
```

Would emit this warning when running `cargo test`

```
warning: missing documentation for a constant
 --> src/lib.rs:5:1
  |
4 | #[test]
  | ------- in this procedural macro expansion
5 | fn just_a_test() {}
  | ^^^^^^^^^^^^^^^^^^^
```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 11, 2024
Fix false positive with `missing_docs` and `#[test]`

Since rust-lang#130025, the compiler don't ignore missing_docs when compiling the tests. But there is now a false positive warning for every `#[test]`

For example, this code
```rust
//! Crate docs

fn just_a_test() {}
```

Would emit this warning when running `cargo test`

```
warning: missing documentation for a constant
 --> src/lib.rs:5:1
  |
4 | #[test]
  | ------- in this procedural macro expansion
5 | fn just_a_test() {}
  | ^^^^^^^^^^^^^^^^^^^
```
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
Rollup merge of rust-lang#130219 - ogoffart:missing-docs-test, r=Urgau

Fix false positive with `missing_docs` and `#[test]`

Since rust-lang#130025, the compiler don't ignore missing_docs when compiling the tests. But there is now a false positive warning for every `#[test]`

For example, this code
```rust
//! Crate docs

fn just_a_test() {}
```

Would emit this warning when running `cargo test`

```
warning: missing documentation for a constant
 --> src/lib.rs:5:1
  |
4 | #[test]
  | ------- in this procedural macro expansion
5 | fn just_a_test() {}
  | ^^^^^^^^^^^^^^^^^^^
```
jonasbb added a commit to jonasbb/serde_with that referenced this pull request Sep 12, 2024
kpreid added a commit to kpreid/all-is-cubes that referenced this pull request Sep 13, 2024
rustc 1.83 will lint `missing_docs` in `--test` builds, which did not
occur previously. <rust-lang/rust#130025>
@MrCroxx
Copy link

MrCroxx commented Sep 20, 2024

Hi, thanks for the contribution. May I ask if this PR would be cherry-picked to a patch release like 1.81.1?

@Urgau
Copy link
Member Author

Urgau commented Sep 20, 2024

Due to our fast release cycle, we don't backport to stable simple bug-fixes, only security fixes or major regressions fixes.

#[expect(missing_bugs)] never worked on stable, so it's not technically a regression either, making it unlikely to get beta-backported either.

@workingjubilee
Copy link
Member

great new lint, expect(missing_bugs) so that you can be sure your code is buggy :^)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive unfulfilled_lint_expectations with either of --tests or --all-targets
10 participants