-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in compiler/rustc_attr_parsing These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This doesn't seems to share any code with the already implemented What's the plan with that earlier attempt? Replace it with this one? |
I guess we can decide that after the we work out the "advice on organizing these code" part :) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
☔ The latest upstream changes (presumably #135726) made this pull request unmergeable. Please resolve the merge conflicts. |
I think it makes sense to combine the this PR and the old implementation to implement this strategy.
I suggest starting with the last item and submitting it as a separate PR. |
match lib.cfg { | ||
Some(ref cfg) => attr::cfg_matches(cfg, sess, CRATE_NODE_ID, None), | ||
Some(ref cfg) => rustc_attr_parsing::eval_condition( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Implements an initial version of
cfg(accessible(..))
. The code includes a fewFIXME
s, 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