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

Add fuzzing and loom to well-known cfgs #124804

Closed
wants to merge 1 commit into from

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented May 6, 2024

Just like miri, the cfgs fuzzing and loom are used for ecosystem-wide coordination. These are by definition well-known, so they should be included in the list.

@rustbot
Copy link
Collaborator

rustbot commented May 6, 2024

r? @Nadrieril

rustbot has assigned @Nadrieril.
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 May 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 6, 2024

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

Some changes occurred in src/doc/rustc/src/check-cfg.md

cc @Urgau

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@Nadrieril
Copy link
Member

Implementation looks good. I don't know the policy for such changes though. @Urgau you seem to know about this :D wdyt?

@Urgau
Copy link
Member

Urgau commented May 6, 2024

I have mixed feelings about adding those, not necessarily the same for both.

First this would diverge from the T-compiler FCP of the feature where the rule that was proposed was:

  1. User facing (nightly or not) (rational: internal cfgs are by their name not to be shown)
  2. Set by a Rust Toolchain tool (rational: rustc is not the only tool that sets cfgs, rustdoc sets doc and doctest, cargo-miri sets miri, docs.rs sets docsrs, ...)
  3. Must be an immutable set of names and values (rational: if it's not immutable, rustc cannot possibly know the what to add; this excludes Cargo feature cfg)

As far as I know neither of them are set by any Rust Toolchain tool, failing the second point.

Secondly those two cfgs are very specific are IMO not very well known by the broad community and including them by-default for everyone is a really high bar that I don't think neither of them quite reach.

Thirdly they seems both related to crates.io (maybe fuzzing a bit less, but loom definitely) and including them in rustc seems very weird for projects that don't use crates.io or Cargo at all, like the Rust for Linux project.

Fourthly including loom would give some kind of "recommendation" (even if not intended) and would make the project liable for another project that we don't control.

However it seems that there is some desire in having them as well known cfgs, so I could be sold on having them as compatibility cfg (so no other ecosystem cfgs in the future).

One thing that would help with my first and second point, would be to make them Cargo well known cfgs (instead of rustc well known), like docsrs is.

@rustbot label -S-waiting-on-review +S-waiting-on-author +S-blocked
cc @epage


As for the implementation in this PR it's missing two anti-regressions entries in well-known-values.rs.

@rustbot 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 May 6, 2024
@epage
Copy link
Contributor

epage commented May 6, 2024

Based on the conversation in #124800, I think having a PR up is premature.

@saethlin
Copy link
Member Author

saethlin commented May 6, 2024

As far as I know neither of them are set by any Rust Toolchain tool, failing the second point.

The point being raised in issues is that adding a build script to correct the false positives is an undue burden on the ecosystem, because every maintainer separately has to teach Cargo/rustc about all the well-known ecosystem cfgs that they use. It is entirely unclear to me if this was FCP'd with that concern in mind. I certainly can't find any evidence that it was discussed, so it seems rather dubious to point at the FCP here.

I'm happy to bring this up in a compiler team meeting, unless someone can point me to where the compiler team already discussed this concern.

And I see @workingjubilee has voiced the same feedback as I have, independently #124800 (comment)

@epage
Copy link
Contributor

epage commented May 6, 2024

imo this is a discussion better had on an Issue, rather than a PR

@rustbot rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label May 7, 2024
tbu- added a commit to tbu-/rust that referenced this pull request May 7, 2024
This allows to find solutions to the false positives that were found in
the ecosystem before turning it to `warn` by default again.

Most projects hit by this seem to just disable the warning, which
indicates that it isn't working as expected.

CC rust-lang#124800
CC rust-lang#124804
CC rust-lang#124821
CC hyperium/hyper#3660
CC microsoft/windows-rs#3022
CC rust-bitcoin/rust-bitcoin#2748
CC tokio-rs/tokio#6538
@saethlin
Copy link
Member Author

One thing that would help with my first and second point, would be to make them Cargo well known cfgs

Closing because of this. I agree. I might do a similar PR to Cargo later.

@saethlin saethlin closed this May 12, 2024
@saethlin saethlin deleted the fuzzing-is-well-known branch May 12, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

5 participants