Detect more `cfg`d out items in resolution errors #129183

estebank · 2024-08-17T05:45:54Z

Use a visitor to collect all items (including those nested) that were stripped behind a cfg condition.

error[E0425]: cannot find function `f` in this scope
  --> $DIR/nested-cfg-attrs.rs:4:13
   |
LL | fn main() { f() }
   |             ^ not found in this scope
   |
note: found an item that was configured out
  --> $DIR/nested-cfg-attrs.rs:2:4
   |
LL | fn f() {}
   |    ^
note: the item is gated here
  --> $DIR/nested-cfg-attrs.rs:1:35
   |
LL | #[cfg_attr(all(), cfg_attr(all(), cfg(FALSE)))]
   |                                   ^^^^^^^^^^

rustbot · 2024-08-17T05:46:03Z

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Urgau · 2024-08-17T08:05:19Z

compiler/rustc_expand/src/expand.rs

-                    ast::UseTreeKind::Nested { items, .. } => {
-                        for (ut, _) in items {
-                            collect_use_tree_leaves(ut, idents);
+        struct ItemNameVisitor(Vec<Ident>);


What happens in the case of a double or more stripped items? Won't this visitor and method record them twice?

#![allow(unexpected_cfgs)] #[cfg(foo)] mod fs { #[cfg(bar)] mod unix { fn fchown() {} ) } fn main() { fs::unix::chown(); }

This gives you

error[E0433]: failed to resolve: use of undeclared crate or module `fs` --> f72.rs:13:5 | 13 | fs::unix::chown(); | ^^ use of undeclared crate or module `fs` | note: found an item that was configured out --> f72.rs:4:5 | 4 | mod fs { | ^^ note: the item is gated here --> f72.rs:3:1 | 3 | #[cfg(foo)] | ^^^^^^^^^^^

because fs is not found so that's the only one that would be checked for (not unix or chown).

If you check for unix, it will point at cfg(foo), which is not enough to get unix back, but gets you on the way to it:

error[E0433]: failed to resolve: use of undeclared crate or module `unix` --> f72.rs:14:5 | 14 | unix::chown(); | ^^^^ use of undeclared crate or module `unix` | note: found an item that was configured out --> f72.rs:6:9 | 6 | mod unix { | ^^^^ note: the item is gated here --> f72.rs:3:1 | 3 | #[cfg(foo)] | ^^^^^^^^^^^

In testing, I noticed that we don't point at fs::unix when trying unix directly and it is behind a cfg (the second error):

error[E0433]: failed to resolve: could not find `unix` in `fs` --> f72.rs:13:9 | 13 | fs::unix::chown(); | ^^^^ could not find `unix` in `fs` | note: found an item that was configured out --> f72.rs:6:9 | 6 | mod unix { | ^^^^ note: the item is gated here --> f72.rs:5:5 | 5 | #[cfg(bar)] | ^^^^^^^^^^^ error[E0433]: failed to resolve: use of undeclared crate or module `unix` --> f72.rs:14:5 | 14 | unix::chown(); | ^^^^ use of undeclared crate or module `unix`

If it isn't cfgd out and it's public, we do provide this suggestion:

error[E0433]: failed to resolve: use of undeclared crate or module `unix` --> f72.rs:14:5 | 14 | unix::chown(); | ^^^^ use of undeclared crate or module `unix` | help: consider importing this module | 4 + use fs::unix; |

but if it is private and crate local maybe we should at least point at it (we don't):

error[E0433]: failed to resolve: use of undeclared crate or module `unix` --> f72.rs:14:5 | 14 | unix::chown(); | ^^^^ use of undeclared crate or module `unix`

Changed to behave as you expected.

Noratrieb · 2024-08-17T11:47:47Z

tests/ui/cfg/diagnostics-reexport.rs

@@ -1,7 +1,7 @@
 pub mod inner {
-    #[cfg(FALSE)]
+    #[cfg(FALSE)] //~ NOTE the item is gated here


I don't think this is correct? no one ever references this item.

This is because uwu under inner::gone now gets included for inner (because gone is cfgd out) when trying to access inner::uwu. I believe this is the right thing to do. If the user had written inner::gone::uwu, the output is:

error[E0433]: failed to resolve: could not find `gone` in `inner` --> tests/ui/cfg/diagnostics-reexport.rs:38:12 | 38 | inner::gone::uwu(); //~ ERROR cannot find function | ^^^^ could not find `gone` in `inner` | note: found an item that was configured out --> tests/ui/cfg/diagnostics-reexport.rs:3:9 | 3 | mod gone { | ^^^^ note: the item is gated here --> tests/ui/cfg/diagnostics-reexport.rs:2:5 | 2 | #[cfg(FALSE)] //~ NOTE the item is gated here | ^^^^^^^^^^^^^

but if it is inner::gon::uwu then the output is

error[E0433]: failed to resolve: could not find `gon` in `inner` --> tests/ui/cfg/diagnostics-reexport.rs:38:12 | 38 | inner::gon::uwu(); //~ ERROR cannot find function | ^^^ could not find `gon` in `inner`

We might actually want to still search for cfgd out items both with levenshtein distance (but the signal there is much weaker) and by looking at the very last path segment (to find the cfgd out uwu).

I tend to agree with @Noratrieb: the user wrote inner::uwu, which refers to the use super::uwu below, which already failed to resolve without suggesting gone. Either both suggest gone::uwu, or none of them.

tests/ui/cfg/diagnostics-same-crate.rs

petrochenkov · 2024-08-20T16:30:38Z

We are doing more work on a good path here.
@bors try @rust-timer queue

Detect more `cfg`d out items in resolution errors Use a visitor to collect *all* items (including those nested) that were stripped behind a `cfg` condition. ``` error[E0425]: cannot find function `f` in this scope --> $DIR/nested-cfg-attrs.rs:4:13 | LL | fn main() { f() } | ^ not found in this scope | note: found an item that was configured out --> $DIR/nested-cfg-attrs.rs:2:4 | LL | fn f() {} | ^ note: the item is gated here --> $DIR/nested-cfg-attrs.rs:1:35 | LL | #[cfg_attr(all(), cfg_attr(all(), cfg(FALSE)))] | ^^^^^^^^^^ ```

bors · 2024-08-20T16:31:51Z

⌛ Trying commit 95c2b46 with merge 623e6c7...

bors · 2024-08-20T18:30:37Z

☀️ Try build successful - checks-actions
Build commit: 623e6c7 (623e6c7b5c223cff0a4b75919463e800e0563a50)

rust-timer · 2024-08-20T19:46:57Z

Finished benchmarking commit (623e6c7): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.3%, 0.2%]	3

Max RSS (memory usage)

Results (primary 2.6%, secondary 1.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[1.8%, 3.3%]	3
Regressions ❌ (secondary)	3.1%	[1.7%, 4.3%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.8%	[-4.8%, -4.8%]	1
All ❌✅ (primary)	2.6%	[1.8%, 3.3%]	3

Cycles

Results (secondary -2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.1%, -2.6%]	6
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.4%]	83
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.4%]	83

Bootstrap: 749.153s -> 748.829s (-0.04%)
Artifact size: 338.89 MiB -> 338.83 MiB (-0.02%)

estebank · 2024-08-22T16:56:55Z

It might make sense to limit the visitor to two levels of nesting at most, as a way to attempt to reduce the impact on crates that have large modules cfgd out. I suspect that we might not have any test case of a large crate in that configuration, and that's why we're not seeing more effects from this change.

estebank · 2024-08-28T23:35:21Z

@bors try @rust-timer queue

Detect more `cfg`d out items in resolution errors Use a visitor to collect *all* items (including those nested) that were stripped behind a `cfg` condition. ``` error[E0425]: cannot find function `f` in this scope --> $DIR/nested-cfg-attrs.rs:4:13 | LL | fn main() { f() } | ^ not found in this scope | note: found an item that was configured out --> $DIR/nested-cfg-attrs.rs:2:4 | LL | fn f() {} | ^ note: the item is gated here --> $DIR/nested-cfg-attrs.rs:1:35 | LL | #[cfg_attr(all(), cfg_attr(all(), cfg(FALSE)))] | ^^^^^^^^^^ ```

bors · 2024-08-28T23:36:32Z

⌛ Trying commit 0e22c73 with merge 811a696...

bors · 2024-08-29T01:31:15Z

☀️ Try build successful - checks-actions
Build commit: 811a696 (811a69609bf7d38bbab02fa8f4d3b4cb98802f24)

rust-timer · 2024-08-29T03:55:45Z

Finished benchmarking commit (811a696): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	4

Max RSS (memory usage)

Results (primary 1.6%, secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[0.6%, 2.6%]	4
Regressions ❌ (secondary)	2.3%	[2.1%, 2.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[0.6%, 2.6%]	4

Cycles

Results (secondary -2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.4%, -2.6%]	14
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.5%]	83
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.5%]	83

Bootstrap: 753.9s -> 754.227s (0.04%)
Artifact size: 338.87 MiB -> 339.03 MiB (0.05%)

estebank · 2024-08-30T15:46:05Z

I suspect that the regressions on incr-comp for html5ever-0.26 are caused by this gated macro expansion. clap-3.1.6 is quite feature heavy. I leave it to you whether the impact on incr-comp of munching through more nodes is worth it. I believe it it is, as when items are cfgd out it leads to very confusing output otherwise.

joshka · 2024-12-18T01:39:22Z

Possibly related enough to consider as part of this (if it's not already). #134448
(TLDR - cfg out derive macro and trait with the same path don't trigger this diagnostic)

Noratrieb · 2024-12-18T11:11:34Z

tests/ui/cfg/diagnostics-same-crate.rs

-    // but currently don't have one.
-    // Not that it matters much though, this is highly unlikely to confuse anyone.
+    // Exists in the crate root - we show a diagnostic because we treat "no module DefId" as "crate
+    // root DefId".


I don't think this is correct? it should be the current module def id instead

It's been a while since I last looked at this code, but I think this was referring to https://github.com/rust-lang/rust/pull/129183/files#diff-0e459b4cbb41dd93274d7c68b18a2a3eab7152e9aea107839a7bddff0b4e9195R819

Oh yeah I didn't clarify, the comment matches the implementation but I think the implementation is wrong

Noratrieb · 2024-12-18T11:13:03Z

@estebank can you explain why this nesting is necessary? I fail to see a case where nesting is necessary, as resolution will always fail at the outermost missing item, making the inner ones physically inaccessible.

estebank · 2024-12-31T23:00:57Z

I fail to see a case where nesting is necessary, as resolution will always fail at the outermost missing item, making the inner ones physically inaccessible.

Removed the recursive search. My logic was "the user already thinks an item exists, they might have both used the wrong path and have it disabled behind a cfg flag" where knowing the item is somewhere but not seeing the cfg would be confusing. But left with a single level as I think we should land at least the simpler version.

bors · 2025-01-25T14:43:55Z

☔ The latest upstream changes (presumably #133154) made this pull request unmergeable. Please resolve the merge conflicts.

Use a visitor to collect *all* items (including those nested) that were stripped behind a `cfg` condition. ``` error[E0425]: cannot find function `f` in this scope --> $DIR/nested-cfg-attrs.rs:4:13 | LL | fn main() { f() } | ^ not found in this scope | note: found an item that was configured out --> $DIR/nested-cfg-attrs.rs:2:4 | LL | fn f() {} | ^ note: the item is gated here --> $DIR/nested-cfg-attrs.rs:1:35 | LL | #[cfg_attr(all(), cfg_attr(all(), cfg(FALSE)))] | ^^^^^^^^^^ ```

rustbot assigned cjgillot Aug 17, 2024

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 Aug 17, 2024

Urgau reviewed Aug 17, 2024

View reviewed changes

petrochenkov self-assigned this Aug 17, 2024

Noratrieb reviewed Aug 17, 2024

View reviewed changes

estebank force-pushed the cfg-visitor branch from 1390498 to 95c2b46 Compare August 17, 2024 19:57

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 20, 2024

petrochenkov removed their assignment Aug 20, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 20, 2024

estebank force-pushed the cfg-visitor branch from 95c2b46 to 0e22c73 Compare August 28, 2024 23:35

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 28, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 29, 2024

cjgillot 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 Sep 14, 2024

traviscross added the A-diagnostics Area: Messages for errors, warnings, and lints label Sep 29, 2024

fmease linked an issue Dec 10, 2024 that may be closed by this pull request

Diagnostic doesn't mention cfg'ed out items if the unresolved path is simple (i.e., a single identifier) #132166

Open

joshka mentioned this pull request Dec 18, 2024

cfg resolve diagnostic doesn't show up for axum macro #134448

Open

Noratrieb reviewed Dec 18, 2024

View reviewed changes

estebank force-pushed the cfg-visitor branch from 0e22c73 to 1b7a0f3 Compare December 31, 2024 22:59

estebank force-pushed the cfg-visitor branch from 1b7a0f3 to d4e2623 Compare December 31, 2024 23:13

estebank added 3 commits January 28, 2025 07:49

Limit how deep we visit items to find cfg'd out names

8d9c9b0

remove recursive search for items

4fa08e9

estebank force-pushed the cfg-visitor branch from d4e2623 to 4fa08e9 Compare January 28, 2025 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect more `cfg`d out items in resolution errors #129183

Detect more `cfg`d out items in resolution errors #129183

estebank commented Aug 17, 2024

rustbot commented Aug 17, 2024

Urgau Aug 17, 2024

estebank Aug 17, 2024

estebank Dec 31, 2024

Noratrieb Aug 17, 2024

estebank Aug 17, 2024

cjgillot Sep 14, 2024

petrochenkov commented Aug 20, 2024

This comment has been minimized.

bors commented Aug 20, 2024

bors commented Aug 20, 2024

This comment has been minimized.

rust-timer commented Aug 20, 2024

estebank commented Aug 22, 2024

estebank commented Aug 28, 2024

This comment has been minimized.

bors commented Aug 28, 2024

bors commented Aug 29, 2024

This comment has been minimized.

rust-timer commented Aug 29, 2024

estebank commented Aug 30, 2024

joshka commented Dec 18, 2024

Noratrieb Dec 18, 2024

estebank Dec 31, 2024

Noratrieb Dec 31, 2024

Noratrieb commented Dec 18, 2024

estebank commented Dec 31, 2024

bors commented Jan 25, 2025

Detect more cfgd out items in resolution errors #129183

Are you sure you want to change the base?

Detect more cfgd out items in resolution errors #129183

Conversation

estebank commented Aug 17, 2024

rustbot commented Aug 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Aug 20, 2024

This comment has been minimized.

bors commented Aug 20, 2024

bors commented Aug 20, 2024

This comment has been minimized.

rust-timer commented Aug 20, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

estebank commented Aug 22, 2024

estebank commented Aug 28, 2024

This comment has been minimized.

bors commented Aug 28, 2024

bors commented Aug 29, 2024

This comment has been minimized.

rust-timer commented Aug 29, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

estebank commented Aug 30, 2024

joshka commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Dec 18, 2024

estebank commented Dec 31, 2024

bors commented Jan 25, 2025

Detect more `cfg`d out items in resolution errors #129183

Detect more `cfg`d out items in resolution errors #129183