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 invalid check-cfg Cargo feature diagnostic help #119425

Merged
merged 2 commits into from
Dec 30, 2023

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Dec 30, 2023

#118213 added specialized diagnostic for Cargo feature cfg. However when providing an empty #[cfg(feature)] condition the suggestion would suggest adding feature as a feature in Cargo.toml (wtf!).

This PR removes the invalid logic, which even brings a nice improvement.

   --> $DIR/cargo-feature.rs:18:7
    |
 LL | #[cfg(feature)]
-   |       ^^^^^^^
+   |       ^^^^^^^- help: specify a config value: `= "bitcode"`
    |
    = note: expected values for `feature` are: `bitcode`
-   = help: consider defining `feature` as feature in `Cargo.toml`

The first commit add a test showing the bug and the second commit fixes the bug.

@rustbot label +F-check-cfg

@rustbot
Copy link
Collaborator

rustbot commented Dec 30, 2023

r? @compiler-errors

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

@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. F-check-cfg --check-cfg labels Dec 30, 2023
@@ -804,8 +804,6 @@ pub trait LintContext {
db.span_suggestion(value_span, "there is a expected value with a similar name", format!("\"{best_match}\""), Applicability::MaybeIncorrect);

}
} else if name == sym::feature && is_from_cargo {
Copy link
Member

Choose a reason for hiding this comment

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

unrelated comment, but I feel like all of this logic should be moved to a separate module instead of being in this big match

Copy link
Member Author

Choose a reason for hiding this comment

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

I've put up #119443 to address this suggestion

@Noratrieb
Copy link
Member

r=me when CI is happy
@bors delegate+

@bors
Copy link
Contributor

bors commented Dec 30, 2023

✌️ @Urgau, you can now approve this pull request!

If @Nilstrieb told you to "r=me" after making some further change, please make that change, then do @bors r=@Nilstrieb

@Urgau
Copy link
Member Author

Urgau commented Dec 30, 2023

@bors r=@Nilstrieb

@bors
Copy link
Contributor

bors commented Dec 30, 2023

📌 Commit a25e023 has been approved by Nilstrieb

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 Dec 30, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#119158 (Clean up alloc::sync::Weak Clone implementation)
 - rust-lang#119386 (fix typo in `IpAddr::to_canonical`)
 - rust-lang#119413 (solaris support on bootstrap lock)
 - rust-lang#119424 (Primitive docs: fix confusing `Send` in `&T`'s list)
 - rust-lang#119425 (Fix invalid check-cfg Cargo feature diagnostic help)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e930ea2 into rust-lang:master Dec 30, 2023
11 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2023
Rollup merge of rust-lang#119425 - Urgau:check-cfg-fix-cargo-diag-bug, r=Nilstrieb

Fix invalid check-cfg Cargo feature diagnostic help

rust-lang#118213 added specialized diagnostic for Cargo `feature` cfg. However when providing an empty `#[cfg(feature)]` condition the suggestion would suggest adding `feature` as a feature in `Cargo.toml` (wtf!).

This PR removes the invalid logic, which even brings a nice improvement.

```diff
   --> $DIR/cargo-feature.rs:18:7
    |
 LL | #[cfg(feature)]
-   |       ^^^^^^^
+   |       ^^^^^^^- help: specify a config value: `= "bitcode"`
    |
    = note: expected values for `feature` are: `bitcode`
-   = help: consider defining `feature` as feature in `Cargo.toml`
```

The first commit add a test showing the bug and the second commit fixes the bug.

`@rustbot` label +F-check-cfg
@rustbot rustbot added this to the 1.77.0 milestone Dec 30, 2023
@Urgau Urgau deleted the check-cfg-fix-cargo-diag-bug branch December 30, 2023 13:44
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2023
…xt, r=Nilstrieb

Move around the code responsible for decorating builtin diagnostics

This PR move the code responsible for decorating builtin diagnostics into a separate sub-module for ease of use and readability.

While my original intention was to also move the check-cfg unexpected logic in their own function I changed my mind after moving the match altogether. I can move those if desired.

Fixes rust-lang#119425 (comment)

r? `@Nilstrieb`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2023
…xt, r=Nilstrieb

Move around the code responsible for decorating builtin diagnostics

This PR move the code responsible for decorating builtin diagnostics into a separate sub-module for ease of use and readability.

While my original intention was to also move the check-cfg unexpected logic in their own function I changed my mind after moving the match altogether. I can move those if desired.

Fixes rust-lang#119425 (comment)

r? `@Nilstrieb`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-check-cfg --check-cfg 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.

5 participants