Implement initial version of `cfg(accessible(..))` by crlf0710 · Pull Request #137113 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

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.

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?

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 :)

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.

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?

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.

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(

Choose a reason for hiding this comment

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

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.

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(

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.

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 {

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.

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.

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.