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

Suggest name value cfg when only value is used for check-cfg #120435

Merged
merged 3 commits into from
Feb 7, 2024

Conversation

chenyukang
Copy link
Member

Fixes #120427
r? @Nilstrieb

@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. labels Jan 28, 2024
@@ -187,6 +187,14 @@ pub(super) fn builtin(
BuiltinLintDiagnostics::UnexpectedCfgName((name, name_span), value) => {
let possibilities: Vec<Symbol> =
sess.parse_sess.check_config.expecteds.keys().copied().collect();

#[allow(rustc::potential_query_instability)]
Copy link
Member

@Urgau Urgau Jan 28, 2024

Choose a reason for hiding this comment

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

I don't think this instability can be dismissed. It's possible that two or more different cfgs have the same value that we are searching for, in which case we should either suggest both or none.

@rustbot author

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, we still need to keep #[allow(rustc::potential_query_instability)] to remove the warning because we use iter()...

Copy link
Member

Choose a reason for hiding this comment

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

This is an unfortunate limitation of the lint, it doesn't detect the following sort() call. Can you add a comment explaining that this is fine since we sort the list just below.

Copy link
Member Author

@chenyukang chenyukang Jan 28, 2024

Choose a reason for hiding this comment

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

I'm curious why keys() and values() don't have potential_query_instability, and into_keys(), into_values() have that property, according to the document, they are all in arbitrary order.

/// An iterator visiting all keys in arbitrary order.
/// The iterator element type is `&'a K`.
///
/// # Examples
///
/// ```
/// use std::collections::HashMap;
///
/// let map = HashMap::from([
/// ("a", 1),
/// ("b", 2),
/// ("c", 3),
/// ]);
///
/// for key in map.keys() {
/// println!("{key}");
/// }
/// ```
///
/// # Performance
///
/// In the current implementation, iterating over keys takes O(capacity) time
/// instead of O(len) because it internally visits empty buckets too.
#[stable(feature = "rust1", since = "1.0.0")]
pub fn keys(&self) -> Keys<'_, K, V> {
Keys { inner: self.iter() }
}

Copy link
Member

Choose a reason for hiding this comment

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

It's missing the custom rustc attribute #[rustc_lint_query_instability] like in the into_keys function:

#[rustc_lint_query_instability]
#[stable(feature = "map_into_keys_values", since = "1.54.0")]
pub fn into_values(self) -> IntoValues<K, V> {

This could added in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I know it's missing #[rustc_lint_query_instability], I was wondering if there is any special reason here, but they are actually all iter in the inner.

I will create another pr later.

tests/ui/check-cfg/cfg-value-for-cfg-name.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/context/diagnostics.rs Outdated Show resolved Hide resolved
tests/ui/check-cfg/cfg-value-for-cfg-name.rs Show resolved Hide resolved
compiler/rustc_lint/src/context/diagnostics.rs Outdated Show resolved Hide resolved
tests/ui/check-cfg/cfg-value-for-cfg-name.stderr Outdated Show resolved Hide resolved
compiler/rustc_lint/src/context/diagnostics.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot 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 Jan 28, 2024
@chenyukang chenyukang force-pushed the yukang-fix-120427-cfg-name branch 5 times, most recently from 936b11c to 61ff7de Compare January 28, 2024 15:14
@chenyukang chenyukang force-pushed the yukang-fix-120427-cfg-name branch from 61ff7de to c367983 Compare January 28, 2024 15:25
Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

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

This looks almost perfect :)

Just one last thing to improve.

compiler/rustc_lint/src/context/diagnostics.rs Outdated Show resolved Hide resolved
Co-authored-by: Urgau <3616612+Urgau@users.noreply.github.com>
@chenyukang
Copy link
Member Author

@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 Jan 30, 2024
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

Thanks, and thanks @Urgau for the review!
One last thing: Could you add a test for the >3 case?
r=Urgau,Nilstrieb then

@Noratrieb
Copy link
Member

@bors r=Urgau,Nilstrieb

@bors
Copy link
Contributor

bors commented Jan 30, 2024

📌 Commit ca243e7 has been approved by Urgau,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 Jan 30, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 30, 2024
…ility-check, r=michaelwoerister

add missing potential_query_instability for keys and values in hashmap

From rust-lang#120435 (comment),

These API are also returning iterator, so we need add `potential_query_instability` for them?
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2024
Rollup merge of rust-lang#120485 - chenyukang:yukang-add-query-instability-check, r=michaelwoerister

add missing potential_query_instability for keys and values in hashmap

From rust-lang#120435 (comment),

These API are also returning iterator, so we need add `potential_query_instability` for them?
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 30, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? `@Nilstrieb`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? ````@Nilstrieb````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? `````@Nilstrieb`````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern)
 - rust-lang#120060 (Use the same mir-opt bless targets on all platforms)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120326 (Actually abort in -Zpanic-abort-tests)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? ``````@Nilstrieb``````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120520 (Some cleanups around diagnostic levels.)
 - rust-lang#120521 (Make `NonZero` constructors generic.)
 - rust-lang#120527 (Switch OwnedStore handle count to AtomicU32)
 - rust-lang#120550 (Continue to borrowck even if there were previous errors)
 - rust-lang#120575 (Simplify codegen diagnostic handling)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? ```````@Nilstrieb```````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120521 (Make `NonZero` constructors generic.)
 - rust-lang#120527 (Switch OwnedStore handle count to AtomicU32)
 - rust-lang#120550 (Continue to borrowck even if there were previous errors)
 - rust-lang#120575 (Simplify codegen diagnostic handling)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? ````````@Nilstrieb````````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#120023 (tidy: reduce allocs)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120521 (Make `NonZero` constructors generic.)
 - rust-lang#120527 (Switch OwnedStore handle count to AtomicU32)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? `````````@Nilstrieb`````````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? ``````````@Nilstrieb``````````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#120023 (tidy: reduce allocs)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120520 (Some cleanups around diagnostic levels.)
 - rust-lang#120575 (Simplify codegen diagnostic handling)
 - rust-lang#120670 (cleanup effect var handling)

Failed merges:

 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? ```````````@Nilstrieb```````````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#119614 (unstably allow constants to refer to statics and read from immutable statics)
 - rust-lang#119939 (Improve 'generic param from outer item' error for `Self` and inside `static`/`const` items)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120331 (pattern_analysis: use a plain `Vec` in `DeconstructedPat`)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120502 (Remove `ffi_returns_twice` feature)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120513 (Normalize type outlives obligations in NLL for new solver)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2024
…ame, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? ````````````@Nilstrieb````````````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119939 (Improve 'generic param from outer item' error for `Self` and inside `static`/`const` items)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120331 (pattern_analysis: use a plain `Vec` in `DeconstructedPat`)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120502 (Remove `ffi_returns_twice` feature)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120513 (Normalize type outlives obligations in NLL for new solver)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119939 (Improve 'generic param from outer item' error for `Self` and inside `static`/`const` items)
 - rust-lang#120331 (pattern_analysis: use a plain `Vec` in `DeconstructedPat`)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120502 (Remove `ffi_returns_twice` feature)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120513 (Normalize type outlives obligations in NLL for new solver)
 - rust-lang#120707 (Don't expect early-bound region to be local when reporting errors in RPITIT well-formedness)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0a4702d into rust-lang:master Feb 7, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
Rollup merge of rust-lang#120435 - chenyukang:yukang-fix-120427-cfg-name, r=Urgau,Nilstrieb

Suggest name value cfg when only value is used for check-cfg

Fixes rust-lang#120427
r? `````````````@Nilstrieb`````````````
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.

Suggest name value cfg when only value is used for check-cfg
6 participants