Skip to content

Commit

Permalink
Improve TOML decoding error messages
Browse files Browse the repository at this point in the history 
Unfortunately while `#[serde(untagged)]` is precisely what we want in terms of
semantics it leaves a little to be desired in terms of error messages. This
commit updates to remove the usage of that attribute in favor of implementing
`Deserialize` directly, which is quite simple in these few cases.

Closes #3790
  • Loading branch information
@alexcrichton
alexcrichton committed Mar 8, 2017
1 parent a226b3c commit cb53e1b
Show file tree
Hide file tree
Showing 2 changed files with 194 additions and 42 deletions.
169 changes: 127 additions & 42 deletions src/cargo/util/toml.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,42 @@ type TomlExampleTarget = TomlTarget;
type TomlTestTarget = TomlTarget;
type TomlBenchTarget = TomlTarget;

#[derive(Deserialize)]
#[serde(untagged)]
pub enum TomlDependency {
Simple(String),
Detailed(DetailedTomlDependency)
}

impl de::Deserialize for TomlDependency {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where D: de::Deserializer
{
struct TomlDependencyVisitor;

impl de::Visitor for TomlDependencyVisitor {
type Value = TomlDependency;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a version string like \"0.9.8\" or a \
detailed dependency like { version = \"0.9.8\" }")
}

fn visit_str<E>(self, s: &str) -> Result<Self::Value, E>
where E: de::Error
{
Ok(TomlDependency::Simple(s.to_owned()))
}

fn visit_map<V>(self, map: V) -> Result<Self::Value, V::Error>
where V: de::MapVisitor
{
let mvd = de::value::MapVisitorDeserializer::new(map);
DetailedTomlDependency::deserialize(mvd).map(TomlDependency::Detailed)
}
}

deserializer.deserialize(TomlDependencyVisitor)
}
}

#[derive(Deserialize, Clone, Default)]
pub struct DetailedTomlDependency {
Expand Down Expand Up @@ -288,13 +317,48 @@ impl de::Deserialize for TomlOptLevel {
}
}

#[derive(Deserialize, Clone)]
#[serde(untagged)]
#[derive(Clone)]
pub enum U32OrBool {
U32(u32),
Bool(bool),
}

impl de::Deserialize for U32OrBool {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where D: de::Deserializer
{
struct Visitor;

impl de::Visitor for Visitor {
type Value = U32OrBool;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a boolean or an integer")
}

fn visit_i64<E>(self, u: i64) -> Result<Self::Value, E>
where E: de::Error,
{
Ok(U32OrBool::U32(u as u32))
}

fn visit_u64<E>(self, u: u64) -> Result<Self::Value, E>
where E: de::Error,
{
Ok(U32OrBool::U32(u as u32))
}

fn visit_bool<E>(self, b: bool) -> Result<Self::Value, E>
where E: de::Error,
{
Ok(U32OrBool::Bool(b))
}
}

deserializer.deserialize(Visitor)
}
}

#[derive(Deserialize, Clone, Default)]
pub struct TomlProfile {
#[serde(rename = "opt-level")]
Expand All @@ -309,13 +373,42 @@ pub struct TomlProfile {
panic: Option<String>,
}

#[derive(Deserialize, Clone, Debug)]
#[serde(untagged)]
#[derive(Clone, Debug)]
pub enum StringOrBool {
String(String),
Bool(bool),
}

impl de::Deserialize for StringOrBool {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where D: de::Deserializer
{
struct Visitor;

impl de::Visitor for Visitor {
type Value = StringOrBool;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a boolean or a string")
}

fn visit_str<E>(self, s: &str) -> Result<Self::Value, E>
where E: de::Error,
{
Ok(StringOrBool::String(s.to_string()))
}

fn visit_bool<E>(self, b: bool) -> Result<Self::Value, E>
where E: de::Error,
{
Ok(StringOrBool::Bool(b))
}
}

deserializer.deserialize(Visitor)
}
}

#[derive(Deserialize)]
pub struct TomlProject {
name: String,
Expand Down Expand Up @@ -405,7 +498,7 @@ fn inferred_lib_target(name: &str, layout: &Layout) -> Option<TomlTarget> {
layout.lib.as_ref().map(|lib| {
TomlTarget {
name: Some(name.to_string()),
path: Some(PathValue::Path(lib.clone())),
path: Some(PathValue(lib.clone())),
.. TomlTarget::new()
}
})
Expand All @@ -423,7 +516,7 @@ fn inferred_bin_targets(name: &str, layout: &Layout) -> Vec<TomlTarget> {
name.map(|name| {
TomlTarget {
name: Some(name),
path: Some(PathValue::Path(bin.clone())),
path: Some(PathValue(bin.clone())),
.. TomlTarget::new()
}
})
Expand All @@ -435,7 +528,7 @@ fn inferred_example_targets(layout: &Layout) -> Vec<TomlTarget> {
ex.file_stem().and_then(|s| s.to_str()).map(|name| {
TomlTarget {
name: Some(name.to_string()),
path: Some(PathValue::Path(ex.clone())),
path: Some(PathValue(ex.clone())),
.. TomlTarget::new()
}
})
Expand All @@ -447,7 +540,7 @@ fn inferred_test_targets(layout: &Layout) -> Vec<TomlTarget> {
ex.file_stem().and_then(|s| s.to_str()).map(|name| {
TomlTarget {
name: Some(name.to_string()),
path: Some(PathValue::Path(ex.clone())),
path: Some(PathValue(ex.clone())),
.. TomlTarget::new()
}
})
Expand All @@ -459,7 +552,7 @@ fn inferred_bench_targets(layout: &Layout) -> Vec<TomlTarget> {
ex.file_stem().and_then(|s| s.to_str()).map(|name| {
TomlTarget {
name: Some(name.to_string()),
path: Some(PathValue::Path(ex.clone())),
path: Some(PathValue(ex.clone())),
.. TomlTarget::new()
}
})
Expand Down Expand Up @@ -498,7 +591,7 @@ impl TomlManifest {
TomlTarget {
name: lib.name.clone().or(Some(project.name.clone())),
path: lib.path.clone().or_else(
|| layout.lib.as_ref().map(|p| PathValue::Path(p.clone()))
|| layout.lib.as_ref().map(|p| PathValue(p.clone()))
),
..lib.clone()
}
Expand Down Expand Up @@ -995,11 +1088,15 @@ struct TomlTarget {
required_features: Option<Vec<String>>,
}

#[derive(Deserialize, Clone)]
#[serde(untagged)]
enum PathValue {
String(String),
Path(PathBuf),
#[derive(Clone)]
struct PathValue(PathBuf);

impl de::Deserialize for PathValue {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where D: de::Deserializer
{
Ok(PathValue(String::deserialize(deserializer)?.into()))
}
}

/// Corresponds to a `target` entry, but `TomlTarget` is already used.
Expand Down Expand Up @@ -1118,21 +1215,9 @@ impl TomlTarget {
}
}

impl PathValue {
fn to_path(&self) -> PathBuf {
match *self {
PathValue::String(ref s) => PathBuf::from(s),
PathValue::Path(ref p) => p.clone(),
}
}
}

impl fmt::Debug for PathValue {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match *self {
PathValue::String(ref s) => s.fmt(f),
PathValue::Path(ref p) => p.display().fmt(f),
}
self.0.fmt(f)
}
}

Expand All @@ -1159,7 +1244,7 @@ fn normalize(package_root: &Path,

let lib_target = |dst: &mut Vec<Target>, l: &TomlLibTarget| {
let path = l.path.clone().unwrap_or_else(
|| PathValue::Path(Path::new("src").join(&format!("{}.rs", l.name())))
|| PathValue(Path::new("src").join(&format!("{}.rs", l.name())))
);
let crate_types = l.crate_type.as_ref().or(l.crate_type2.as_ref());
let crate_types = match crate_types {
Expand All @@ -1172,7 +1257,7 @@ fn normalize(package_root: &Path,
};

let mut target = Target::lib_target(&l.name(), crate_types,
package_root.join(path.to_path()));
package_root.join(&path.0));
configure(l, &mut target);
dst.push(target);
};
Expand All @@ -1181,14 +1266,14 @@ fn normalize(package_root: &Path,
default: &mut FnMut(&TomlBinTarget) -> PathBuf| {
for bin in bins.iter() {
let path = bin.path.clone().unwrap_or_else(|| {
let default_bin_path = PathValue::Path(default(bin));
if package_root.join(default_bin_path.to_path()).exists() {
let default_bin_path = PathValue(default(bin));
if package_root.join(&default_bin_path.0).exists() {
default_bin_path // inferred from bin's name
} else {
PathValue::Path(Path::new("src").join("main.rs"))
PathValue(Path::new("src").join("main.rs"))
}
});
let mut target = Target::bin_target(&bin.name(), package_root.join(path.to_path()),
let mut target = Target::bin_target(&bin.name(), package_root.join(&path.0),
bin.required_features.clone());
configure(bin, &mut target);
dst.push(target);
Expand All @@ -1207,7 +1292,7 @@ fn normalize(package_root: &Path,
default: &mut FnMut(&TomlExampleTarget) -> PathBuf| {
for ex in examples.iter() {
let path = ex.path.clone().unwrap_or_else(|| {
PathValue::Path(default(ex))
PathValue(default(ex))
});

let crate_types = ex.crate_type.as_ref().or(ex.crate_type2.as_ref());
Expand All @@ -1219,7 +1304,7 @@ fn normalize(package_root: &Path,
let mut target = Target::example_target(
&ex.name(),
crate_types,
package_root.join(path.to_path()),
package_root.join(&path.0),
ex.required_features.clone()
);
configure(ex, &mut target);
Expand All @@ -1232,10 +1317,10 @@ fn normalize(package_root: &Path,
default: &mut FnMut(&TomlTestTarget) -> PathBuf| {
for test in tests.iter() {
let path = test.path.clone().unwrap_or_else(|| {
PathValue::Path(default(test))
PathValue(default(test))
});

let mut target = Target::test_target(&test.name(), package_root.join(path.to_path()),
let mut target = Target::test_target(&test.name(), package_root.join(&path.0),
test.required_features.clone());
configure(test, &mut target);
dst.push(target);
Expand All @@ -1247,10 +1332,10 @@ fn normalize(package_root: &Path,
default: &mut FnMut(&TomlBenchTarget) -> PathBuf| {
for bench in benches.iter() {
let path = bench.path.clone().unwrap_or_else(|| {
PathValue::Path(default(bench))
PathValue(default(bench))
});

let mut target = Target::bench_target(&bench.name(), package_root.join(path.to_path()),
let mut target = Target::bench_target(&bench.name(), package_root.join(&path.0),
bench.required_features.clone());
configure(bench, &mut target);
dst.push(target);
Expand Down
67 changes: 67 additions & 0 deletions tests/bad-config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -947,3 +947,70 @@ fn bad_source_config7() {
error: more than one source URL specified for `source.foo`
"));
}

#[test]
fn bad_dependency() {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.0.0"
authors = []
[dependencies]
bar = 3
"#)
.file("src/lib.rs", "");

assert_that(p.cargo_process("build"),
execs().with_status(101).with_stderr("\
error: failed to parse manifest at `[..]`
Caused by:
invalid type: integer `3`, expected a version string like [..]
"));
}

#[test]
fn bad_debuginfo() {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.0.0"
authors = []
[profile.dev]
debug = 'a'
"#)
.file("src/lib.rs", "");

assert_that(p.cargo_process("build"),
execs().with_status(101).with_stderr("\
error: failed to parse manifest at `[..]`
Caused by:
invalid type: string \"a\", expected a boolean or an integer for [..]
"));
}

#[test]
fn bad_opt_level() {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.0.0"
authors = []
build = 3
"#)
.file("src/lib.rs", "");

assert_that(p.cargo_process("build"),
execs().with_status(101).with_stderr("\
error: failed to parse manifest at `[..]`
Caused by:
invalid type: integer `3`, expected a boolean or a string for key [..]
"));
}

0 comments on commit cb53e1b

Please sign in to comment.