Additionally gate negative bounds behind new -Ztrait-solver-testing-features#156212
Additionally gate negative bounds behind new -Ztrait-solver-testing-features#156212fmease wants to merge 1 commit intorust-lang:mainfrom
-Ztrait-solver-testing-features#156212Conversation
rustbot
added
S-waiting-on-review
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| track_diagnostics: bool = (false, parse_bool, [UNTRACKED], | ||
| "tracks where in rustc a diagnostic was emitted"), | ||
| trait_solver_testing_features: bool = (false, parse_bool, [TRACKED], | ||
| "allow certain internal language features to be enabled that help exercise & test the trait solver"), |
There was a problem hiding this comment.
it's "allow […] to be enabled" not "allow" since it doesn't automatically allow negative bounds as they're still gated behind internal feature negative_bounds; it merely allows you to allow negative_bounds.
| track_diagnostics: bool = (false, parse_bool, [UNTRACKED], | ||
| "tracks where in rustc a diagnostic was emitted"), | ||
| trait_solver_testing_features: bool = (false, parse_bool, [TRACKED], | ||
| "allow certain internal language features to be enabled that help exercise & test the trait solver"), |
There was a problem hiding this comment.
I'm not saying "trait solvers", plural, so this description doesn't need to be updated when the next solver supersedes the old one.
There was a problem hiding this comment.
If y'all disagree with the hypothesis and the solution, please voice your concern! I'm fine with retracting this PR if it's not liked.
This seems fine to me: simple and reasonable. But I'm not an expert on feature (stable and unstable). Do you want to wait for (or request) feedback from additional people?
| if !visitor.features.negative_bounds() { | ||
| // Negative bounds are *super* internal. We require `-Ztrait-solver-testing-features` *and* | ||
| // `#![feature(negative_bounds)]` to prevent proliferation. Under no circumstances do we | ||
| // want to advertise the zee flag and the feature name to users! |
There was a problem hiding this comment.
"zee flag" caught me off guard. I've never seen it used before, and I don't see any other examples like it in the codebase. Just drop the "zee" so it says "the flag"?
3 participants
Support for negative bounds (not to be confused with negative impls) was added in PR RUST-110791 (2023) for internal testing purposes only.
In PR RUST-119354 (2023) I marked it internal. Moreover, we intentionally never advertise this feature and we declare it as an internal-only feature for testing the trait solver in the Unstable Book (via).
However, these measures didn't prevent someone from trying to use them in the public API of the stdlib1! I can't blame them in the slightest: The stdlib uses unstable and internal features left and right, even in stable APIs. What's more, there's the sibling feature negative impls that obviously bears a similar name, is not internal and is used in stable APIs. I can see that it's easy to miss that their status differs so drastically.
Well, I have to admit, it's unlikely that such a PR would ever pass review. However, there's a chance it might, I don't know. Last time it was me who caught it.
I argue that "better docs" just wouldn't really help, a tidy rule would be too much effort & be too fragile and triagebot mentions would be the wrong tool for the job (see also) even if their functionality got extended.
Therefore I've decided to additionally gate negative bounds behind a new
-Ztrait-solver-testing-featuresflag. This should be deterrent enough since surely nobody would modify the build flags for the stdlib I'm thinking and the name should scream "I'm unfit for general use".If y'all disagree with the hypothesis and the solution, please voice your concern! I'm fine with retracting this PR if it's not liked.
Footnotes
This should go without saying but don't put any blame on individuals! Context: https://github.com/rust-lang/rust/pull/146668#issuecomment-3303881805. ↩