-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
add unreachable_cfg_select_predicates lint
#149960
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
Open
folkertdev
wants to merge
1
commit into
rust-lang:main
Choose a base branch
from
folkertdev:cfg-select-unreachable-lint
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+146
−51
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| #![warn(unreachable_cfg_select_predicates)] | ||
| //~^ WARN unknown lint: `unreachable_cfg_select_predicates` | ||
|
|
||
| cfg_select! { | ||
| //~^ ERROR use of unstable library feature `cfg_select` | ||
| _ => {} | ||
| // With the feature enabled, this branch would trip the unreachable_cfg_select_predicate lint. | ||
| true => {} | ||
| } | ||
|
|
||
| fn main() {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| error[E0658]: use of unstable library feature `cfg_select` | ||
| --> $DIR/feature-gate-cfg-select.rs:4:1 | ||
| | | ||
| LL | cfg_select! { | ||
| | ^^^^^^^^^^ | ||
| | | ||
| = note: see issue #115585 <https://github.com/rust-lang/rust/issues/115585> for more information | ||
| = help: add `#![feature(cfg_select)]` to the crate attributes to enable | ||
| = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date | ||
|
|
||
| warning: unknown lint: `unreachable_cfg_select_predicates` | ||
| --> $DIR/feature-gate-cfg-select.rs:1:9 | ||
| | | ||
| LL | #![warn(unreachable_cfg_select_predicates)] | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: the `unreachable_cfg_select_predicates` lint is unstable | ||
| = note: see issue #115585 <https://github.com/rust-lang/rust/issues/115585> for more information | ||
| = help: add `#![feature(cfg_select)]` to the crate attributes to enable | ||
| = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date | ||
| = note: `#[warn(unknown_lints)]` on by default | ||
|
|
||
| error: aborting due to 1 previous error; 1 warning emitted | ||
|
|
||
| For more information about this error, try `rustc --explain E0658`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to warn, unless there is a wildcard, why so? Does it not warn in case of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, it's hard to do better than that. we'd need to actually have some sort of logic solver to do so in general, and even then I think it might misfire if there is some sort of feature flag implication that the checker does not know about.
So the current imlementation is basic but reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have similar checks for unreachable match arms, so it can't be that bad, right?
I'm really concerned that this lint in its current form is just misleading. I think a user that sees
#![deny(unreachable_cfg_select_predicates)will assume that rust will detect all unreachable predicates (since rust is normally reliable like that), and then will not notice that there is an unreachable predicate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems feasible, but it really is adding a SMT solver, so, not straightforward.
https://doc.rust-lang.org/beta/nightly-rustc/rustc_hir/attrs/data_structures/enum.CfgEntry.html
We currently store
Meaning that e.g. for
target_endianwe'd need to encode that covering big/little is exhaustive. We need target feature implications to be taken into account, and cargo feature implication to be taken into account. This all seems useful, but it's complex. I'd rather not blockcfg_select!on it.@traviscross do you have more details of what T-lang had in mind here (and whether we want to block
cfg_select!itself on that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I do not have more details on hand. I've noted the question, though, and we'll cover this when we pick up the nomination on #149783.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for you. I've been working on a minimal SAT solver that fits in around 600 lines of Rust code (while still being impressively fast). Let's say we had this. How much of the problem is that, and how much of the problem is encoding the needed invariants in CNF? I.e., if we had a good solver, is this still hard due to e.g. not knowing what we need to know in the right place to add the clauses, needing hard circuits such as for string operations, etc.? Or, if we had a solver, would it be straightforward to encode the problem in CNF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly have no idea, this is not really my area of expertise.
The
NameValuevariant might need some design around when that is exhaustive.Versionseems hard, maybe that is just never exhaustive?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, a solver is going to offer an interface like this:
That is, we have a set of$N$ boolean variables. We use "literals" to refer to them: literal
1asserts that a variable is true; literal-1asserts that it is false.A clause is a set of literals that are ORed together (e.g.,
[1, -2]meansv1 OR !v2). The solver ensures all added clauses are true.If the formula is satisfiable (
SAT), the solver returns an assignment of each variable. If it's not, it returnsUNSAT.The game is to assert all of the predicates of the earlier
cfg_selectarms as false (negated) and the predicate of the current arm as true. If it'sSAT, then the arm is reachable. If it'sUNSAT, then it's not.We'd need to map every unique atomic
cfgpredicate (e.g.,unix,target_pointer_width="32") to a unique SAT variable index. The tooling with the solver then provides a mechanism for encoding arbitrary boolean trees (e.g.not(all(a, any(b, c)))) into the right form (using a Tseitin transformation).One design question is whether it'd be straightforward to produce the needed mapping and clauses.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and we'd want to think about what we expect of the interaction with
--cfg-check.Sounds right. We'd give each version a variable, then assert that at most one of these can be true.