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

Use of feature flagged functions in std::cfg macro #71679

Closed
dfreese opened this issue Apr 29, 2020 · 2 comments · Fixed by #71692
Closed

Use of feature flagged functions in std::cfg macro #71679

dfreese opened this issue Apr 29, 2020 · 2 comments · Fixed by #71692
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@dfreese
Copy link
Contributor

dfreese commented Apr 29, 2020

I tried this code:

#[cfg(feature = "enabled")]
pub fn conditionally_enabled() -> i64 {
    42
}

pub fn always_enabled() -> i64 {
    9001
}

pub fn integration() -> i64 {
    if cfg!(feature = "enabled") {
        conditionally_enabled()
    } else {
        always_enabled()
    }
}

I expected to see this compile based on the std::cfg macro docs, instead, this it failed to compile. This was confusing, as I was able to write this, in a slightly less clear way, using cfg attributes. The documentation calls out the links between the two, saying The syntax given to this macro is the same syntax as the cfg attribute.

Should the docs for std::cfg be updated to state that both branches of the if/else need to be valid regardless of the cfg! condition? Or is this something that would be expected to compile?

Meta

This was tested on 1.42.0, but from compiler explorer, this shows up in all versions since 1.0.0
rustc --version --verbose:

rustc 1.42.0 (b8cedc004 2020-03-09)
@dfreese dfreese added the C-bug Category: This is a bug. label Apr 29, 2020
dfreese referenced this issue in dfreese/prost Apr 29, 2020
Opening this as a way of opening the discussion about an issue that I
was running into.  If you would prefer a different approach, I'm happy
to discuss that.

Within multi-language environments, such as bazel, cargo isn't always
able to drive the build process.  However, prost and prost-build
provides an amazing foundation of which to build off.  The general idea
of this commit was to open up prost-build to having tools built off of
it, while still preserving previous behavior.

Currently, if PROTOC or PROTO_INCLUDE are not set at build-time, then
the build fails.  This commit introduces feature, runtime-protoc. This
hides env! invocations when enabled, allowing compilation, under the
assumption that the protoc location will be provided at runtime.
@Mark-Simulacrum
Copy link
Member

We can update the docs, for sure, but I'm not sure what to say -- maybe something like "this simply evaluates to true/false"? cfg! is basically not special to the compiler, it doesn't ever remove code, so everything still needs to compile (unlike #[cfg(...)] which will remove code if the expression doesn't evaluate to true).

@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools E-help-wanted Call for participation: Help is requested to fix this issue. and removed C-bug Category: This is a bug. labels Apr 29, 2020
@dfreese
Copy link
Contributor Author

dfreese commented Apr 29, 2020

To rephrase, slightly, how about adding the following to the docs:

cfg!, unlike #[cfg(...)], does not remove any code and only evaluates to true or false. For example, this means all code in an if/else block needs to be valid when cfg! is used for the condition, regardless of what cfg! is evaluating.

dfreese added a commit to dfreese/prost that referenced this issue Apr 29, 2020
Opening this as a way of opening the discussion about an issue that I
was running into.  If you would prefer a different approach, I'm happy
to discuss that.

Within multi-language environments, such as bazel, cargo isn't always
able to drive the build process.  However, prost and prost-build
provides an amazing foundation of which to build off.  The general idea
of this commit was to open up prost-build to having tools built off of
it, while still preserving previous behavior.

Currently, if PROTOC or PROTO_INCLUDE are not set at build-time, then
the build fails.  This commit introduces feature, runtime-protoc. This
hides env! invocations when enabled, allowing compilation, under the
assumption that the protoc location will be provided at runtime.  This
wouldn't be my first preference, but I was prioritizing the build-break
if the environment flags were not specified.  I would generally have
preferred grabbing those with option_env! macros and then trying to
gracefully handle the error at runtime protoc has not been provided at
build time or runtime similar, since this is already covered by tests.
That would allow removal of the feature flag.  It would also make the
code a little cleaner, since there's not a great way to do conditionals
with cfg (see rust-lang/rust#71679).
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 30, 2020
Add clarification on std::cfg macro docs v. #[cfg] attribute

The wording was discussed, to a limited degree in rust-lang#71679.  This tries to
address some confusion I as well as someone else had independently when
looking at this macro.

Fixes rust-lang#71679
@bors bors closed this as completed in 4813a81 Apr 30, 2020
dfreese added a commit to dfreese/prost that referenced this issue May 5, 2020
Opening this as a way of opening the discussion about an issue that I
was running into.  If you would prefer a different approach, I'm happy
to discuss that.

Within multi-language environments, such as bazel, cargo isn't always
able to drive the build process.  However, prost and prost-build
provides an amazing foundation of which to build off.  The general idea
of this commit was to open up prost-build to having tools built off of
it, while still preserving previous behavior.

Currently, if PROTOC or PROTO_INCLUDE are not set at build-time, then
the build fails.  This commit introduces feature, runtime-protoc. This
hides env! invocations when enabled, allowing compilation, under the
assumption that the protoc location will be provided at runtime.  This
wouldn't be my first preference, but I was prioritizing the build-break
if the environment flags were not specified.  I would generally have
preferred grabbing those with option_env! macros and then trying to
gracefully handle the error at runtime protoc has not been provided at
build time or runtime similar, since this is already covered by tests.
That would allow removal of the feature flag.  It would also make the
code a little cleaner, since there's not a great way to do conditionals
with cfg (see rust-lang/rust#71679).
dfreese added a commit to dfreese/prost that referenced this issue May 17, 2020
Opening this as a way of opening the discussion about an issue that I
was running into.  If you would prefer a different approach, I'm happy
to discuss that.

Within multi-language environments, such as bazel, cargo isn't always
able to drive the build process.  However, prost and prost-build
provides an amazing foundation of which to build off.  The general idea
of this commit was to open up prost-build to having tools built off of
it, while still preserving previous behavior.

Currently, if PROTOC or PROTO_INCLUDE are not set at build-time, then
the build fails.  This commit introduces feature, runtime-protoc. This
hides env! invocations when enabled, allowing compilation, under the
assumption that the protoc location will be provided at runtime.  This
wouldn't be my first preference, but I was prioritizing the build-break
if the environment flags were not specified.  I would generally have
preferred grabbing those with option_env! macros and then trying to
gracefully handle the error at runtime protoc has not been provided at
build time or runtime similar, since this is already covered by tests.
That would allow removal of the feature flag.  It would also make the
code a little cleaner, since there's not a great way to do conditionals
with cfg (see rust-lang/rust#71679).
dfreese added a commit to dfreese/prost that referenced this issue May 17, 2020
Opening this as a way of opening the discussion about an issue that I
was running into.  If you would prefer a different approach, I'm happy
to discuss that.

Within multi-language environments, such as bazel, cargo isn't always
able to drive the build process.  However, prost and prost-build
provides an amazing foundation of which to build off.  The general idea
of this commit was to open up prost-build to having tools built off of
it, while still preserving previous behavior.

Currently, if PROTOC or PROTO_INCLUDE are not set at build-time, then
the build fails.  This commit introduces feature, runtime-protoc. This
hides env! invocations when enabled, allowing compilation, under the
assumption that the protoc location will be provided at runtime.  This
wouldn't be my first preference, but I was prioritizing the build-break
if the environment flags were not specified.  I would generally have
preferred grabbing those with option_env! macros and then trying to
gracefully handle the error at runtime protoc has not been provided at
build time or runtime similar, since this is already covered by tests.
That would allow removal of the feature flag.  It would also make the
code a little cleaner, since there's not a great way to do conditionals
with cfg (see rust-lang/rust#71679).
dfreese added a commit to dfreese/prost that referenced this issue May 17, 2020
Opening this as a way of opening the discussion about an issue that I
was running into.  If you would prefer a different approach, I'm happy
to discuss that.

Within multi-language environments, such as bazel, cargo isn't always
able to drive the build process.  However, prost and prost-build
provides an amazing foundation of which to build off.  The general idea
of this commit was to open up prost-build to having tools built off of
it, while still preserving previous behavior.

Currently, if PROTOC or PROTO_INCLUDE are not set at build-time, then
the build fails.  This commit introduces feature, runtime-protoc. This
hides env! invocations when enabled, allowing compilation, under the
assumption that the protoc location will be provided at runtime.  This
wouldn't be my first preference, but I was prioritizing the build-break
if the environment flags were not specified.  I would generally have
preferred grabbing those with option_env! macros and then trying to
gracefully handle the error at runtime protoc has not been provided at
build time or runtime similar, since this is already covered by tests.
That would allow removal of the feature flag.  It would also make the
code a little cleaner, since there's not a great way to do conditionals
with cfg (see rust-lang/rust#71679).
dfreese added a commit to dfreese/prost that referenced this issue May 17, 2020
Opening this as a way of opening the discussion about an issue that I
was running into.  If you would prefer a different approach, I'm happy
to discuss that.

Within multi-language environments, such as bazel, cargo isn't always
able to drive the build process.  However, prost and prost-build
provides an amazing foundation of which to build off.  The general idea
of this commit was to open up prost-build to having tools built off of
it, while still preserving previous behavior.

Currently, if PROTOC or PROTO_INCLUDE are not set at build-time, then
the build fails.  This commit introduces feature, runtime-protoc. This
hides env! invocations when enabled, allowing compilation, under the
assumption that the protoc location will be provided at runtime.  This
wouldn't be my first preference, but I was prioritizing the build-break
if the environment flags were not specified.  I would generally have
preferred grabbing those with option_env! macros and then trying to
gracefully handle the error at runtime protoc has not been provided at
build time or runtime similar, since this is already covered by tests.
That would allow removal of the feature flag.  It would also make the
code a little cleaner, since there's not a great way to do conditionals
with cfg (see rust-lang/rust#71679).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants