Skip to content

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Feb 20, 2022

This pull-request is a mix of improvements regarding the --check-cfg implementation:

  • Simpler internal representation (usage of Option instead of separate bool)
  • Add --check-cfg to the unstable book (based on the RFC)
  • Improved diagnostics:
    • List possible values when the value is unexpected
    • Suggest if possible a name or value that is similar
  • Add more tests (well known names, mix of combinations, ...)

r? @petrochenkov

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 20, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2022
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the check-cfg-improvements branch from aa754d7 to f36c077 Compare February 21, 2022 15:24
@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 Feb 22, 2022
@Urgau
Copy link
Member Author

Urgau commented Feb 22, 2022

I've address all review comments, except for #94175 (comment) and #94175 (comment) where I left a comment.
This is ready for a another review.

I also would like to apologies for my English mistakes, it's not my native language. I will try my best to make sure these mistakes doesn't happen again.

@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 Feb 22, 2022
@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 Feb 22, 2022
@Urgau
Copy link
Member Author

Urgau commented Feb 22, 2022

Fixed #94175 (comment) and added a note when no value is expected ecd3a65.

@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 Feb 22, 2022
@petrochenkov
Copy link
Contributor

r=me with the remaining comments addressed and commits squashed appropriately.

@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 Feb 22, 2022
@Urgau Urgau force-pushed the check-cfg-improvements branch from ecd3a65 to 6e3f642 Compare February 22, 2022 22:37
@Urgau
Copy link
Member Author

Urgau commented Feb 22, 2022

Fixed last remaining comments and squashed to 5 commits. This is ready for r=you.

@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 Feb 22, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 22, 2022

📌 Commit 6e3f642 has been approved by petrochenkov

@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 Feb 23, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 24, 2022

📌 Commit a556a2a has been approved by petrochenkov

@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 Feb 24, 2022
@bors
Copy link
Collaborator

bors commented Feb 24, 2022

⌛ Testing commit a556a2a with merge d0a34dc40eba51c4cd4173f8fb56b4c562209c90...

@bors
Copy link
Collaborator

bors commented Feb 24, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2022
@Mark-Simulacrum
Copy link
Member

@bors retry (crates.io down)

@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 Feb 24, 2022
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2022
…rochenkov

Improve `--check-cfg` implementation

This pull-request is a mix of improvements regarding the `--check-cfg` implementation:

- Simpler internal representation (usage of `Option` instead of separate bool)
- Add --check-cfg to the unstable book (based on the RFC)
- Improved diagnostics:
    * List possible values when the value is unexpected
    * Suggest if possible a name or value that is similar
- Add more tests (well known names, mix of combinations, ...)

r? `@petrochenkov`
@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
…rochenkov

Improve `--check-cfg` implementation

This pull-request is a mix of improvements regarding the `--check-cfg` implementation:

- Simpler internal representation (usage of `Option` instead of separate bool)
- Add --check-cfg to the unstable book (based on the RFC)
- Improved diagnostics:
    * List possible values when the value is unexpected
    * Suggest if possible a name or value that is similar
- Add more tests (well known names, mix of combinations, ...)

r? `@petrochenkov`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
…rochenkov

Improve `--check-cfg` implementation

This pull-request is a mix of improvements regarding the `--check-cfg` implementation:

- Simpler internal representation (usage of `Option` instead of separate bool)
- Add --check-cfg to the unstable book (based on the RFC)
- Improved diagnostics:
    * List possible values when the value is unexpected
    * Suggest if possible a name or value that is similar
- Add more tests (well known names, mix of combinations, ...)

r? ``@petrochenkov``
This was referenced Feb 24, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#91795 (resolve/metadata: Stop encoding macros as reexports)
 - rust-lang#93714 (better ObligationCause for normalization errors in `can_type_implement_copy`)
 - rust-lang#94175 (Improve `--check-cfg` implementation)
 - rust-lang#94212 (Stop manually SIMDing in `swap_nonoverlapping`)
 - rust-lang#94242 (properly handle fat pointers to uninhabitable types)
 - rust-lang#94308 (Normalize main return type during mono item collection & codegen)
 - rust-lang#94315 (update auto trait lint for `PhantomData`)
 - rust-lang#94316 (Improve string literal unescaping)
 - rust-lang#94327 (Avoid emitting full macro body into JSON errors)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 000e38d into rust-lang:master Feb 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 25, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 18, 2022
…, r=petrochenkov

Always evaluate all cfg predicate in all() and any()

This pull-request adjust the handling of the `all()` and `any()` to always evaluate every cfg predicate because not doing so result in accepting incorrect `cfg`:

```rust
#[cfg(any(unix, foo::bar))] // Should error on foo::bar, but does not on unix platform (but does on non unix platform)
fn foo1() {}

#[cfg(all(foo, foo::bar))] // Should error on foo::bar, but does not
fn foo2() {}

#[cfg(all(foo::bar, foo))] // Correctly error on foo::bar
fn foo3() {}

#[cfg(any(foo::bar, foo))] // Correctly error on foo::bar
fn foo4() {}
```
This pull-request take the side to directly turn it into a hard error instead of having a future incompatibility lint because the combination to get this incorrect behavior is unusual and highly probable that some code have this without noticing.

A [search](https://cs.github.com/?scopeName=All+repos&scope=&q=lang%3Arust+%2Fany%5C%28%5Ba-zA-Z%5D%2C+%5Ba-zA-Z%5D%2B%3A%3A%5Ba-zA-Z%5D%2B%2F) on Github reveal no such instance nevertheless a Crater run should probably be done before merging this.

This was discover in rust-lang#94175 when trying to lint on the second predicate. Also note that this seems to have being introduce with Rust 1.27.0: https://rust.godbolt.org/z/KnfqKv15f.

r? `@petrochenkov`
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants