Improve check-cfg implementation #111068
Merged
Improve check-cfg implementation #111068
Conversation
This file contains 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
rustbot
added
S-waiting-on-review
Urgau
force-pushed
the
check-cfg-improvements
branch
from
6fede2c
to
113cd27
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Urgau
force-pushed
the
check-cfg-improvements
branch
from
113cd27
to
9c76d9a
Compare
petrochenkov
reviewed
May 3, 2023
petrochenkov
added
S-waiting-on-author
Urgau
force-pushed
the
check-cfg-improvements
branch
3 times, most recently
from
3612ab3
to
7962aa0
Compare
|
Fixed and responded to all the review comments. @rustbot ready |
rustbot
added
S-waiting-on-review
petrochenkov
reviewed
May 5, 2023
|
r=me with #111068 (comment) addressed. |
rustbot
added
S-waiting-on-author
This is done to simplify to relationship between names() and values() but also make thing clearer (having an Any to represent that any values are allowed) but also to allow the (none) + values expected cases that wasn't possible before.
Urgau
force-pushed
the
check-cfg-improvements
branch
from
7962aa0
to
5364784
Compare
|
I don't have bors rights @rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r=petrochenkov |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 5, 2023
Rollup of 6 pull requests Successful merges: - rust-lang#103056 (Fix `checked_{add,sub}_duration` incorrectly returning `None` when `other` has more than `i64::MAX` seconds) - rust-lang#108801 (Implement RFC 3348, `c"foo"` literals) - rust-lang#110773 (Reduce MIR dump file count for MIR-opt tests) - rust-lang#110876 (Added default target cpu to `--print target-cpus` output and updated docs) - rust-lang#111068 (Improve check-cfg implementation) - rust-lang#111238 (btree_map: `Cursor{,Mut}::peek_prev` must agree) Failed merges: - rust-lang#110694 (Implement builtin # syntax and use it for offset_of!(...)) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 17, 2023
…ochenkov Add new simpler and more explicit syntax for check-cfg <details> <summary> Old proposition (before the MCP) </summary> This PR adds a new simpler and more explicit syntax for check-cfg. It consist of two new form: - `exhaustive(names, values)` - `configure(name, "value1", "value2", ... "valueN")` The preview forms `names(...)` and `values(...)` have implicit meaning that are not strait-forward. In particular `values(foo)`&`values(bar)` and `names(foo, bar)` are not equivalent which has created [some confusions](rust-lang#98080). Also the `names()` and `values()` form are not clear either and again created some confusions where peoples believed that `values()`&`values(foo)` could be reduced to just `values(foo)`. To fix that the two new forms are made to be explicit and simpler. See the table of correspondence: - `names()` -> `exhaustive(names)` - `values()` -> `exhaustive(values)` - `names(foo)` -> `exhaustive(names)`&`configure(foo)` - `values(foo)` -> `configure(foo)` - `values(feat, "foo", "bar")` -> `configure(feat, "foo", "bar")` - `values(foo)`&`values(bar)` -> `configure(foo, bar)` - `names()`&`values()`&`values(my_cfg)` -> `exhaustive(names, values)`&`configure(my_cfg)` Another benefits of the new syntax is that it allow for further options (like conditional checking for --cfg, currently always on) without syntax change. The two previous forms are deprecated and will be removed once cargo and beta rustc have the necessary support. </details> This PR is the first part of the implementation of [MCP636 - Simplify and improve explicitness of the check-cfg syntax](rust-lang/compiler-team#636). ## New `cfg` form It introduces the new [`cfg` form](rust-lang/compiler-team#636) and deprecate the other two: ``` rustc --check-cfg 'cfg(name1, ..., nameN, values("value1", "value2", ... "valueN"))' ``` ## Default built-in names and values It also changes the default for the built-in names and values checking. - Built-in values checking would always be activated as long as a `--check-cfg` argument is present - Built-in names checking would always be activated as long as a `--check-cfg` argument is present **unless** if any `cfg(any())` arg is passed ~~**Note: depends on rust-lang#111068 but is reviewable (last two commits)!**~~ Resolve rust-lang/compiler-team#636 r? `@petrochenkov`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 17, 2023
Rollup merge of rust-lang#111072 - Urgau:check-cfg-new-syntax, r=petrochenkov Add new simpler and more explicit syntax for check-cfg <details> <summary> Old proposition (before the MCP) </summary> This PR adds a new simpler and more explicit syntax for check-cfg. It consist of two new form: - `exhaustive(names, values)` - `configure(name, "value1", "value2", ... "valueN")` The preview forms `names(...)` and `values(...)` have implicit meaning that are not strait-forward. In particular `values(foo)`&`values(bar)` and `names(foo, bar)` are not equivalent which has created [some confusions](rust-lang#98080). Also the `names()` and `values()` form are not clear either and again created some confusions where peoples believed that `values()`&`values(foo)` could be reduced to just `values(foo)`. To fix that the two new forms are made to be explicit and simpler. See the table of correspondence: - `names()` -> `exhaustive(names)` - `values()` -> `exhaustive(values)` - `names(foo)` -> `exhaustive(names)`&`configure(foo)` - `values(foo)` -> `configure(foo)` - `values(feat, "foo", "bar")` -> `configure(feat, "foo", "bar")` - `values(foo)`&`values(bar)` -> `configure(foo, bar)` - `names()`&`values()`&`values(my_cfg)` -> `exhaustive(names, values)`&`configure(my_cfg)` Another benefits of the new syntax is that it allow for further options (like conditional checking for --cfg, currently always on) without syntax change. The two previous forms are deprecated and will be removed once cargo and beta rustc have the necessary support. </details> This PR is the first part of the implementation of [MCP636 - Simplify and improve explicitness of the check-cfg syntax](rust-lang/compiler-team#636). ## New `cfg` form It introduces the new [`cfg` form](rust-lang/compiler-team#636) and deprecate the other two: ``` rustc --check-cfg 'cfg(name1, ..., nameN, values("value1", "value2", ... "valueN"))' ``` ## Default built-in names and values It also changes the default for the built-in names and values checking. - Built-in values checking would always be activated as long as a `--check-cfg` argument is present - Built-in names checking would always be activated as long as a `--check-cfg` argument is present **unless** if any `cfg(any())` arg is passed ~~**Note: depends on rust-lang#111068 but is reviewable (last two commits)!**~~ Resolve rust-lang/compiler-team#636 r? `@petrochenkov`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 17, 2024
…mpty, r=petrochenkov Add way to express that no values are expected with check-cfg This PR adds way to express no-values (no values expected) with `--check-cfg` by making empty `values()` no longer mean `values(none())` (internal: `&[None]`) and now be an empty list (internal: `&[]`). ### Context Currently `--check-cfg` has a way to express that _any value is expected_ with `values(any())`, but has no way to do the inverse and say that _no value is expected_. This would be particularly useful for build systems that control a config name and it's values as they could always declare a config name as expected and if in the current state they have values pass them and if not pass an empty list. To give a more concrete example, Cargo `--check-cfg` currently needs to generate: - `--check-cfg=cfg(feature, values(...))` for the case with declared features - and `--check-cfg=cfg()` for the case without any features declared This means that when there are no features declared, users will get an `unexpected config name` but from the point of view of Cargo the config name `feature` is expected, it's just that for now there aren't any values for it. See [Cargo `check_cfg_args` function](https://github.com/rust-lang/cargo/blob/92395d90106b3b61bcb68bcf2069052c93771764/src/cargo/core/compiler/mod.rs#L1263-L1281) for more details. ### De-specializing *empty* `values()` To solve this issue I propose that we "de-specialize" empty `values()` to no longer mean `values(none())` but to actually mean empty set/list. This is one of the last source of confusion for my-self and others with the `--check-cfg` syntax. > The confusing part here is that an empty `values()` currently means the same as `values(none())`, i.e. an expected list of values with the _none_ variant (as in `#[cfg(name)]` where the value is none) instead of meaning an empty set. Before the new `cfg()` syntax, defining the _none_ variant was only possible under certain circumstances, so in rust-lang#111068 I decided to make `values()` to mean the _none_ variant, but it is no longer necessary since rust-lang#119473 which introduced the `none()` syntax. A simplified representation of the proposed "de-specialization" would be: | Syntax | List/set of expected values | |-----------------------------------------|-----------------------------| | `cfg(name)`/`cfg(name, values(none()))` | `&[None]` | | `cfg(name, values())` | `&[]` | Note that I have my-self made the mistake of using an empty `values()` as meaning empty set, see rust-lang/cargo#13011. `@rustbot` label +F-check-cfg r? `@petrochenkov` cc `@epage`
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Jan 18, 2024
…etrochenkov Add way to express that no values are expected with check-cfg This PR adds way to express no-values (no values expected) with `--check-cfg` by making empty `values()` no longer mean `values(none())` (internal: `&[None]`) and now be an empty list (internal: `&[]`). ### Context Currently `--check-cfg` has a way to express that _any value is expected_ with `values(any())`, but has no way to do the inverse and say that _no value is expected_. This would be particularly useful for build systems that control a config name and it's values as they could always declare a config name as expected and if in the current state they have values pass them and if not pass an empty list. To give a more concrete example, Cargo `--check-cfg` currently needs to generate: - `--check-cfg=cfg(feature, values(...))` for the case with declared features - and `--check-cfg=cfg()` for the case without any features declared This means that when there are no features declared, users will get an `unexpected config name` but from the point of view of Cargo the config name `feature` is expected, it's just that for now there aren't any values for it. See [Cargo `check_cfg_args` function](https://github.com/rust-lang/cargo/blob/92395d90106b3b61bcb68bcf2069052c93771764/src/cargo/core/compiler/mod.rs#L1263-L1281) for more details. ### De-specializing *empty* `values()` To solve this issue I propose that we "de-specialize" empty `values()` to no longer mean `values(none())` but to actually mean empty set/list. This is one of the last source of confusion for my-self and others with the `--check-cfg` syntax. > The confusing part here is that an empty `values()` currently means the same as `values(none())`, i.e. an expected list of values with the _none_ variant (as in `#[cfg(name)]` where the value is none) instead of meaning an empty set. Before the new `cfg()` syntax, defining the _none_ variant was only possible under certain circumstances, so in rust-lang/rust#111068 I decided to make `values()` to mean the _none_ variant, but it is no longer necessary since rust-lang/rust#119473 which introduced the `none()` syntax. A simplified representation of the proposed "de-specialization" would be: | Syntax | List/set of expected values | |-----------------------------------------|-----------------------------| | `cfg(name)`/`cfg(name, values(none()))` | `&[None]` | | `cfg(name, values())` | `&[]` | Note that I have my-self made the mistake of using an empty `values()` as meaning empty set, see rust-lang/cargo#13011. `@rustbot` label +F-check-cfg r? `@petrochenkov` cc `@epage`
lnicola
pushed a commit
to lnicola/rust-analyzer
that referenced
this pull request
Apr 7, 2024
…etrochenkov Add way to express that no values are expected with check-cfg This PR adds way to express no-values (no values expected) with `--check-cfg` by making empty `values()` no longer mean `values(none())` (internal: `&[None]`) and now be an empty list (internal: `&[]`). ### Context Currently `--check-cfg` has a way to express that _any value is expected_ with `values(any())`, but has no way to do the inverse and say that _no value is expected_. This would be particularly useful for build systems that control a config name and it's values as they could always declare a config name as expected and if in the current state they have values pass them and if not pass an empty list. To give a more concrete example, Cargo `--check-cfg` currently needs to generate: - `--check-cfg=cfg(feature, values(...))` for the case with declared features - and `--check-cfg=cfg()` for the case without any features declared This means that when there are no features declared, users will get an `unexpected config name` but from the point of view of Cargo the config name `feature` is expected, it's just that for now there aren't any values for it. See [Cargo `check_cfg_args` function](https://github.com/rust-lang/cargo/blob/92395d90106b3b61bcb68bcf2069052c93771764/src/cargo/core/compiler/mod.rs#L1263-L1281) for more details. ### De-specializing *empty* `values()` To solve this issue I propose that we "de-specialize" empty `values()` to no longer mean `values(none())` but to actually mean empty set/list. This is one of the last source of confusion for my-self and others with the `--check-cfg` syntax. > The confusing part here is that an empty `values()` currently means the same as `values(none())`, i.e. an expected list of values with the _none_ variant (as in `#[cfg(name)]` where the value is none) instead of meaning an empty set. Before the new `cfg()` syntax, defining the _none_ variant was only possible under certain circumstances, so in rust-lang/rust#111068 I decided to make `values()` to mean the _none_ variant, but it is no longer necessary since rust-lang/rust#119473 which introduced the `none()` syntax. A simplified representation of the proposed "de-specialization" would be: | Syntax | List/set of expected values | |-----------------------------------------|-----------------------------| | `cfg(name)`/`cfg(name, values(none()))` | `&[None]` | | `cfg(name, values())` | `&[]` | Note that I have my-self made the mistake of using an empty `values()` as meaning empty set, see rust-lang/cargo#13011. `@rustbot` label +F-check-cfg r? `@petrochenkov` cc `@epage`
RalfJung
pushed a commit
to RalfJung/rust-analyzer
that referenced
this pull request
Apr 27, 2024
…etrochenkov Add way to express that no values are expected with check-cfg This PR adds way to express no-values (no values expected) with `--check-cfg` by making empty `values()` no longer mean `values(none())` (internal: `&[None]`) and now be an empty list (internal: `&[]`). ### Context Currently `--check-cfg` has a way to express that _any value is expected_ with `values(any())`, but has no way to do the inverse and say that _no value is expected_. This would be particularly useful for build systems that control a config name and it's values as they could always declare a config name as expected and if in the current state they have values pass them and if not pass an empty list. To give a more concrete example, Cargo `--check-cfg` currently needs to generate: - `--check-cfg=cfg(feature, values(...))` for the case with declared features - and `--check-cfg=cfg()` for the case without any features declared This means that when there are no features declared, users will get an `unexpected config name` but from the point of view of Cargo the config name `feature` is expected, it's just that for now there aren't any values for it. See [Cargo `check_cfg_args` function](https://github.com/rust-lang/cargo/blob/92395d90106b3b61bcb68bcf2069052c93771764/src/cargo/core/compiler/mod.rs#L1263-L1281) for more details. ### De-specializing *empty* `values()` To solve this issue I propose that we "de-specialize" empty `values()` to no longer mean `values(none())` but to actually mean empty set/list. This is one of the last source of confusion for my-self and others with the `--check-cfg` syntax. > The confusing part here is that an empty `values()` currently means the same as `values(none())`, i.e. an expected list of values with the _none_ variant (as in `#[cfg(name)]` where the value is none) instead of meaning an empty set. Before the new `cfg()` syntax, defining the _none_ variant was only possible under certain circumstances, so in rust-lang/rust#111068 I decided to make `values()` to mean the _none_ variant, but it is no longer necessary since rust-lang/rust#119473 which introduced the `none()` syntax. A simplified representation of the proposed "de-specialization" would be: | Syntax | List/set of expected values | |-----------------------------------------|-----------------------------| | `cfg(name)`/`cfg(name, values(none()))` | `&[None]` | | `cfg(name, values())` | `&[]` | Note that I have my-self made the mistake of using an empty `values()` as meaning empty set, see rust-lang/cargo#13011. `@rustbot` label +F-check-cfg r? `@petrochenkov` cc `@epage`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR makes multiple improvements into the implementation of check-cfg, it is a prerequisite to a follow-up PR that will introduce a simpler and more explicit syntax.
The 2 main area of improvements are:
FxHashSet<Option<Symbol>>instead ofFxHashSet<Symbol>, it made the no value expected case only possible when no values where in theHashSetwhich is now represented asNone(same as cfg represent-it).SomeandAnymakes it now clear if some values are expected or not, necessary forfeatureandtarget_feature.I highly recommend reviewing commit by commit.
r? @petrochenkov