Handle more cases in cfg_accessible #97391

Urgau · 2022-05-25T11:14:14Z

This PR tries to handle more cases in the cfg_accessible implementation by only emitting a "not sure" error only if we have partially resolved a path.

This PR also adds many tests for the "not sure" cases and for private items.

r? @petrochenkov

src/test/ui/conditional-compilation/cfg_accessible.rs

src/test/ui/conditional-compilation/cfg_accessible-not_sure.rs

src/test/ui/conditional-compilation/cfg_accessible.rs

compiler/rustc_resolve/src/macros.rs

Urgau · 2022-05-25T14:29:44Z

I've address the review comments. This is ready for another review.

@rustbot ready

compiler/rustc_resolve/src/macros.rs

src/test/ui/conditional-compilation/cfg_accessible-bugs.rs

src/test/ui/conditional-compilation/cfg_accessible.rs

src/test/ui/conditional-compilation/cfg_accessible-not_sure.rs

Urgau · 2022-05-25T18:19:04Z

I've addressed all review comments expect #97391 (comment) for which I put a message describing the situation.

@rustbot ready

src/test/ui/conditional-compilation/cfg_accessible-bugs.rs

compiler/rustc_resolve/src/macros.rs

Urgau · 2022-05-30T17:40:33Z

I've addressed the review comments: removed known-bug and added the requested modification in rustc_resolve. I'm not fully sure about the last one and there are some diagnostics regression. But I want your opinion on it and it's ready for another review.

@rustbot ready

compiler/rustc_resolve/src/ident.rs

petrochenkov · 2022-06-02T18:48:09Z

src/test/ui/conditional-compilation/cfg_accessible-bugs.rs

+trait TraitAlias = std::fmt::Debug + Send;
+
+// FIXME: Currently shows "cannot determine" but should be `false`
+#[cfg_accessible(unresolved)] //~ ERROR cannot determine


This is not a bug, if the compiler says "cannot determine" (rather than "not sure") then it's legitimately stuck, and it's unlikely we can do anything here.

The problem here is that we return an error instead of false (ie it is unresolvable).
The error message is irrelevant to the bug.

Urgau · 2022-06-03T10:25:32Z

I've reverted the failed -> NonModule change as requested; I've posted the patch here https://gist.github.com/Urgau/7356282bc688ea3ab71b94412e86a2d1. I also responded to the last comment.

@rustbot ready

petrochenkov · 2022-06-04T19:42:14Z

Thanks!
@bors r+

bors · 2022-06-04T19:42:16Z

📌 Commit b76d112 has been approved by petrochenkov

bors · 2022-06-05T04:16:06Z

⌛ Testing commit b76d112 with merge 656eec8...

bors · 2022-06-05T06:35:27Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 656eec8 to master...

rust-timer · 2022-06-05T07:53:45Z

Finished benchmarking commit (656eec8): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.3%	3.3%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.8%	-4.4%	2
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	3.6%	3.8%	2
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.6%	3.8%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 25, 2022

rust-highfive assigned petrochenkov May 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2022

Urgau force-pushed the cfg_accessible branch 2 times, most recently from 806b00c to bc049c1 Compare May 25, 2022 11:29

This comment has been minimized.

Sign in to view

Urgau force-pushed the cfg_accessible branch from bc049c1 to b040a20 Compare May 25, 2022 11:50

est31 reviewed May 25, 2022

View reviewed changes

src/test/ui/conditional-compilation/cfg_accessible.rs Show resolved Hide resolved

petrochenkov reviewed May 25, 2022

View reviewed changes

petrochenkov mentioned this pull request May 25, 2022

Tracking issue for RFC 2523, #[cfg(accessible(::path::to::thing))] #64797

Open

3 tasks

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 May 25, 2022

Urgau force-pushed the cfg_accessible branch from b040a20 to e704700 Compare May 25, 2022 14:27

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 May 25, 2022

Urgau force-pushed the cfg_accessible branch from e704700 to 8610703 Compare May 25, 2022 14:39

petrochenkov reviewed May 25, 2022

View reviewed changes

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 May 25, 2022

petrochenkov mentioned this pull request May 25, 2022

RFC3239: Implement cfg(target) - Part 1 #96909

Closed

Urgau force-pushed the cfg_accessible branch 2 times, most recently from fc9dc1a to a6156ca Compare May 25, 2022 18:09

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 May 25, 2022

This comment has been minimized.

Sign in to view

Urgau force-pushed the cfg_accessible branch from a6156ca to f63c2b7 Compare May 25, 2022 18:24

petrochenkov reviewed May 25, 2022

View reviewed changes

src/test/ui/conditional-compilation/cfg_accessible-bugs.rs Outdated Show resolved Hide resolved

compiler/rustc_resolve/src/macros.rs Show resolved Hide resolved

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2022

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 25, 2022

petrochenkov mentioned this pull request May 26, 2022

Add the known-bug test directive, use it, and do some cleanup #93953

Merged

Urgau added 4 commits May 30, 2022 16:51

Handle more cases in cfg_accessible

1633746

Add tests for "not sure" case of cfg_accessible

c413250

Add tests for private items of cfg_accessible

8337367

Add test for known-bugs of cfg_accessible

a4d03c6

Urgau force-pushed the cfg_accessible branch from f63c2b7 to 63705cb Compare May 30, 2022 15:08

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 May 30, 2022

petrochenkov reviewed Jun 2, 2022

View reviewed changes

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 Jun 2, 2022

Conservatively report "not sure" in cfg_accessible

b76d112

Urgau force-pushed the cfg_accessible branch from 57f03cc to b76d112 Compare June 3, 2022 09:43

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 Jun 3, 2022

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 Jun 4, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 5, 2022

bors merged commit 656eec8 into rust-lang:master Jun 5, 2022

rustbot added this to the 1.63.0 milestone Jun 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle more cases in cfg_accessible #97391

Handle more cases in cfg_accessible #97391

Urgau commented May 25, 2022

This comment has been minimized.

Urgau commented May 25, 2022

Urgau commented May 25, 2022

This comment has been minimized.

Urgau commented May 30, 2022

petrochenkov Jun 2, 2022

Urgau Jun 3, 2022

Urgau commented Jun 3, 2022

petrochenkov commented Jun 4, 2022

bors commented Jun 4, 2022

bors commented Jun 5, 2022

bors commented Jun 5, 2022

rust-timer commented Jun 5, 2022

Handle more cases in cfg_accessible #97391

Handle more cases in cfg_accessible #97391

Conversation

Urgau commented May 25, 2022

This comment has been minimized.

Urgau commented May 25, 2022

Urgau commented May 25, 2022

This comment has been minimized.

Urgau commented May 30, 2022

petrochenkov Jun 2, 2022

Choose a reason for hiding this comment

Urgau Jun 3, 2022

Choose a reason for hiding this comment

Urgau commented Jun 3, 2022

petrochenkov commented Jun 4, 2022

bors commented Jun 4, 2022

bors commented Jun 5, 2022

bors commented Jun 5, 2022

rust-timer commented Jun 5, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes