Limit the number of names and values in check-cfg diagnostics by Urgau · Pull Request #121202 · rust-lang/rust (original) (raw)

The Rust for Linux feedback to the check-cfg Call for Testing, revealed a weakness in the check-cfg. They are unbounded and in the case RfL they have ~20k cfgs and having them printed (even once) is unbearable.

This PR limits it to 35 (28 rustc well known + feature + docsrs + 5 custom) which feels like a good middle ground for regular users (i.e. Cargo users).

When it goes over that limit print the N first with " and X more".

@rustbot label +F-check-cfg

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 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

Feb 16, 2024

ojeda mentioned this pull request

Feb 17, 2024

95 tasks

Are there cases where it might be necessary to read the full list? In that case, perhaps it could be written to a file on disk and the error could give that file path, similar to how very long types are written to disk.

I proposed it in the issue and the responses were no.

@ojeda:

Yeah, that's not fine. I will send a fix for it, probably similar to our very long types where we write the full type to disk.

Thanks! However, I wouldn't write the list to disk -- I don't think it is useful to see the list, and we would need to ignore those files in .gitignore and so on.

@epage:

Maybe its just me but I feel like if the list of suggestions is too long, its just not worth doing anything with. I don't see it being likely someone is going to open a file with 29k options to find the exact one they wanted.

I will r+ this once we resolve the question of whether to use an environment variable or a -Z-flag to restore the prior behavior.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Feb 27, 2024

The team prefers -Z flags over environment variables for these sorts of things

@rustbot author

rustbot 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-review

Status: Awaiting review from the assignee but also interested parties.

labels

Feb 29, 2024

Flag added under -Zcheck-cfg-all-expected.

@rustbot ready

rustbot 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

Feb 29, 2024

📌 Commit 91322f4 has been approved by pnkfelix

It is now in the queue for this repository.

bors added the S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

label

Mar 4, 2024

jhpratt added a commit to jhpratt/rust that referenced this pull request

Mar 5, 2024

… r=pnkfelix

Limit the number of names and values in check-cfg diagnostics

The Rust for Linux [feedback](rust-lang#82450 (comment)) to the check-cfg Call for Testing, revealed a weakness in the check-cfg. They are unbounded and in the case RfL they have ~20k cfgs and having them printed (even once) is unbearable.

This PR limits it to 35 (28 rustc well known + feature + docsrs + 5 custom) which feels like a good middle ground for regular users (i.e. Cargo users).

When it goes over that limit print the N first with " and X more".

@rustbot label +F-check-cfg

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

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

Mar 5, 2024

rustbot 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

Mar 5, 2024

📌 Commit 9d9b26b has been approved by pnkfelix

It is now in the queue for this repository.

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

Mar 5, 2024

Urgau mentioned this pull request

Mar 5, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 5, 2024

… r=pnkfelix

Limit the number of names and values in check-cfg diagnostics

This PR limits it to 35 (28 rustc well known + feature + docsrs + 5 custom) which feels like a good middle ground for regular users (i.e. Cargo users).

When it goes over that limit print the N first with " and X more".

@rustbot label +F-check-cfg

This was referenced

Mar 5, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 6, 2024

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 6, 2024

Rollup merge of rust-lang#121202 - Urgau:check-cfg-limit-diagnostics, r=pnkfelix

Limit the number of names and values in check-cfg diagnostics

This PR limits it to 35 (28 rustc well known + feature + docsrs + 5 custom) which feels like a good middle ground for regular users (i.e. Cargo users).

When it goes over that limit print the N first with " and X more".

@rustbot label +F-check-cfg

ojeda added a commit to ojeda/linux that referenced this pull request

Apr 1, 2024

This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0 (i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

Unstable features

There have been no changes to the set of unstable features used in our own code. Therefore, the only unstable features allowed to be used outside the kernel crate is still new_uninit.

However, since we are finally dropping our alloc fork [3], all the unstable features used by alloc (~30 language ones, ~60 library ones) are not a concern anymore. This reduces the maintanance burden, increases the chances of new compiler versions working without changes and gets us closer to the goal of supporting several compiler versions.

It also means that, ignoring non-language/library features, we are currently left with just the few language features needed to implement the kernel Arc, the new_uninit library feature, the compiler_builtins marker and the few no_* cfgs we pass when compiling core/alloc.

Please see [4] for details.

Required changes

LLVM's data layout

Rust 1.77.0 (i.e. the previous upgrade) introduced a check for matching LLVM data layouts [5]. Then, Rust 1.78.0 upgraded LLVM's bundled major version from 17 to 18 [6], which changed the data layout in x86 [7]. Thus update the data layout in our custom target specification for x86 so that the compiler does not complain about the mismatch:

error: data-layout for target `target-5559158138856098584`,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
differs from LLVM target's `x86_64-linux-gnu` default layout,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

In the future, the goal is to drop the custom target specification files. Meanwhile, if we want to support other LLVM versions used in rustc (e.g. for LTO), we will need to add some extra logic (e.g. conditional on LLVM's version, or extracting the data layout from an existing built-in target specification).

`unused_imports`

Rust's unused_imports lint covers both unused and redudant imports. Now, in 1.78.0, the lint detects more cases of redundant imports [8]. Thus the previous commit cleaned them up.

Clippy's `new_without_default`

Clippy now suggests to implement Default even when new() is const, since Default::default() may call const functions even if it is not const itself [9]. Thus the previous commit added the implementation.

Other changes in Rust

Rust 1.78.0 introduces feature(asm_goto) [10] [11]. This feature was discussed in the past [12].

Rust 1.78.0 introduced support for mutable pointers to Rust statics, including a test case for the Linux kernel's VTABLE use case [13].

Rust 1.78.0 with debug assertions enabled (i.e. -Cdebug-assertions=y, kernel's CONFIG_RUST_DEBUG_ASSERTIONS=y) will now always check all unsafe preconditions, without a way to opt-out for particular cases [14].

Rust 1.78.0 also improved a couple issues we reported when giving feedback for the new --check-cfg feature [15] [16].

`alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update alloc anymore, since we are dropping our alloc fork [3].

As a bonus, even if that series is not applied, the new compiler release happens to build cleanly the existing alloc too (i.e. the previous version's).

ojeda added a commit to ojeda/linux that referenced this pull request

Apr 1, 2024

This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0 (i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

Unstable features

There have been no changes to the set of unstable features used in our own code. Therefore, the only unstable features allowed to be used outside the kernel crate is still new_uninit.

However, since we are finally dropping our alloc fork [3], all the unstable features used by alloc (~30 language ones, ~60 library ones) are not a concern anymore. This reduces the maintenance burden, increases the chances of new compiler versions working without changes and gets us closer to the goal of supporting several compiler versions.

Please see [4] for details.

Required changes

LLVM's data layout

error: data-layout for target `target-5559158138856098584`,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
differs from LLVM target's `x86_64-linux-gnu` default layout,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

`unused_imports`

Rust's unused_imports lint covers both unused and redundant imports. Now, in 1.78.0, the lint detects more cases of redundant imports [8]. Thus one of the previous patches cleaned them up.

Clippy's `new_without_default`

Clippy now suggests to implement Default even when new() is const, since Default::default() may call const functions even if it is not const itself [9]. Thus one of the previous patches implemented it.

Other changes in Rust

Rust 1.78.0 introduced feature(asm_goto) [10] [11]. This feature was discussed in the past [12].

Rust 1.78.0 introduced support for mutable pointers to Rust statics, including a test case for the Linux kernel's VTABLE use case [13].

Rust 1.78.0 with debug assertions enabled (i.e. -Cdebug-assertions=y, kernel's CONFIG_RUST_DEBUG_ASSERTIONS=y) now always checks all unsafe preconditions, without a way to opt-out for particular cases [14].

Rust 1.78.0 also improved a couple issues we reported when giving feedback for the new --check-cfg feature [15] [16].

`alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update alloc anymore, since we are dropping our alloc fork [3].

ojeda added a commit to ojeda/linux that referenced this pull request

Apr 1, 2024

This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0 (i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

Unstable features

There have been no changes to the set of unstable features used in our own code. Therefore, the only unstable features allowed to be used outside the kernel crate is still new_uninit.

However, since we are finally dropping our alloc fork [3], all the unstable features used by alloc (~30 language ones, ~60 library ones) are not a concern anymore. This reduces the maintenance burden, increases the chances of new compiler versions working without changes and gets us closer to the goal of supporting several compiler versions.

Please see [4] for details.

Required changes

LLVM's data layout

error: data-layout for target `target-5559158138856098584`,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
differs from LLVM target's `x86_64-linux-gnu` default layout,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

In the future, the goal is to drop the custom target specifications. Meanwhile, if we want to support other LLVM versions used in rustc (e.g. for LTO), we will need to add some extra logic (e.g. conditional on LLVM's version, or extracting the data layout from an existing built-in target specification).

`unused_imports`

Rust's unused_imports lint covers both unused and redundant imports. Now, in 1.78.0, the lint detects more cases of redundant imports [8]. Thus one of the previous patches cleaned them up.

Clippy's `new_without_default`

Other changes in Rust

Rust 1.78.0 introduced feature(asm_goto) [10] [11]. This feature was discussed in the past [12].

Rust 1.78.0 introduced support for mutable pointers to Rust statics, including a test case for the Linux kernel's VTABLE use case [13].

Rust 1.78.0 also improved a couple issues we reported when giving feedback for the new --check-cfg feature [15] [16].

`alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update alloc anymore, since we are dropping our alloc fork [3].

ojeda added a commit to ojeda/linux that referenced this pull request

May 5, 2024

This is the next upgrade to the Rust toolchain, from 1.77.1 to 1.78.0 (i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

It is much smaller than previous upgrades, since the alloc fork was dropped in commit 9d0441b ("rust: alloc: remove our fork of the alloc crate") [3].

Unstable features

There have been no changes to the set of unstable features used in our own code. Therefore, the only unstable features allowed to be used outside the kernel crate is still new_uninit.

However, since we finally dropped our alloc fork [3], all the unstable features used by alloc (~30 language ones, ~60 library ones) are not a concern anymore. This reduces the maintenance burden, increases the chances of new compiler versions working without changes and gets us closer to the goal of supporting several compiler versions.

Please see [4] for details.

Required changes

LLVM's data layout

error: data-layout for target `target-5559158138856098584`,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`,
differs from LLVM target's `x86_64-linux-gnu` default layout,
`e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

`unused_imports`

Rust's unused_imports lint covers both unused and redundant imports. Now, in 1.78.0, the lint detects more cases of redundant imports [8]. Thus one of the previous patches cleaned them up.

Clippy's `new_without_default`

Other changes in Rust

Rust 1.78.0 introduced feature(asm_goto) [10] [11]. This feature was discussed in the past [12].

Rust 1.78.0 introduced feature(const_refs_to_static) [13] to allow referencing statics in constants and extended feature(const_mut_refs) to allow raw mutable pointers in constants. Together, this should cover the kernel's VTABLE use case. In fact, the implementation [14] in upstream Rust added a test case for it [15].

Rust 1.78.0 with debug assertions enabled (i.e. -Cdebug-assertions=y, kernel's CONFIG_RUST_DEBUG_ASSERTIONS=y) now always checks all unsafe preconditions, though without a way to opt-out for particular cases [16]. It would be ideal to have a way to selectively disable certain checks per-call site for this one (i.e. not just per check but for particular instances of a check), even if the vast majority of the checks remain in place [17].

Rust 1.78.0 also improved a couple issues we reported when giving feedback for the new --check-cfg feature [18] [19].

`alloc` upgrade and reviewing

As mentioned above, compiler upgrades will not update alloc anymore, since we dropped our alloc fork [3].

Limit the number of names and values in check-cfg diagnostics by Urgau · Pull Request #121202 · rust-lang/rust (original) (raw)

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

Unstable features

Required changes

LLVM's data layout

unused_imports

Clippy's new_without_default

Other changes in Rust

alloc upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing

`unused_imports`

Clippy's `new_without_default`

`alloc` upgrade and reviewing