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

Lint for cfg usage which would provide a misleading result in a build script. #9419

Open
thomcc opened this issue Sep 2, 2022 · 4 comments · May be fixed by #12862
Open

Lint for cfg usage which would provide a misleading result in a build script. #9419

thomcc opened this issue Sep 2, 2022 · 4 comments · May be fixed by #12862
Labels
A-lint Area: New lints

Comments

@thomcc
Copy link
Member

thomcc commented Sep 2, 2022

What it does

Lints for uses of cfg!, #[cfg], #[cfg_attr] with keys which will have misleading1 values when used inside build scripts (e.g. build.rs files), such as windows, unix, and the ones that begin with target_*.

This is a restriction lint, as I'm assuming Clippy has no way of figuring out if we're in a build script on it's own (it seems completely impossible, as build scripts are just normal rust code).

The idea is that developers of build scripts that support cross compilation can add something like #![deny(clippy::misleading_cfg_in_build_script)].

Lint Name

misleading_cfg_in_build_script

Category

restriction

Advantage

The recommended code produces the expected value even when cross-compiling.

Drawbacks

  • This is not a useful lint outside of build scripts. The advice it gives will be wrong in other environments.
  • Unlike with #[cfg], there may be no alternative for uses of #[cfg]/#[cfg_attr] (perhaps that code needs to not compile).
  • Even within a build scripts, it may be impossible or impractical to support cross compilation.
  • Even within build-scripts that support cross-compilation, you need to use cfg! to detect if you're cross-compiling.
  • The correct equivalent is significantly more complicated and much harder to write correctly.
  • It's an opt-in restriction lint, so the usefulness is inherently limited.

Example

The correct replacement is a bit tricky and has several cases, and IDK if Clippy should even suggest a replacement. Also, it's worth noting that none of these have any hope of working outside a build script.

Anyway, there are a few cases here:

For no-value cfgs, like cfg!(windows) or cfg!(unix)

if cfg!(windows) { ... }

Can be written as:

if std::env::var("CARGO_CFG_WINDOWS").is_ok() { ... }

For single-value cfgs, like cfg!(target_os = ...)

if cfg!(target_os = "macos") { ... }

Can be written as:

if std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_default() == "macos" { ... }

For multi-value cfgs, like cfg!(target_feature = ...) or target_family

(This also works for single-value cfgs, but you can see why it's less desirable)

if cfg!(target_feature = "crt-static") { ... }

Can be written as:

if std::env::var("CARGO_CFG_TARGET_FEATURE").unwrap_or_default().split(',').any(|f| f == "crt-static") { ... }

The complexity is caused by multi-value cfg being provided by cargo as comma-separated environment vars. This not a theoretical issue either (as crt-static is definitely useful for build scripts).

Footnotes

  1. These will report the values for the host, not the target, e.g. cfg!(windows) is true when compiling on Windows, so will give the wrong answer if cross compiling. This is because build scripts run on the machine performing compilation, rather than on the target.

@thomcc thomcc added the A-lint Area: New lints label Sep 2, 2022
@thomcc
Copy link
Member Author

thomcc commented Sep 3, 2022

I'm not sure if this is too niche, and will understand if it is. I've wanted it for a long time, though.

@smoelius
Copy link
Contributor

smoelius commented Sep 3, 2022

I think this is still true(?): #2802

@GuillaumeGomez
Copy link
Member

Taking a look.

@GuillaumeGomez GuillaumeGomez linked a pull request May 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants