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 new simpler and more explicit syntax for check-cfg #111072

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented May 1, 2023

Old proposition (before the MCP)

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.

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.

This PR is the first part of the implementation of MCP636 - Simplify and improve explicitness of the check-cfg syntax.

New cfg form

It introduces the new cfg form 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 #111068 but is reviewable (last two commits)!

Resolve rust-lang/compiler-team#636

r? @petrochenkov

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 1, 2023

Error: Parsing relabel command in comment failed: ...'-on-author' | error: a label delta at >| ' (for the'...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 1, 2023
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the check-cfg-new-syntax branch from a01a326 to 3b54cec Compare May 1, 2023 17:02
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the check-cfg-new-syntax branch from 3b54cec to e072419 Compare May 1, 2023 17:12
@petrochenkov
Copy link
Contributor

Blocked on #111068.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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 May 2, 2023
@bors

This comment was marked as resolved.

@Urgau Urgau force-pushed the check-cfg-new-syntax branch from e072419 to 6846f33 Compare May 5, 2023 19:54
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 5, 2023
@petrochenkov
Copy link
Contributor

How can exhaustive(values) be a single flag?
Isn't value checking enabled and disabled per-name?
I.e. we can check values for name foo, and not check values for name bar.
Or does it only affect cfgs built into the compiler?

@petrochenkov
Copy link
Contributor

petrochenkov commented May 10, 2023

Does --check-cfg exhaustive(names, values) basically mean --check-cfg builtin(names, values)?
What does --check-cfg exhaustive(values) mean without names?

UPD: Looks like --check-cfg exhaustive(values) does mean --check-cfg builtin(values), but --check-cfg exhaustive(names) does not.

@petrochenkov
Copy link
Contributor

With this new design we cannot specify a name as known, while keeping its list of values unknown, right?

Previously names(foo) didn't restrict the set of values for foo, but exhaustive(names) & configure(foo) does restrict it to "no values".

@Urgau
Copy link
Member Author

Urgau commented May 10, 2023

How can exhaustive(values) be a single flag? Isn't value checking enabled and disabled per-name? I.e. we can check values for name foo, and not check values for name bar. Or does it only affect cfgs built into the compiler?

exhaustive(values) works in the same way as values(), it adds (nearly) all the builtin cfgs with their expected values into the expected list. That means that all the builtin cfgs will lint if there is an unexpected value and that's it.
It doesn't change anything for non-builtin cfgs.

Remember that we can only lint on values for which we have an entry for, if we don't have one then it's not a value problem but a name problem and this is gated under exhaustive(names).

Does --check-cfg exhaustive(names, values) basically mean --check-cfg builtin(names, values)?

Not exactly, for values yes, it's an append of the builtin values. But for names it's not only appending the list of builtin names but also enabling the checking of all names; otherwise we wouldn't be able to check names since we would always trigger on builtin names.

What does --check-cfg exhaustive(values) mean without names?

Enable checking of the builtin values without triggering on potentially unexpected cfg.

@petrochenkov petrochenkov 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 10, 2023
@Urgau
Copy link
Member Author

Urgau commented May 10, 2023

With this new design we cannot specify a name as known, while keeping its list of values unknown, right?

Yes, this is the only "regression" of the new design, but I've yet to found a example where this would be useful. The only place where it is useful is in the compiler for feature and target_feature where getting the expected values is non trivial.

If a consumer of rustc doesn't know the expected values of a config, how would a actual user know which one to use? Or even how does the consumer of rustc know which one to set via --cfg?

Previously names(foo) didn't restrict the set of values for foo, but exhaustive(names) & configure(foo) does restrict it to "no values".

Btw, no need for exhaustive(names) here, doing configure(foo) no matter if exhaustive(names) is set, set the expected values to none.

@rustbot ready

@rustbot 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 May 10, 2023
@petrochenkov
Copy link
Contributor

So what we really need to cover all cases without regressions (without any syntax beautifications) is:

  • --check-cfg builtin-values - will be commonly used if we expect full checking to be typically used.
  • --check-cfg custom-values(noval, oneval, "val1", twovals, "val1", "val2") - will be commonly used if we expect full checking to be typically used.
  • --check-cfg exhaustive-names(foo, bar) - doesn't put value restrictions on foo and bar, --check-cfg exhaustive-names() without arguments will be commonly used if we expect full checking to be typically used, the form with arguments - not so commonly.

@petrochenkov
Copy link
Contributor

In that case we probably need some short syntax for the commonly used combination --check-cfg builtin-values --check-cfg exhaustive-names() --check-cfg custom-values(noval, oneval, "val1", twovals, "val1", "val2") + some additional rarely used options for opting out of exhaustiveness and adding names without value requirements.

@Urgau
Copy link
Member Author

Urgau commented May 10, 2023

So what we really need to cover all cases without regressions (without any syntax beautifications) is:

  • --check-cfg builtin-values - will be commonly used if we expect full checking to be typically used.

  • --check-cfg custom-values(noval, oneval, "val1", twovals, "val1", "val2") - will be commonly used if we expect full checking to be typically used.

Agree for these two.

  • --check-cfg exhaustive-names(foo, bar) - doesn't put value restrictions on foo and bar,

As I said, I don't think this is useful for users, as users knows all the possible values. Otherwise they couldn't possible use cfgs in any meaningful way.

--check-cfg exhaustive-names() without arguments will be commonly used if we expect full checking to be typically used, the form with arguments - not so commonly.

👍 (but note that there isn't currently any plan to enable this by default in cargo)

In that case we probably need some short syntax for the commonly used combination --check-cfg builtin-values --check-cfg exhaustive-names() --check-cfg custom-values(noval, oneval, "val1", twovals, "val1", "val2") + some additional rarely used options for opting out of exhaustiveness and adding names without value requirements.

If cargo were to enable all things, it would be with my syntax exhaustive(names, values)&configure(feature, ...) set by cargo + users defined (but cannot really do anything about them).

@rustbot rustbot removed S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 17, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2023

📌 Commit fc78d78 has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 Oct 17, 2023
@WaffleLapkin
Copy link
Member

WaffleLapkin commented Oct 17, 2023

@Urgau is the syntax in the OP still up to date? (I don't immediately get what the syntax means, specifically the values(...) so I'm asking to make sure) And is this feature gated? Or is it insta-stable?

@Urgau
Copy link
Member Author

Urgau commented Oct 17, 2023

Urgau is the syntax in the OP still up to date? (I don't immediately get what the syntax means, specifically the values(...) so I'm asking to make sure)

Yes, the PR description, the commit message and the MCP are all up to date.
Regarding values(...) (I assume you mean in cfg(...) and not the old syntax), it's a designator to separate the names previously given and the values associated with them. In other words, it's like a key=value pair, where both the names (key) and the values are de-duplicated.

For more information, I would highly suggest reading the check-cfg section of the rustc unstable book.

And is this feature gated? Or is it insta-stable?

It's a pre-existing unstable feature, so yes it's gated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#111072 (Add new simpler and more explicit syntax for check-cfg)
 - rust-lang#116717 (Special case iterator chain checks for suggestion)
 - rust-lang#116719 (Add MonoItems and Instance to stable_mir)
 - rust-lang#116787 (Implement an internal lint encouraging use of `Span::eq_ctxt`)
 - rust-lang#116827 (Make `handle_options` public again.)

r? `@ghost`
`@rustbot` modify labels: rollup
@WaffleLapkin
Copy link
Member

(Sorry for commenting on details after MCP was accepted and PR approved...) For me it looks weird that values() is it's own thing. From the syntax it looks like an element just like feature names, not a thing on a different level. I'd expect something more like name = ["value", "value2"] or name("value", "value2"), rather than name, values("value", value2), again, because name and values here don't live on the same level, they are not elements of the same list.

Also the documentation seems to have a configure in an example, which looks like a mistake?

Anyway, thanks for answering my questions.

@bors bors merged commit ce40742 into rust-lang:master Oct 17, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 17, 2023
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-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 18, 2023
57: Pull upstream master 2023 10 18 r=pietroalbini a=Veykril

* rust-lang/rust#116505
* rust-lang/rust#116840
* rust-lang/rust#116767
* rust-lang/rust#116855
  * rust-lang/rust#116827
  * rust-lang/rust#116787
  * rust-lang/rust#116719
  * rust-lang/rust#116717
  * rust-lang/rust#111072
* rust-lang/rust#116844
* rust-lang/rust#115577
* rust-lang/rust#116756
* rust-lang/rust#116518



Co-authored-by: Urgau <[email protected]>
Co-authored-by: Esteban Küber <[email protected]>
Co-authored-by: Deadbeef <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: Camille GILLOT <[email protected]>
Co-authored-by: Celina G. Val <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: Arthur Lafrance <[email protected]>
Co-authored-by: Nikolay Arhipov <[email protected]>
Co-authored-by: Nikita Popov <[email protected]>
Co-authored-by: bors <[email protected]>
@Urgau
Copy link
Member Author

Urgau commented Oct 18, 2023

values(...) being it's "own" thing is on purpose, cf. https://hackmd.io/@wesleywiser/rJlumbfep:

  1. I think we should specifically denote the values. Perhaps cfg(feature, values(foo, bar)). No values would be spelled cfg(feature) or cfg(feature, values()). This makes it easer to extend the constraints we can express in the future (for example cfg(target_pointer_width, one_of(16, 32, 64))).

btw, if you wish to continue discussing it let's do that on Zulip


Also the documentation seems to have a configure in an example, which looks like a mistake?

Yes, missed-it.

bors added a commit to rust-lang/cargo that referenced this pull request Oct 18, 2023
Adjust `-Zcheck-cfg` for new rustc syntax and behavior

rust-lang/rust#111072 introduced a new syntax for `rustc` `--check-cfg` argument. This PR adjust cargo `-Zcheck-cfg` for new that new syntax and behavior.

This PR removes all the `-Zcheck-cfg` options (`features`, `names`, `values`, `output`), as they don't make much sense now since with the new `rustc` behavior: `features`, `names` and `values` are all combine together and the `output` option was only here because the other were.

Now the new behavior from cargo is to always pass one `--check-cfg` argument to rustc for the `feature`s which implicitly enables well known names and values.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2023
…k-cfg, r=<try>

Prepare the `bootstrap` tool for the new check-cfg syntax

This PR prepare the `bootstrap` tool for the [new check-cfg syntax](rust-lang#111072) as well as the according [changes to Cargo](rust-lang/cargo#12845).

Note that while the new syntax can technically available on stage > 2, we actually cannot use it since we need a cargo version that supports the new syntax which won't happen until the next beta bump (if I understand everything correctly).

r? bootstrap
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 26, 2023
…k-cfg, r=Kobzol

Prepare the `bootstrap` tool for the new check-cfg syntax

This PR prepare the `bootstrap` tool for the [new check-cfg syntax](rust-lang#111072) as well as the according [changes to Cargo](rust-lang/cargo#12845).

~~Note that while the new syntax can technically available on stage > 2, we actually cannot use it since we need a cargo version that supports the new syntax which won't happen until the next beta bump (if I understand everything correctly).~~

r? bootstrap
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 28, 2023
…Kobzol

Prepare the `bootstrap` tool for the new check-cfg syntax

This PR prepare the `bootstrap` tool for the [new check-cfg syntax](rust-lang/rust#111072) as well as the according [changes to Cargo](rust-lang/cargo#12845).

~~Note that while the new syntax can technically available on stage > 2, we actually cannot use it since we need a cargo version that supports the new syntax which won't happen until the next beta bump (if I understand everything correctly).~~

r? bootstrap
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Dec 5, 2023
…syntax, r=b-naber

Remove deprecated `--check-cfg` syntax

This PR removes the deprecated `--check-cfg` `names(...)` and `values(...)` syntax.

Follow up to rust-lang#111072
Part of rust-lang/compiler-team#636

r? compiler
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 6, 2023
…syntax, r=b-naber

Remove deprecated `--check-cfg` syntax

This PR removes the deprecated `--check-cfg` `names(...)` and `values(...)` syntax.

Follow up to rust-lang#111072
Part of rust-lang/compiler-team#636

r? compiler
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2023
Rollup merge of rust-lang#117981 - Urgau:check-cfg-remove-deprecated-syntax, r=b-naber

Remove deprecated `--check-cfg` syntax

This PR removes the deprecated `--check-cfg` `names(...)` and `values(...)` syntax.

Follow up to rust-lang#111072
Part of rust-lang/compiler-team#636

r? compiler
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
…Kobzol

Prepare the `bootstrap` tool for the new check-cfg syntax

This PR prepare the `bootstrap` tool for the [new check-cfg syntax](rust-lang/rust#111072) as well as the according [changes to Cargo](rust-lang/cargo#12845).

~~Note that while the new syntax can technically available on stage > 2, we actually cannot use it since we need a cargo version that supports the new syntax which won't happen until the next beta bump (if I understand everything correctly).~~

r? bootstrap
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
…Kobzol

Prepare the `bootstrap` tool for the new check-cfg syntax

This PR prepare the `bootstrap` tool for the [new check-cfg syntax](rust-lang/rust#111072) as well as the according [changes to Cargo](rust-lang/cargo#12845).

~~Note that while the new syntax can technically available on stage > 2, we actually cannot use it since we need a cargo version that supports the new syntax which won't happen until the next beta bump (if I understand everything correctly).~~

r? bootstrap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify and improve explicitness of the check-cfg syntax
8 participants