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

201/pass cfg flags #214

Merged
merged 4 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cargo-marker/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,12 @@ pub fn run_check(config: &Config, info: CheckInfo, additional_cargo_args: &[Stri
pub fn to_marker_lint_crates_env(lints: &[LintCrate]) -> OsString {
let lint_paths: Vec<_> = lints
.iter()
.map(|krate| OsString::from(krate.file.as_os_str()))
.map(|krate| {
let mut env_str = OsString::from(&krate.name);
env_str.push(":");
env_str.push(krate.file.as_os_str());
env_str
})
.collect();
lint_paths.join(OsStr::new(";"))
}
2 changes: 2 additions & 0 deletions cargo-marker/src/backend/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ pub struct LintCrateSource {
/// The information of a compiled lint crate.
#[derive(Debug)]
pub struct LintCrate {
/// The name of the crate
pub name: String,
/// The absolute path of the compiled crate, as a dynamic library.
pub file: PathBuf,
}
Expand Down
64 changes: 38 additions & 26 deletions cargo-marker/src/backend/lints/build.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{ffi::OsStr, path::Path};
use std::{collections::HashSet, ffi::OsStr, path::Path};

use crate::{backend::Config, ExitStatus};

Expand Down Expand Up @@ -26,40 +26,52 @@ const ARTIFACT_ENDINGS: &[&str] = &[
pub fn build_lints(sources: &[LintCrateSource], config: &Config) -> Result<Vec<LintCrate>, ExitStatus> {
// By default Cargo doesn't provide the path of the compiled lint crate.
// As a work around, we use the `--out-dir` option to make cargo copy all
// created binaries into one folder. We then scan that folder and collect
// all dynamic libraries, assuming that they're lint crates.
// created binaries into one folder. We then scan that folder as we build our crates
// and collect all dynamic libraries that show up, we link the name of the crate we just built
// with the file(s) we found after we built it, assuming that they are related.
//
// TODO Look into how to optimize this a bit better:
// Another option that would be potentially more performant is if we built each
// crate in it's own target dir. this would eliminate the need for HashSet<_> below, without
// changing too much else about the implementation.
//
// This would be so much simpler if we could get an output name from Cargo

// Clear previously build lints
let lints_dir = config.lint_crate_dir();
clear_lints_dir(&lints_dir)?;

// Build lint crates
// Build lint crates and find the output of those builds
let mut found_paths = HashSet::new();
let ending = OsStr::new(DYNAMIC_LIB_FILE_ENDING);
let mut lints = Vec::with_capacity(sources.len());

for lint_src in sources {
build_lint(lint_src, config)?;
}

// Find lint lint crates
match std::fs::read_dir(&lints_dir) {
Ok(dir) => {
let ending = OsStr::new(DYNAMIC_LIB_FILE_ENDING);
let mut lints = vec![];
for file in dir {
let file = file.unwrap().path();
if file.extension() == Some(ending) {
lints.push(LintCrate { file });
match std::fs::read_dir(&lints_dir) {
Comment on lines 49 to +51
Copy link
Member

Choose a reason for hiding this comment

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

As you noted above, it feels inefficient to scan the directory after every compilation. cargo metadata sadly doesn't provide any information how the build file will be called.

I see a few solutions:

  1. Leaving it like this, it seems to work

  2. Your suggestion of changing the target directory per crate

  3. Guess the output file name and error if the name can't be found. The dynamic library names are constructed by adding a lib prefix on unix systems and the OS-specific file ending. Here is a draft of the code:

    // Probably store these above as constants with `DYNAMIC_LIB_FILE_ENDING`
    #[cfg(unix)]
    let lib_file_prefix = "lib";
    #[cfg(windows)]
    let lib_file_prefix = "";
    
    // ...
    let dir = &lints_dir;
    for lint_src in sources {
        build_lint(lint_src, config)?;
    
        let name = lint_src.name.clone();
        let file = dir.join(format!("{lib_file_prefix}{name}.{DYNAMIC_LIB_FILE_ENDING}"));
        if file.exists() {
            lints.push(LintCrate { name, file });
        }
    }
  4. Guess the file name with more confidence. Option 3 would fail, if the user specified a name field in the [lib] section, as that changes the name. This is where we can do some more hackery:

    Cargo's build command supports the --config KEY=VALUE argument, to override parts of the manifest (At least from my understanding). We should be able to use this during lint compilation to prevent any weird renames, by setting the name ourself: --config "lib.name = '<lint_crate_name>'". Then we can just apply option 3 to confidently guess that the file actually exists and uses the name we expect.

Would you mind looking at option 4? It's totally fine if not, this is already a really nice addition to Marker. Option 2, 3 and 4 can also be added later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can absolutely look into number 4, But I'd like to focus on getting this in and the user documentation ready first. I'm inclined to handle making this path more efficient as a separate issue, but If you think it should be part of this, I can work on it here.

I was trying to think if we'd get any real sandboxing type benefits from option 2, but I don't really see a point in making sure that some lints can't use build artifacts of others. it would just be more file system usage with little benefit. Besides, if there's one thing I learned writing C#, it's that windows doesn't like deeply nested files, so don't make folders unless you need them :)

Copy link
Member

Choose a reason for hiding this comment

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

For me it's fine if we continue this implementation and do the path cleanup in a later PR. This system should have the same robustness level :)

Ok(dir) => {
for file in dir {
let file = file.unwrap().path();
if file.extension() == Some(ending) && !found_paths.contains(&file) {
found_paths.insert(file.clone());
lints.push(LintCrate {
file,
name: lint_src.name.clone(),
});
}
}
}
Ok(lints)
},
Err(err) => {
// This shouldn't really be a point of failure. In this case, I'm
// more interested in the HOW?
panic!(
"unable to read lints dir after lint compilation: {} ({err:#?})",
lints_dir.display()
);
},
},
Err(err) => {
// This shouldn't really be a point of failure. In this case, I'm
// more interested in the HOW?
panic!(
"unable to read lints dir after lint compilation: {} ({err:#?})",
lints_dir.display()
);
},
NathanFrasier marked this conversation as resolved.
Show resolved Hide resolved
}
}
Ok(lints)
}

/// This function clears the `marker/lints` directory holding all compiled lints. This
Expand Down
1 change: 1 addition & 0 deletions docs/book/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
[Introduction](./README.md)
- [Usage](./usage.md)
- [Installation](./usage/installation.md)
- [Setting Lint Levels](./usage/setting-lint-levels.md)
- [Lint Crate Declaration](./usage/lint-crate-declaration.md)
- [Lint Crate Security](./usage/lint-crate-security.md)
- [Lint Development](./lint-dev.md)
Expand Down
56 changes: 56 additions & 0 deletions docs/book/src/usage/setting-lint-levels.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# Setting Lint Levels
Once your crate is configured to run some lints, it quickly becomes important to control the lint levels within your
code. Marker provides the ability to use normal lint control attributes like `#[allow(...)]` `#[deny(...)]` and others
to control the behavior of marker lints.

## On nightly
Marker uses the `marker::` tool prefix for lints. In order for rustc to recognize this prefix, it requires marker to be
registered via the [`#[register_tool()]` feature](https://github.com/rust-lang/rust/issues/66079).

If your crate is compiled using nightly, then controlling lints is as simple as placing `#![feature(register_tool)]`
and `#![register_tool(marker)]` at the top of your crate `lib.rs` or `mod.rs` file. You can then use all of the normal
lint attributes you might usually use, on marker provided lints, like `#[allow(marker::my_lint)]`.


#### Nightly Required Attributes in `lib.rs` / `main.rs`
```rust
#![feature(register_tool)]
#![register_tool(marker)]
```

#### Nightly Lint Attribute
```
// Marker lints can be controlled like this
#[allow(marker::my_lint)]
fn foo() {}
```

## On Stable
If your project isn't on nightly, you can still use the `register_tool` attribute, you'll just need to add some extra
guards around it. Marker provides config arguments to rust during the lint passes, so that your linted code can
conditionally apply attributes only during the marker run.

Specifically marker passes `--cfg=marker` and a `--cfg=marker="my_crate"` flag for each lint crate. This means that you
can use `#[cfg_attr(marker, foo)]` to conditionally apply the `foo` attribute only during lint runs.

This conditional compilation can be used to leverage the fact that marker uses nightly for linting, without requiring
the project to use a nightly toolchain. To do this, add `#![cfg_attr(marker, feature(register_tool))]` and
`#![cfg_attr(marker, register_tool(marker))]` attributes to the top of your crate file, to register marker as a tool.
Then you can apply lint level attributes like `#[cfg_attr(marker, allow(marker::foo))]` to control your marker lints.

Additionally, you can check if a specific lint crate is in the set of loaded lint crates. This is useful, when you
only want to enable some attributes if specific lints are loaded. For this you can use a `marker = "<lint-crate-name>"`
check, like this: `#[cfg_attr(marker = "my_crate", allow(marker::foo))]`.

#### Stable Required Attributes in `lib.rs` / `main.rs`
```rust
#![cfg_attr(marker, feature(register_tool))]
#![cfg_attr(marker, register_tool(marker))]
```

#### Stable Conditional Lint Attribute
```
// Marker lints can be controlled like this
#[cfg_attr(marker, allow(marker::my_lint))]
fn foo() {}
```
2 changes: 1 addition & 1 deletion marker_adapter/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ Marker's API requires some callbacks from the lint crates into the driver. The a

An adapter instance can be crated from the environment. For this, the following environment values are read:

* `MARKER_LINT_CRATES`: A semicolon separated list of (absolute) paths to lint crates compiled as dynamic libraries.
* `MARKER_LINT_CRATES`: A semicolon separated list of crate name and absolute path pairs. Each pair is internally separated by a colon.

## Contributing

Expand Down
16 changes: 13 additions & 3 deletions marker_adapter/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ use super::{AdapterError, LINT_CRATES_ENV};
/// A struct describing a lint crate that can be loaded
#[derive(Debug, Clone)]
pub struct LintCrateInfo {
/// The name of the lint crate
pub name: String,
/// The absolute path of the compiled dynamic library, which can be loaded as a lint crate.
pub path: PathBuf,
}
Expand All @@ -25,9 +27,17 @@ impl LintCrateInfo {
pub fn list_from_env() -> Result<Vec<LintCrateInfo>, AdapterError> {
let env_str = std::env::var_os(LINT_CRATES_ENV).ok_or(AdapterError::LintCratesEnvUnset)?;

Ok(std::env::split_paths(&env_str)
.map(|path| LintCrateInfo { path })
.collect())
let mut lint_crates = vec![];
for item in env_str.to_str().ok_or(AdapterError::LintCratesEnvMalformed)?.split(';') {
let mut item_parts = item.splitn(2, ':');
let name = item_parts.next().ok_or(AdapterError::LintCratesEnvMalformed)?;
let path = item_parts.next().ok_or(AdapterError::LintCratesEnvMalformed)?;
NathanFrasier marked this conversation as resolved.
Show resolved Hide resolved
lint_crates.push(LintCrateInfo {
name: name.to_string(),
path: PathBuf::from(path),
});
}
Ok(lint_crates)
}
}

Expand Down
13 changes: 12 additions & 1 deletion marker_rustc_driver/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ fn main() {
// in preparation.
if !has_sys_root_arg {
let sys_root = find_sys_root(sys_root_arg);
orig_args.extend(vec!["--sysroot".into(), sys_root]);
orig_args.extend(["--sysroot".into(), sys_root]);
};

// make "marker_rustc_driver --rustc" work like a subcommand that passes
Expand Down Expand Up @@ -302,6 +302,17 @@ fn main() {
Err(marker_adapter::AdapterError::LintCratesEnvUnset) => vec![],
Err(err) => panic!("Error while determining the lint crates to load: {err:#?}"),
};

// We need to provide a marker cfg flag to allow conditional compilation,
// we add a simple `marker` config for the common use case, but also provide
// `marker=crate_name` for more complex uses
orig_args.push("--cfg=marker".into());
orig_args.extend(
lint_crates
.iter()
.map(|krate| format!(r#"--cfg=marker="{}""#, krate.name)),
);

let mut callback = MarkerCallback { env_vars, lint_crates };
rustc_driver::RunCompiler::new(&orig_args, &mut callback).run()
} else {
Expand Down
22 changes: 22 additions & 0 deletions marker_uilints/tests/ui/cfg_attr_check.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#![cfg_attr(marker, feature(register_tool))]
#![cfg_attr(marker, register_tool(marker))]

mod allow_with_simple_attr {
#[cfg_attr(marker, allow(marker::item_with_test_name))]
fn find_me_fn() {}
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
}

mod allow_with_crate_check_attr {
#[cfg_attr(marker = "marker_uilints", allow(marker::item_with_test_name))]
fn find_me_fn() {}
}

mod lint_with_unloaded_crate_attr {
#[cfg_attr(marker = "some_unknown_crate_that_isnt_loaded", allow(marker::item_with_test_name))]
fn find_me_fn() {}
}

mod unknown_lint_allow {
#[cfg_attr(marker, allow(marker::some_unknown_lint_that_doesnt_exist))]
fn foo() {}
}
18 changes: 18 additions & 0 deletions marker_uilints/tests/ui/cfg_attr_check.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
warning: unknown lint: `marker::some_unknown_lint_that_doesnt_exist`
--> $DIR/cfg_attr_check.rs:20:30
|
20 | #[cfg_attr(marker, allow(marker::some_unknown_lint_that_doesnt_exist))]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unknown_lints)]` on by default

warning: found a `fn` item with a test name
--> $DIR/cfg_attr_check.rs:16:5
|
16 | fn find_me_fn() {}
| ^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(marker::item_with_test_name)]` on by default

warning: 2 warnings emitted