Skip to content

Commit

Permalink
Deprecate several fields (#606)
Browse files Browse the repository at this point in the history
This PR deprecates several fields which will be removed in a future
update. I'll explain in detail why below, but the TLDR is that
cargo-deny surfaces several configuration options that were added
because we _could_, but not necessarily because they are useful in
practice.

## Licenses

###
[`deny`](https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-allow-and-deny-fields-optional)

This field was only added for consistency with `[bans]` but makes no
sense for `[licenses]`, if a license you don't explicitly allow is used
it is implicitly denied.

###
[`copyleft`](https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-copyleft-field-optional)

There is no reason to treat these differently from any other license, if
it's not explicitly allowed it should be denied, and it just adds
confusion due to the terrible default.

See: #602
See: #354

###
[`allow-osi-fsf-free`](https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-allow-osi-fsf-free-field-optional)

Similarly to copyleft, this field just makes no sense and was only added
because the SPDX metadata allowed us to query this information.

###
[`default`](https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-default-field-optional)

This was added so that users could just ignore/warn all their
dependencies not following the set of allowed licenses, but just isn't
much value. Even in large projects with literally hundreds of external
dependencies the set of licenses that need to be allowed are relatively
small compared to the total set of licenses in SPDX due to the Rust
ecosystem generally using only a handful of licenses, with rare
exceptions.

###
[`unlicensed`](https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-unlicensed-field-optional)

Crates that don't specify a license via `[package.license/license-file]`
or have a license file in their package source are incredibly rare, and
there is already a
[mechanism](https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-clarify-field-optional)
to provide/override license information for those rare crates.

## Advisories

### Blanket

-
[`vulnerability`](https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html#the-vulnerability-field-optional)
-
[`unmaintained`](https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html#the-unmaintained-field-optional)
-
[`unsound`](https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html#the-unsound-field-optional)
-
[`notice`](https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html#the-notice-field-optional)

There's no need to blanket handle any of these specific advisory types,
there just aren't enough advisories (currently, this could change in the
future) that a typical workspace will encounter that they can't be
handled explicitly via `ignore`.

See: #449

###
[`severity-threshold`](https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html#the-severity-threshold-field-optional)

This optional field is available in rustsec advisories, but provides no
real value as it's just flavor on top of a reported vulnerability, but
doesn't fundamentally change that it is a vulnerability, and can either
be ignored or better yet, updated to a version without the
vulnerability.
  • Loading branch information
Jake-Shadle authored Feb 21, 2024
1 parent 3b13cc9 commit 800c768
Show file tree
Hide file tree
Showing 9 changed files with 324 additions and 115 deletions.
78 changes: 71 additions & 7 deletions src/advisories/cfg.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{
cfg::{PackageSpecOrExtended, Reason, ValidationContext},
diag::{Diagnostic, FileId, Label},
LintLevel, PathBuf, Spanned,
LintLevel, PathBuf, Span, Spanned,
};
use rustsec::advisory;
use time::Duration;
Expand Down Expand Up @@ -44,6 +44,7 @@ pub struct Config {
/// use the '.' separator instead of ',' which is used by some locales and
/// supported in the RFC3339 format, but not by this implementation
pub maximum_db_staleness: Spanned<Duration>,
deprecated: Vec<Span>,
}

impl Default for Config {
Expand All @@ -62,6 +63,7 @@ impl Default for Config {
git_fetch_with_cli: None,
disable_yank_checking: false,
maximum_db_staleness: Spanned::new(Duration::seconds_f64(NINETY_DAYS)),
deprecated: Vec::new(),
}
}
}
Expand Down Expand Up @@ -97,13 +99,20 @@ impl<'de> toml_span::Deserialize<'de> for Config {
} else {
Vec::new()
};
let vulnerability = th.optional("vulnerability").unwrap_or(LintLevel::Deny);
let unmaintained = th.optional("unmaintained").unwrap_or(LintLevel::Warn);
let unsound = th.optional("unsound").unwrap_or(LintLevel::Warn);

use crate::cfg::deprecated;

let mut fdeps = Vec::new();

let vulnerability =
deprecated(&mut th, "vulnerability", &mut fdeps).unwrap_or(LintLevel::Deny);
let unmaintained =
deprecated(&mut th, "unmaintained", &mut fdeps).unwrap_or(LintLevel::Warn);
let unsound = deprecated(&mut th, "unsound", &mut fdeps).unwrap_or(LintLevel::Warn);
let yanked = th
.optional_s("yanked")
.unwrap_or(Spanned::new(LintLevel::Warn));
let notice = th.optional("notice").unwrap_or(LintLevel::Warn);
let notice = deprecated(&mut th, "notice", &mut fdeps).unwrap_or(LintLevel::Warn);
let (ignore, ignore_yanked) = if let Some((_, mut ignore)) = th.take("ignore") {
let mut u = Vec::new();
let mut y = Vec::new();
Expand Down Expand Up @@ -152,7 +161,38 @@ impl<'de> toml_span::Deserialize<'de> for Config {
} else {
(Vec::new(), Vec::new())
};
let severity_threshold = th.parse_opt("severity-threshold");
let st = |th: &mut TableHelper<'_>, fdeps: &mut Vec<Span>| {
let (k, mut v) = th.take("severity-threshold")?;

fdeps.push(k.span);
let s = match v.take_string(Some(
"https://docs.rs/rustsec/latest/rustsec/advisory/enum.Severity.html",
)) {
Ok(s) => s,
Err(err) => {
th.errors.push(err);
return None;
}
};

match s.parse() {
Ok(st) => Some(st),
Err(err) => {
th.errors.push(
(
toml_span::ErrorKind::Custom(
format!("failed to parse rustsec::Severity: {err}").into(),
),
v.span,
)
.into(),
);
None
}
}
};

let severity_threshold = st(&mut th, &mut fdeps);
let git_fetch_with_cli = th.optional("git-fetch-with-cli");
let disable_yank_checking = th.optional("disable-yank-checking").unwrap_or_default();
let maximum_db_staleness = if let Some((_, mut val)) = th.take("maximum-db-staleness") {
Expand Down Expand Up @@ -199,6 +239,7 @@ impl<'de> toml_span::Deserialize<'de> for Config {
git_fetch_with_cli,
disable_yank_checking,
maximum_db_staleness,
deprecated: fdeps,
})
}
}
Expand Down Expand Up @@ -226,6 +267,23 @@ impl crate::cfg::UnvalidatedConfig for Config {
}
}

use crate::diag::general::{Deprecated, DeprecationReason};

// Output any deprecations, we'll remove the fields at the same time we
// remove all the logic they drive
for dep in self.deprecated {
ctx.push(
Deprecated {
reason: DeprecationReason::WillBeRemoved(Some(
"https://github.com/EmbarkStudios/cargo-deny/pull/606",
)),
key: dep,
file_id: ctx.cfg_id,
}
.into(),
);
}

ValidConfig {
file_id: ctx.cfg_id,
db_path: self.db_path,
Expand Down Expand Up @@ -450,7 +508,13 @@ mod test {
#[test]
fn deserializes_advisories_cfg() {
let cd = ConfigData::<Advisories>::load("tests/cfg/advisories.toml");
let validated = cd.validate(|a| a.advisories);
let validated = cd.validate_with_diags(
|a| a.advisories,
|files, diags| {
let diags = write_diagnostics(files, diags.into_iter());
insta::assert_snapshot!(diags);
},
);

insta::assert_json_snapshot!(validated);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
---
source: src/advisories/cfg.rs
expression: validated
---
{
"file_id": 1,
"db_path": "~/.cargo/advisory-dbs",
"db_urls": [
"https://github.com/RustSec/advisory-db"
],
"ignore": [
"RUSTSEC-0000-0000"
],
"ignore_yanked": [
{
"spec": {
"name": "crate",
"version-req": "=0.1"
},
"reason": null,
"use-instead": null
},
{
"spec": {
"name": "yanked",
"version-req": null
},
"reason": "a new version has not been released",
"use-instead": null
}
],
"vulnerability": "deny",
"unmaintained": "warn",
"unsound": "warn",
"yanked": "warn",
"notice": "warn",
"severity_threshold": "medium",
"git_fetch_with_cli": false,
"disable_yank_checking": false,
"maximum_db_staleness": [
466560000,
0
]
}
Original file line number Diff line number Diff line change
@@ -1,44 +1,35 @@
---
source: src/advisories/cfg.rs
expression: validated
expression: diags
---
{
"file_id": 1,
"db_path": "~/.cargo/advisory-dbs",
"db_urls": [
"https://github.com/RustSec/advisory-db"
],
"ignore": [
"RUSTSEC-0000-0000"
],
"ignore_yanked": [
{
"spec": {
"name": "crate",
"version-req": "=0.1"
},
"reason": null,
"use-instead": null
},
{
"spec": {
"name": "yanked",
"version-req": null
},
"reason": "a new version has not been released",
"use-instead": null
}
],
"vulnerability": "deny",
"unmaintained": "warn",
"unsound": "warn",
"yanked": "warn",
"notice": "warn",
"severity_threshold": "medium",
"git_fetch_with_cli": false,
"disable_yank_checking": false,
"maximum_db_staleness": [
466560000,
0
]
}
warning[deprecated]: this key will be removed in a future update, see https://github.com/EmbarkStudios/cargo-deny/pull/606 for details
┌─ tests/cfg/advisories.toml:4:1
4vulnerability = "deny"
^^^^^^^^^^^^^

warning[deprecated]: this key will be removed in a future update, see https://github.com/EmbarkStudios/cargo-deny/pull/606 for details
┌─ tests/cfg/advisories.toml:5:1
5unmaintained = "warn"
^^^^^^^^^^^^

warning[deprecated]: this key will be removed in a future update, see https://github.com/EmbarkStudios/cargo-deny/pull/606 for details
┌─ tests/cfg/advisories.toml:6:1
6unsound = "warn"
^^^^^^^

warning[deprecated]: this key will be removed in a future update, see https://github.com/EmbarkStudios/cargo-deny/pull/606 for details
┌─ tests/cfg/advisories.toml:8:1
8notice = "warn"
^^^^^^

warning[deprecated]: this key will be removed in a future update, see https://github.com/EmbarkStudios/cargo-deny/pull/606 for details
┌─ tests/cfg/advisories.toml:14:1
14severity-threshold = "medium"
^^^^^^^^^^^^^^^^^^


24 changes: 24 additions & 0 deletions src/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,27 @@ impl<'de> toml_span::Deserialize<'de> for Reason {
Ok(Self(r))
}
}

/// Deserialize a field from the table if it exists, but append the key's span
/// so it can be marked as deprecated
pub fn deprecated<'de, T>(
th: &mut toml_span::de_helpers::TableHelper<'de>,
field: &'static str,
spans: &mut Vec<Span>,
) -> Option<T>
where
T: toml_span::Deserialize<'de>,
{
let Some((k, mut v)) = th.take(field) else {
return None;
};
spans.push(k.span);

match T::deserialize(&mut v) {
Ok(v) => Some(v),
Err(mut err) => {
th.errors.append(&mut err.errors);
None
}
}
}
26 changes: 21 additions & 5 deletions src/diag/general.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,39 @@ impl From<Code> for String {
}

pub enum DeprecationReason {
WillBeRemoved,
WillBeRemoved(Option<&'static str>),
Moved(&'static str),
Renamed(&'static str),
MovedAndRenamed {
table: &'static str,
key: &'static str,
},
Removed(&'static str),
}

impl fmt::Display for DeprecationReason {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::WillBeRemoved => f.write_str("the key will be removed in a future update"),
Self::Moved(tab) => write!(f, "the key has been moved to [{tab}]"),
Self::Renamed(nname) => write!(f, "the key has been renamed to '{nname}'"),
Self::WillBeRemoved(url) => {
if let Some(url) = url {
write!(
f,
"this key will be removed in a future update, see {url} for details"
)
} else {
f.write_str("this key will be removed in a future update")
}
}
Self::Moved(tab) => write!(f, "this key has been moved to [{tab}]"),
Self::Renamed(nname) => write!(f, "this key has been renamed to '{nname}'"),
Self::MovedAndRenamed { table, key } => {
write!(f, "the key been moved to [{table}] and renamed to '{key}'")
write!(f, "this key been moved to [{table}] and renamed to '{key}'")
}
Self::Removed(url) => {
write!(
f,
"this key has been removed, see {url} for migration information"
)
}
}
}
Expand Down
Loading

0 comments on commit 800c768

Please sign in to comment.