Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RFC: naming groups of configuration with
cfg_alias
#3804New 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
base: master
Are you sure you want to change the base?
RFC: naming groups of configuration with
cfg_alias
#3804Changes from all commits
304fefc
780ccd7
47d8ed1
a164189
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
In #3804 (comment) I had meant more that you could use
#[cfg(todo)]
as an attribute you could add/delete from sections of code, rather than togglingtodo
's value. So it would be kinda like howtodo!()
always panics, rather than disappearing if you toggle some flag.e.g.:
later, once that api is implemented, you can just delete the
#[cfg(todo)]
line to have your code no longer be skipped.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'd probably drop ", including..." here as it makes it more rather than less ambiguous. Alternatively, it'd be OK to say e.g. "including (but not limited to) combining operators such as
all
,any
, andnot
.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.
Could you add an example of an alias being used within an alias to make clear that that should work.
The example in https://crates.io/crates/cfg_aliases provides a good motivating case. Might also be good to pull ideas from that readme for the Summary section
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.
Have a look at the grammar syntax in the Reference. Probably best to just use that.
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.
to clarify, if we passed
--cfg 'foo="bar"'
, it meanscfg_alias(foo, ...)
will be also conflicting right?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.
Does this make the compatibility hazard worse than today? Code can work perfectly fine but will then break if a new builtin is added or somewhere else in the dependency tree defines a new cfg where the end-user would do
RUSTFLAGS=--cfg=...
(remember: RUSTFLAGS are generally global to the entire dependency tree)Is that acceptable? Or can we mitigate this somehow?
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.
An alternative, which might be more consistent with allowing this attribute more generally at a module/item level:
This would preclude using
cfg
aliases for crate-level attributes, unless we implement it in Cargo as well, but it would also avoid difficult concerns with ordering: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.
Why module level? Once we eat the cost of making these scoped, I'm curious if there's a reason we shouldn't specify them for other scopes also.
In terms of how we specify the language, it actually makes the specification simpler to have fewer rather than more exceptions. (We can always of course still incrementally stabilize if there are reasons to do so.)
Of course, if we go with a different design, such as leaning into macros somehow, then we could sidestep this question.
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.
Please add some details on hygiene. For instance, what happens if you write a
cfg_alias
on amacro_defining_a_fn!()
? Does the macro "see" the alias set, ignoring hygiene? Or do you need to pass the identifier into the macro (e.g.macro_defining_a_fn!(the_alias)
, so that hygiene works? I would expect the latter.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.
cc @ehuss @eholk
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.
What about shadowing? IMO it should be supported like we do for any other scoped variable:
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.
Inspired by the above, one alternative that comes to mind is declarative attribute macros that do the cfg matching for you. I think actually you have to declare two macros, one when the cfg you want is true and one when it is false, so that's a major drawback because it would require repeating the same clause twice.
However, attribute macros (possibly in combination with this feature) would allow a crate to "export" an alias.
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.
Great point, I'll mention that. One other downside is that with a specific set of config tied to an attribute macro, it wouldn't be easily possible to combine with other config in
all
orany
(could probably be done with the attribute macro's parameters).Exporting would be quite convenient at times.
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.
Good point that the macro approach doesn't compose all that well. I wish there was an obvious way to support exporting these.
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.
For a more radical design, it might be possible to treat aliases as effectively a new kind of macro; something that shares the macro namespace but only expands within
cfg
. I think this would mean the new macro scoping can be used, sopub use some_alias
can make an alias crate-public for another crate to import withuse crate_with_alias::some_alias
.It sounds borderline too complex for an otherwise pretty simple feature, but being able to do that could be a nice help if public macros expand to code that contains
#[cfg(...)]
.With that, it would almost be possible to define the builtin
cfg(windows)
/cfg(unix)
as something likecfg_alias(windows = target_os = "windows")
in the prelude (not that we'd have any reason to actually do that).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 would expect some kind of way to export an alias. I'd be disappointed with a design that wouldn't permit it.
Anyway, your comment about macros made me think of something like...
That has the advantage of not needing a new kind of attribute or new syntax to define an alias. It also makes it obvious when an alias is being used.
Moreover this syntax implies that you can pass arguments.
Let me give an example where this would be useful to me personally. The Python C api has different ABI guarantees. Take
PyObject_Vectorcall
for example. This function was added in Python 3.9, but only in 3.12 it was added to the Stable ABI.That means I define the bindings as:
This would be a lot simpler if the syntax is macro-like:
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 do think @mejrs that this would be quite nice. I suspect it might be difficult to implement, though. Maybe what's needed is an experiment.
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.
+1 for that.
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.
Since this has now been added to the RFC, this alternative can be removed.
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.
The cfg_aliases crate could be mentioned. https://crates.io/crates/cfg_aliases
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.
If we decide that the complexity of allowing this at a module level is too high, that is, only allowing it as a crate attribute, then I would argue that the feature would be better suited to only being accessible in Cargo /
--cfg-alias
.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 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 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 strongly suspect that having this feature in Cargo will be desired, and that having it there would solve 90% of the use-case for this feature.
Implementation-wise, I also suspect that doing it would be possible and fairly simple in Cargo today by parsing the
cfg
itself (it already understandscfg
s as part of dependency resolving), and then passing extra--cfg
s /--check-cfg
s to the compiler.So maybe it would be valuable to implement (or at least stabilize) the ability for doing this in Cargo first, and only later consider making this an attribute in the language itself?
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.
This gets into a weird gray area of Cargo.
Defining
--cfg
s is a rustc concept that is lower level than Cargo. In fact, there was a proposed[cfg]
table for--check-cfg
which as rejected for that reason. Instead, cfgs are either defined by the environment or by[features]
. We do recognize there is a gap for more--cfg
use cases and have done some brainstorming on "global features" but more work is needed.Cargo does allow reading of cfg's through build script environment variables and through
target.*
tables.Depending on how you look at it, a
--cfg-alias
is like--cfg
and too low level for Cargo. As people want--cfg
s to influence "global features", maybe there is something there that can be designed that can fill both needs. Or--cfg
is a helper for#[cfg()]
s and would fit similar to build scripts andtarget.*
tables.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.
As pointed out, there is a build script to emulate cfgs: https://github.com/rust-lang/rfcs/pull/3804/files#r2059296180
With metabuild, we could go a step further in semi-native cargo support. This would allow something like
(newlines in the inline table is me being hopeful that TOML 1.1 is finally unblocked)