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

Fix --check-cfg invocations with zero features #13011

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Nov 19, 2023

When generating the --check-cfg arguments for -Zcheck-cfg we currently generate cfg(feature, values()) when there is 0 features. This is wrong since a empty values() would mean that it's possible to have #[cfg(feature)] without a feature name which is impossible.

We replace this by a simple cfg() to still enable well known names and values.


Note that currently rustc defines feature as a well known name with ANY values if it's not overridden by Cargo. I plan on submitting a PR to rustc to remove feature from being a well known name so that Cargo is the only source of truth.

This doesn't block this PR from being merged

@rustbot
Copy link
Collaborator

rustbot commented Nov 19, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2023
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds pretty reasonable to me. Thank you!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 20, 2023

📌 Commit 4781592 has been approved by weihanglo

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 Nov 20, 2023
@bors
Copy link
Collaborator

bors commented Nov 20, 2023

⌛ Testing commit 4781592 with merge 79acbdb...

@bors
Copy link
Collaborator

bors commented Nov 20, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 79acbdb to master...

@bors bors merged commit 79acbdb into rust-lang:master Nov 20, 2023
20 checks passed
@epage
Copy link
Contributor

epage commented Nov 20, 2023

When generating the --check-cfg arguments for -Zcheck-cfg we currently generate cfg(feature, values()) when there is 0 features. This is wrong since a empty values() would mean that it's possible to have #[cfg(feature)] without a feature name which is impossible.

To me, this reads as a bug in the design of the syntax if values() is special cased like this while the semantics are different.

@Urgau
Copy link
Member Author

Urgau commented Nov 20, 2023

values() (without any specified values) isn't special cased. It means no values are expected.
values("foo", "bar") means that the values "foo" and "bar" are expected.

The issue here lies before that, cfg(feature, values()) means: a config named "feature" exists and takes no values; but this isn't what we want, we want that no config named "feature" with whatever values exists, which is simply achieved by not defining "feature" in the first place.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 20, 2023
Update cargo

9 commits in 9765a449d9b7341c2b49b88da41c2268ea599720..71cd3a926f0cf41eeaf9f2a7f2194b2aff85b0f6
2023-11-17 20:58:23 +0000 to 2023-11-20 15:30:57 +0000
- Handle $message_type in JSON diagnostics (rust-lang/cargo#13016)
- refactor(toml): Further clean up inheritance (rust-lang/cargo#13000)
- Fix `--check-cfg` invocations with zero features (rust-lang/cargo#13011)
- chore: bump `cargo-credential-*` crates as e58b84d broke stuff (rust-lang/cargo#13010)
- contrib docs: Update now that credential crates are published. (rust-lang/cargo#13006)
- Add more resources to the contrib docs. (rust-lang/cargo#13008)
- Respect `rust-lang/rust`'s `omit-git-hash` (rust-lang/cargo#12968)
- Fix clippy-wrapper test race condition. (rust-lang/cargo#12999)
- fix(resolver): Don't do git fetches when updating workspace members (rust-lang/cargo#12975)
@epage
Copy link
Contributor

epage commented Nov 21, 2023

values() (without any specified values) isn't special cased. It means no values are expected.
values("foo", "bar") means that the values "foo" and "bar" are expected.

To put this in Rust types, does values() make it a unit type or an empty array?

struct EmptyValues;
const EMPTY_VALUES = [];

If its like a unit type, then that does feel like special casing.

The issue here lies before that, cfg(feature, values()) means: a config named "feature" exists and takes no values; but this isn't what we want, we want that no config named "feature" with whatever values exists, which is simply achieved by not defining "feature" in the first place.

Is that what we want?

Whether there even is a way to define a cfg as an empty array, I suspect being able to do so for features would offer the best usability.

As a user, if I get an diagnostic saying that features doesn't exist, I assume I fundamentally didn't understand how this worked. However, if I get a diagnostic saying that "foo" doesn't exist for features, then that is much clearer what the next steps are.

@Urgau
Copy link
Member Author

Urgau commented Nov 21, 2023

To put this in Rust types, does values() make it a unit type or an empty array?

struct EmptyValues;
const EMPTY_VALUES = [];

If its like a unit type, then that does feel like special casing.

It's more like const EMPTY_VALUES = [];.

Internally it would be more like const EMPTY_VALUES = [None]; where None represent the absence of value since we need to differentiate from "" (empty string).
This follows what is done for --cfg:

  • --cfg foo => ("foo", None)
  • --cfg foo="" => ("foo", Some(""))

The issue here lies before that, cfg(feature, values()) means: a config named "feature" exists and takes no values; but this isn't what we want, we want that no config named "feature" with whatever values exists, which is simply achieved by not defining "feature" in the first place.

Is that what we want?

I think so, from the perceptive of --check-cfg when config is not to be ever expected, since cargo will never pass --cfg feature=... to rustc, we should not declare feature as expected.

Whether there even is a way to define a cfg as an empty array, I suspect being able to do so for features would offer the best usability.

Being able to say: declare a config as expected with all values as unexpected, is not something that is currently possible.

As a user, if I get an diagnostic saying that features doesn't exist, I assume I fundamentally didn't understand how this worked. However, if I get a diagnostic saying that "foo" doesn't exist for features, then that is much clearer what the next steps are.

I agree, but feature is really unexpected here, making it awkward to say it exist but really no it doesn't.

Saying that feature is unexpected is not wrong but is not 100% true either, same for saying that feature is expected, it is not 100% right but it's not 100% wrong either.

I don't know how to fundamentally improve the situation here.

In rust-lang/rust#118071 I propose to add a diagnostic for this case: = help: consider defining some features in Cargo.toml, which should help, but it's not incredible.

@epage
Copy link
Contributor

epage commented Dec 1, 2023

imo the right answer, from the users perspective, is for us to say that a cfg can exist but no values are defined.

However. since rust-lang/rust#118071 at least provided some help to the user, that is likely sufficient so long as the design allows us to reconsider this in the future.

@epage
Copy link
Contributor

epage commented Dec 1, 2023

I've noted this in #10554 for us to discuss during stabillization

@ehuss ehuss added this to the 1.76.0 milestone Dec 6, 2023
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`
bors added a commit that referenced this pull request Jan 18, 2024
Go back to passing an empty `values()` when no features are declared

This PR is basically a revert of #13011, which made the `--check-cfg` invocation be `cfg()` when no features are declared instead of `cfg(feature, values())` since it meant declaring that the config `feature` were to be available in this form: `#[cfg(feature)]`, which is not the case.

Thankfully after some brainstorming, I [proposed](rust-lang/rust#119930) changing the behavior of empty `values()` in `rustc` to no longer imply the _none_ value and simply create an empty set of expected values. 😃

For Cargo, always using `cfg(feature, values(...))` means that the config `feature` is always expected, regardless of the number of features. This makes the warning better, as it will now always be `unexpected config value` (instead of `unexpected config name`). 🎉

Fixes #13011 (comment) as well as the concern in the [tracking issue](#10554).

r? `@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
Labels
A-build-execution Area: anything dealing with executing the compiler S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants