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

Tracking issue for release notes of #130025: Also emit missing_docs lint with --test to fulfil expectations #130203

Open
1 of 3 tasks
rustbot opened this issue Sep 10, 2024 · 6 comments
Labels
relnotes Marks issues that should be documented in the release notes of the next release. relnotes-tracking-issue Marks issues tracking what text to put in release ntoes.
Milestone

Comments

@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2024

This issue tracks the release notes text for #130025.

  • Proposed text is drafted by PR author (or team) making the noteworthy change.
  • Issue is nominated for release team review of clarity for wider audience.
  • Release team includes text in release notes/blog posts.

Release notes text:

The responsible team for the underlying change should edit this section to replace the
automatically generated link with a succinct description of what changed, drawing upon text
proposed by the author (either in discussion or through direct editing). If the change is notable
enough for inclusion in the blog post, add content to the blog section below.

# Compatibility Notes

The allow-by-default `missing_docs` lint used to disable itself when invoked through `rustc --test`/`cargo test`, resulting in `#[expect(missing_docs)]` emitting false positives due to the expectation being wrongly unfulfilled. This behavior [has now been removed](https://github.com/rust-lang/rust/pull/130025), which allows `#[expect(missing_docs)]` to be fulfilled, but will also report new `missing_docs` diagnostics for publicly reachable `#[cfg(test)]` items, [integration test](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#integration-tests) crate-level documentation, and publicly reachable items in integration tests.

Note: The section title will be de-duplicated by the release team with other release notes issues.
Please use a standard title from previous releases.
More than one section can be included if needed.

Release blog section (if any, leave blank if no section is expected):

cc @Urgau, @petrochenkov -- origin issue/PR authors and assignees for starting to draft text

@rustbot rustbot added relnotes Marks issues that should be documented in the release notes of the next release. relnotes-tracking-issue Marks issues tracking what text to put in release ntoes. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 10, 2024
@Urgau
Copy link
Member

Urgau commented Sep 10, 2024

Compatibility Notes

The allow-by-default missing_docs lint used to disable it-self when invoked through rustc --test/cargo test, which would prevent the lint from being emitted in the whole crate, preventing #[expect(missing_docs)] from working and causing false positives. This behavior has now been removed, making the lint more consistent (in particular regarding publicly-reacheable #[cfg(test)] items) and fixing the false positives.

# Compatibility Notes

The allow-by-default `missing_docs` lint used to disable it-self when invoked through `rustc --test`/`cargo test`, which would prevent the lint from being emitted in the whole crate, preventing `#[expect(missing_docs)]` from working and causing false positives. This behavior [has now been removed](https://github.com/rust-lang/rust/pull/130025), making the lint more consistent (in particular regarding publicly-reacheable `#[cfg(test)]` items) and fixing the false positives.

@Urgau Urgau removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 10, 2024
@Urgau Urgau added this to the 1.83.0 milestone Sep 10, 2024
@kpreid
Copy link
Contributor

kpreid commented Sep 13, 2024

This also affects Cargo test targets (aka integration tests), which (AFAIK) are never built without --test and therefore were never subject to a project-wide lints.missing_docs = "deny" before. Presuming the crate has no public items, it will still be linted for lack of crate documentation (since the crate itself cannot have a lower visibility):

  error: missing documentation for the crate
    --> all-is-cubes-desktop/tests/end-to-end.rs:1:1
     |
  1  | / #[test]
  2  | | fn trycmd_tests() {
  3  | |     let tc = trycmd::TestCases::new();
  ...  |
  20 | |     tc.run();
  21 | | }
     | |_^
     |
     = note: `-D missing-docs` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(missing_docs)]`

I would suggest mentioning test targets in the compatibility notes, since they are not exactly #[cfg(test)] items and there is no pub involved.

@Urgau
Copy link
Member

Urgau commented Sep 13, 2024

Sure, what do you think about changing the last sentence to?

We have now removed this faulty behavior fixing the false positives and making the lint more consistent, in particular regarding publicly-reacheable #[cfg(test)] items, but also regarding Cargo test-only targets which were never subject to the lint and will now require at-least crate documentation.

@kpreid
Copy link
Contributor

kpreid commented Sep 13, 2024

Hmm, well, now I have several critiques of the text:

  • I think “faulty behavior” is unnecessarily aggressive towards the prior design; it made sense when it was designed, and only interacts poorly with expect in particular.
  • “Test-only targets” is not a term in common use. The documentation largely calls them integration tests.
  • The text seems to over-use hyphens, in particular in “publicly-reachable” and “at-least” (which I would expect to be ordinary phrases with spaces, though this is of course subject to language evolution), and “it-self” (which should be a single word).
  • Spelling: reacheablereachable

Proposed revision:

Compatibility Notes

The allow-by-default missing_docs lint used to disable itself when invoked through rustc --test/cargo test, resulting in #[expect(missing_docs)] emitting false positives. This behavior has now been removed, which will allow #[expect(missing_docs)] to function as expected, but will also report new missing_docs diagnostics for publicly reachable #[cfg(test)] items, integration test crate-level documentation, and publicly reachable items in integration tests.

# Compatibility Notes

The allow-by-default `missing_docs` lint used to disable itself when invoked through `rustc --test`/`cargo test`, resulting in `#[expect(missing_docs)]` emitting false positives. This behavior [has now been removed](https://github.com/rust-lang/rust/pull/130025), which will allow `#[expect(missing_docs)]` to function as expected, but will also report new `missing_docs` diagnostics for publicly reachable `#[cfg(test)]` items, [integration test](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#integration-tests) crate-level documentation, and publicly reachable items in integration tests.

@Urgau
Copy link
Member

Urgau commented Sep 13, 2024

Thanks for the feedback, as a non-native English speaker it's always appreciable to have critique of not quite English sentence.

I'm in general agreement with your proposed revision, I would just like to out a bit more emphasis the (un)fulfilled part and avoid the negativity associated with the "but" (I replaced it with "as collateral consequence" and "increasing the consistency of the lint").

Compatibility Notes

The allow-by-default missing_docs lint used to disable itself when invoked through rustc --test/cargo test, resulting in #[expect(missing_docs)] emitting false positives due to the lint being wrongly-unfulfilled. This behavior has now been removed, which allows #[expect(missing_docs)] to correctly detect the lint as fulfilled, as collateral consequence new missing_docs diagnostics for publicly reachable #[cfg(test)] items, integration test crate-level documentation, and publicly reachable items in integration tests are going to be reported, increasing the consistency of the lint.

# Compatibility Notes

The allow-by-default `missing_docs` lint used to disable itself when invoked through `rustc --test`/`cargo test`, resulting in `#[expect(missing_docs)]` emitting false positives due to the lint being wrongly-unfulfilled. This behavior [has now been removed](https://github.com/rust-lang/rust/pull/130025), which allows `#[expect(missing_docs)]` to correctly detect the lint as fulfilled, as collateral consequence new `missing_docs` diagnostics for publicly reachable `#[cfg(test)]` items, [integration test](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#integration-tests) crate-level documentation, and publicly reachable items in integration tests are going to be reported, increasing the consistency of the lint.

@kpreid
Copy link
Contributor

kpreid commented Sep 13, 2024

I would just like to out a bit more emphasis the (un)fulfilled part and avoid the negativity associated with the "but" (I replaced it with "as collateral consequence" and "increasing the consistency of the lint").

In my opinion, most of these changes make it worse. The negativity of a “but” is not very strong, and attempting to avoid it makes one look like one is trying to save face excessively. "Increasing the consistency of the lint" is also making an excuse that doesn’t need to be said — when a change needs compatibility notes, that already means “this change has negative elements but we think it is better on net”, and the notes should be focused on the technical aspect — telling users what changes they need to make to adapt — not on apologizing.

(Also, “as collateral consequence” is an odd phrase; I would write “at the cost of” or “with the side effect that”. But I don't think that part is needed at all.)

I do agree that “fulfilled” is a good thing to point out, but I think one should say “the expectation being wrongly unfulfilled” instead of “the lint being wrongly unfulfilled”.

Proposed revision:

Compatibility Notes

The allow-by-default missing_docs lint used to disable itself when invoked through rustc --test/cargo test, resulting in #[expect(missing_docs)] emitting false positives due to the expectation being wrongly unfulfilled. This behavior has now been removed, which will allow #[expect(missing_docs)] to be fulfilled, but will also report new missing_docs diagnostics for publicly reachable #[cfg(test)] items, integration test crate-level documentation, and publicly reachable items in integration tests.

# Compatibility Notes

The allow-by-default `missing_docs` lint used to disable itself when invoked through `rustc --test`/`cargo test`, resulting in `#[expect(missing_docs)]` emitting false positives due to the expectation being wrongly unfulfilled. This behavior [has now been removed](https://github.com/rust-lang/rust/pull/130025), which will allow `#[expect(missing_docs)]` to be fulfilled, but will also report new `missing_docs` diagnostics for publicly reachable `#[cfg(test)]` items, [integration test](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#integration-tests) crate-level documentation, and publicly reachable items in integration tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. relnotes-tracking-issue Marks issues tracking what text to put in release ntoes.
Projects
None yet
Development

No branches or pull requests

3 participants