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

Implement initial version of cfg(accessible(..)) #137113

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crlf0710
Copy link
Member

Implements an initial version of cfg(accessible(..)). The code includes a few FIXMEs, and i think i need some help with understanding how things suppose to work before i can solve them. Also, maybe i need some advice on organizing these code.

I'll assign to Vadim but feel free to reassign.

cc #64797

r? @petrochenkov

@crlf0710 crlf0710 added the F-cfg_accessible `#![feature(cfg_accessible)]` label Feb 16, 2025
@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 Feb 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@Urgau
Copy link
Member

Urgau commented Feb 16, 2025

This doesn't seems to share any code with the already implemented cfg_accessible macro.

What's the plan with that earlier attempt? Replace it with this one?

@crlf0710
Copy link
Member Author

What's the plan with that earlier attempt?

I guess we can decide that after the we work out the "advice on organizing these code" part :)

Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be existing tests at tests/ui/conditional-compilation/cfg_accessible-*, maybe these should stay in the same place? Or just make a new cfg-accessible directory and move everything there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, will move those files in the next push. Any advice on the implementation?

Copy link
Contributor

@tgross35 tgross35 Feb 18, 2025

Choose a reason for hiding this comment

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

I don't know too much about this area of the compiler unfortunately, petrochenkov is probably the expert here. Just happened to notice the existing tests while doing a driveby of this feature's history.

@bors
Copy link
Contributor

bors commented Feb 25, 2025

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

@petrochenkov
Copy link
Contributor

I think it makes sense to combine the this PR and the old implementation to implement this strategy.

  • It can be implemented as #[cfg(accessible)] (as opposed to #[cfg_accessible]) and resolver can be passed to cfg-expander, like in this PR.
  • The path resolution can just be taken from the old implementation, the special logic from cfg_accessible_crate/cfg_accessible_mod/cfg_accessible_item shouldn't be necessary.
  • If the resolution logic returns Indeterminate we must immediately produce a user-facing error, because we cannot postpone it if we are in cfg.
  • The existing test suite for cfg_accessible should be duplicated for cfg(accessible), preferably in the same files to highlight possible differences in behavior.
  • The time travel prevention should be implemented in any case, it's need for both cfg(accessible) and cfg_accessible.

I suggest starting with the last item and submitting it as a separate PR.
The examples of very similar logic can be found by searching for uses of single_segment_macro_resolutions and multi_segment_macro_resolutions, which are used for time travel prevention in macro paths.

match lib.cfg {
Some(ref cfg) => attr::cfg_matches(cfg, sess, CRATE_NODE_ID, None),
Some(ref cfg) => rustc_attr_parsing::eval_condition(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Some(ref cfg) => rustc_attr_parsing::eval_condition(
Some(ref cfg) => attr::cfg_matches(cfg, sess, CRATE_NODE_ID, None, None),

Is the CfgEval trait really necessary?
I think adding an optional resolver (*) parameter to eval_condition/cfg_matches would be enough.
Then the code here and in trait selection will require no changes besides adding a single None.

eval_condition should produce an error on encountering accessible if the resolver is None.

(*) Perhaps hidden under a trait, if the compiler crate dependency structure doesn't allow passing it directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, let me see if removing it works better.

eval_condition_inner(cfg, sess, features, eval).unwrap_or_default()
}

fn eval_condition_inner(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is necessary if None always turns into false anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

My intention was to make #[cfg(not(invalid_cfg))] and #[cfg(invalid_cfg)] both acts as #[cfg(FALSE)]. I can leave comment in the code if it's not obvious enough.

fn cfg_accessible_crate(&mut self, crate_name: Ident) -> Result<DefId, Indeterminate> {
let this = self;
let finalize = false;
let Some(binding) = this.extern_prelude_get(crate_name, finalize) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

One example why time travel is still necessary even with the new "restricted" rules - extern prelude changes as crate expands and extern_prelude_get can give different results at different points in time.

Copy link
Member Author

Choose a reason for hiding this comment

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

extern_prelude_get can give different results at different points in time

Now I realize i didn't really understand how the extern prelude works. I'll reach you on Zulip to gain better understanding on this bullet point before i continue.

@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 Mar 12, 2025
@crlf0710 crlf0710 marked this pull request as draft March 19, 2025 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) F-cfg_accessible `#![feature(cfg_accessible)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

None yet

6 participants