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

[WIP] Configure lints in Cargo.toml #5728

Closed
wants to merge 7 commits into from
Closed
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
13 changes: 13 additions & 0 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,19 @@ fn build_base_args<'a, 'cfg>(
cmd.args(args);
}

let unit_cfg = bcx.cfg(unit.kind);
let features = bcx.resolve.features(unit.pkg.package_id());
if let Some(ref lints) = unit.pkg.manifest().lints() {
for ref lint_section in lints.iter() {
lint_section.set_lint_flags(unit_cfg, features, cmd);
}
}
if let Some(ref virtual_lints) = bcx.ws.virtual_lints() {
for ref lint_section in virtual_lints.iter() {
lint_section.set_lint_flags(unit_cfg, features, cmd);
}
}

// -C overflow-checks is implied by the setting of -C debug-assertions,
// so we only need to provide -C overflow-checks if it differs from
// the value of -C debug-assertions we would provide.
Expand Down
88 changes: 88 additions & 0 deletions src/cargo/core/lints.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
use std::collections::BTreeMap;
use std::collections::HashSet;
use std::str::FromStr;

use util::{Cfg, CfgExpr, ProcessBuilder};
use util::errors::CargoResult;

#[derive(Clone, PartialEq, Debug)]
enum LintKind {
Allow,
Warn,
Deny,
Forbid,
}

impl LintKind {
pub fn try_from_string(lint_state: &str) -> Option<LintKind> {
match lint_state.as_ref() {
"allow" => Some(LintKind::Allow),
"warn" => Some(LintKind::Warn),
"deny" => Some(LintKind::Deny),
"forbid" => Some(LintKind::Forbid),
_ => None,
}
}

pub fn flag(&self) -> char {
match self {
LintKind::Allow => 'A',
LintKind::Warn => 'W',
LintKind::Deny => 'D',
LintKind::Forbid => 'F',
}
}
}

#[derive(Clone, Debug)]
pub struct Lints {
lints: Vec<(String, LintKind)>,
cfg: Option<CfgExpr>,
}

impl Lints {
pub fn new(
cfg: Option<&String>,
manifest_lints: &BTreeMap<String, String>,
warnings: &mut Vec<String>,
) -> CargoResult<Lints> {
let cfg = if let Some(t) = cfg {
if t.starts_with("cfg(") && t.ends_with(')') {
Some(CfgExpr::from_str(&t[4..t.len() - 1])?)
} else {
bail!("expected `cfg(...)`, found {}", t)
}
} else {
None
};

let mut lints = vec![];
for (lint_name, lint_state) in manifest_lints.iter() {
if let Some(state) = LintKind::try_from_string(lint_state) {
lints.push((lint_name.to_string(), state));
} else {
warnings.push(format!(
"invalid lint state for `{}` (expected `warn`, `allow`, `deny` or `forbid`)",
lint_name
));
}
}
Ok(Lints { lints, cfg })
}

pub fn set_lint_flags(&self, unit_cfg: &[Cfg], features: &HashSet<String>, cmd: &mut ProcessBuilder) {
match self.cfg {
None => self.set_flags(cmd),
Some(CfgExpr::Value(Cfg::KeyPair(ref key, ref value)))
Copy link
Member

Choose a reason for hiding this comment

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

I think we've already got a number of #[cfg] matching functions throughout Cargo, could those be reused here?

Copy link
Member Author

@detrumi detrumi Aug 20, 2018

Choose a reason for hiding this comment

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

I don't see one that could be used here, except maybe the CfgExpr::matches function that is used in the next line. The way I wrote it seems simpler than wrapping all feature strings inside Cfg::KeyPair, unless feature = foo keypairs can be nested inside other CfgExpr's?

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll want to funnel everything through matches to ensure it's consistently appplied

if key == "feature" && features.contains(value) => self.set_flags(cmd),
Some(ref cfg) if cfg.matches(unit_cfg) => self.set_flags(cmd),
_ => (),
}
}

fn set_flags(&self, cmd: &mut ProcessBuilder) {
for (lint_name, state) in self.lints.iter() {
cmd.arg(format!("-{}", state.flag())).arg(lint_name);
}
}
}
14 changes: 14 additions & 0 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use toml;
use url::Url;

use core::interning::InternedString;
use core::lints::Lints;
use core::profiles::Profiles;
use core::{Dependency, PackageId, PackageIdSpec, SourceId, Summary};
use core::{Edition, Feature, Features, WorkspaceConfig};
Expand Down Expand Up @@ -44,6 +45,7 @@ pub struct Manifest {
original: Rc<TomlManifest>,
features: Features,
edition: Edition,
lints: Option<Vec<Lints>>,
im_a_teapot: Option<bool>,
default_run: Option<String>,
metabuild: Option<Vec<String>>,
Expand All @@ -68,6 +70,7 @@ pub struct VirtualManifest {
workspace: WorkspaceConfig,
profiles: Profiles,
warnings: Warnings,
lints: Option<Vec<Lints>>,
}

/// General metadata about a package which is just blindly uploaded to the
Expand Down Expand Up @@ -361,6 +364,7 @@ impl Manifest {
workspace: WorkspaceConfig,
features: Features,
edition: Edition,
lints: Option<Vec<Lints>>,
im_a_teapot: Option<bool>,
default_run: Option<String>,
original: Rc<TomlManifest>,
Expand All @@ -383,6 +387,7 @@ impl Manifest {
features,
edition,
original,
lints,
im_a_teapot,
default_run,
publish_lockfile,
Expand Down Expand Up @@ -423,6 +428,9 @@ impl Manifest {
pub fn warnings(&self) -> &Warnings {
&self.warnings
}
pub fn lints(&self) -> &Option<Vec<Lints>> {
&self.lints
}
pub fn profiles(&self) -> &Profiles {
&self.profiles
}
Expand Down Expand Up @@ -531,13 +539,15 @@ impl VirtualManifest {
patch: HashMap<Url, Vec<Dependency>>,
workspace: WorkspaceConfig,
profiles: Profiles,
lints: Option<Vec<Lints>>,
) -> VirtualManifest {
VirtualManifest {
replace,
patch,
workspace,
profiles,
warnings: Warnings::new(),
lints,
}
}

Expand All @@ -553,6 +563,10 @@ impl VirtualManifest {
&self.workspace
}

pub fn lints(&self) -> &Option<Vec<Lints>> {
&self.lints
}

pub fn profiles(&self) -> &Profiles {
&self.profiles
}
Expand Down
1 change: 1 addition & 0 deletions src/cargo/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub mod compiler;
pub mod dependency;
mod features;
mod interning;
pub mod lints;
pub mod manifest;
pub mod package;
pub mod package_id;
Expand Down
11 changes: 11 additions & 0 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::slice;
use glob::glob;
use url::Url;

use core::lints::Lints;
use core::profiles::Profiles;
use core::registry::PackageRegistry;
use core::{Dependency, PackageIdSpec};
Expand Down Expand Up @@ -245,6 +246,16 @@ impl<'cfg> Workspace<'cfg> {
}
}

pub fn virtual_lints(&self) -> &Option<Vec<Lints>> {
let root = self.root_manifest
.as_ref()
.unwrap_or(&self.current_manifest);
match *self.packages.get(root) {
MaybePackage::Virtual(ref vm) => vm.lints(),
_ => &None,
}
}

/// Returns the root path of this workspace.
///
/// That is, this returns the path of the directory containing the
Expand Down
32 changes: 31 additions & 1 deletion src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use url::Url;

use core::dependency::{Kind, Platform};
use core::manifest::{LibKind, ManifestMetadata, TargetSourcePath, Warnings};
use core::lints::Lints;
use core::profiles::Profiles;
use core::{Dependency, Manifest, PackageId, Summary, Target};
use core::{Edition, EitherManifest, Feature, Features, VirtualManifest};
Expand Down Expand Up @@ -240,6 +241,9 @@ pub struct TomlManifest {
patch: Option<BTreeMap<String, BTreeMap<String, TomlDependency>>>,
workspace: Option<TomlWorkspace>,
badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
lints: Option<BTreeMap<String, String>>,
#[serde(rename = "lints2")]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely clear on what the lints2 name is doing here, can you add a comment to the naming choice here?

Copy link
Member Author

@detrumi detrumi Aug 20, 2018

Choose a reason for hiding this comment

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

Ah, I haven't figured out yet how to deserialize both [lints] and [lints.'cfg(...)'] correctly, so I just temporarily named it something else (sorry!). Just using #[serde(rename = "lints")] here doesn't work, so it probably needs a custom Deserialize impl.

Copy link
Member

Choose a reason for hiding this comment

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

Hm ok, I'm not really sure we'd want to support cfg(..) for lint configurations though in the sense that it feels like it's a bit overkill for configuration that can otherwise be specific in the crate source anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, let's simplify to just a [lints] section then.

feature_lints: Option<BTreeMap<String, BTreeMap<String, String>>>,
}

#[derive(Deserialize, Serialize, Clone, Debug, Default)]
Expand Down Expand Up @@ -748,6 +752,8 @@ impl TomlManifest {
workspace: None,
badges: self.badges.clone(),
cargo_features: self.cargo_features.clone(),
lints: self.lints.clone(),
feature_lints: self.feature_lints.clone(),
});

fn map_deps(
Expand Down Expand Up @@ -997,6 +1003,7 @@ impl TomlManifest {
),
};
let profiles = Profiles::new(me.profile.as_ref(), config, &features, &mut warnings)?;
let lints = lints(me.lints.as_ref(), me.feature_lints.as_ref(), &mut warnings)?;
let publish = match project.publish {
Some(VecStringOrBool::VecString(ref vecstring)) => {
features
Expand Down Expand Up @@ -1035,6 +1042,7 @@ impl TomlManifest {
workspace_config,
features,
edition,
lints,
project.im_a_teapot,
project.default_run.clone(),
Rc::clone(me),
Expand Down Expand Up @@ -1109,6 +1117,7 @@ impl TomlManifest {
(me.replace(&mut cx)?, me.patch(&mut cx)?)
};
let profiles = Profiles::new(me.profile.as_ref(), config, &features, &mut warnings)?;
let lints = lints(me.lints.as_ref(), me.feature_lints.as_ref(), &mut warnings)?;
let workspace_config = match me.workspace {
Some(ref config) => WorkspaceConfig::Root(WorkspaceRootConfig::new(
&root,
Expand All @@ -1121,7 +1130,7 @@ impl TomlManifest {
}
};
Ok((
VirtualManifest::new(replace, patch, workspace_config, profiles),
VirtualManifest::new(replace, patch, workspace_config, profiles, lints),
nested_paths,
))
}
Expand Down Expand Up @@ -1489,3 +1498,24 @@ impl fmt::Debug for PathValue {
self.0.fmt(f)
}
}

fn lints(
toml_lints: Option<&BTreeMap<String, String>>,
toml_feature_lints: Option<&BTreeMap<String, BTreeMap<String, String>>>,
warnings: &mut Vec<String>
) -> CargoResult<Option<Vec<Lints>>> {
let mut lints = vec![];
if let Some(toml_lints) = toml_lints {
lints.push(Lints::new(None, toml_lints, warnings)?);
}
if let Some(toml_feature_lints) = toml_feature_lints {
for (ref cfg, ref feature_lints) in toml_feature_lints.iter() {
lints.push(Lints::new(Some(cfg), feature_lints, warnings)?);
}
}
Ok(if !lints.is_empty() {
Some(lints)
} else {
None
})
}
Loading