From 57ec0684e340aa5bb237b00ff1fe86ccb6a8104b Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sat, 15 Sep 2018 18:23:41 -0700 Subject: [PATCH 1/2] Fix missing messages when --message-format=json is deeply nested This commit demonstrates using miniserde for the JSON messages emitted by machine_message::emit. Miniserde does not rely on recursion so this approach supports messages with any arbitrarily large amount of nesting. Pulling in a dependency on two different serialization libraries is not ideal, especially as this one is generally more verbose and annoying as a consequence of no recursion. We will need to discuss whether this tradeoff makes sense or whether the bug can be resolved a different way. --- Cargo.toml | 1 + src/cargo/core/compiler/mod.rs | 4 +- src/cargo/core/manifest.rs | 98 ++++++++++++++++++++++++++++--- src/cargo/core/package_id.rs | 13 ++++ src/cargo/lib.rs | 2 + src/cargo/util/machine_message.rs | 57 ++++++++++++++---- 6 files changed, 155 insertions(+), 20 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1c77b43ded1..04280a88a97 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,7 @@ lazycell = "1.0" libc = "0.2" log = "0.4" libgit2-sys = "0.7.5" +miniserde = "0.1.6" num_cpus = "1.0" opener = "0.3.0" rustfix = "0.4.2" diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 10c362e2a16..48e10c7ffbe 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -5,8 +5,8 @@ use std::io::{self, Write}; use std::path::{self, Path, PathBuf}; use std::sync::Arc; +use miniserde; use same_file::is_same_file; -use serde_json; use core::manifest::TargetSourcePath; use core::profiles::{Lto, Profile}; @@ -1009,7 +1009,7 @@ fn json_stderr(line: &str, package_id: &PackageId, target: &Target) -> CargoResu // stderr from rustc/rustdoc can have a mix of JSON and non-JSON output if line.starts_with('{') { // Handle JSON lines - let compiler_message = serde_json::from_str(line) + let compiler_message = miniserde::json::from_str(line) .map_err(|_| internal(&format!("compiler produced invalid json: `{}`", line)))?; machine_message::emit(&machine_message::FromCompiler { diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 88ede658c3e..c24dd28e862 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -1,11 +1,14 @@ #![allow(deprecated)] // for SipHasher +use std::borrow::Cow; use std::collections::{BTreeMap, HashMap}; use std::fmt; use std::hash::{Hash, Hasher, SipHasher}; use std::path::{Path, PathBuf}; use std::rc::Rc; +use std::slice; +use miniserde; use semver::Version; use serde::ser; use toml; @@ -123,6 +126,12 @@ impl LibKind { } } +impl miniserde::Serialize for LibKind { + fn begin(&self) -> miniserde::ser::Fragment { + miniserde::ser::Fragment::Str(Cow::Borrowed(self.crate_type())) + } +} + impl fmt::Debug for LibKind { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.crate_type().fmt(f) @@ -169,6 +178,37 @@ impl ser::Serialize for TargetKind { } } +impl miniserde::Serialize for TargetKind { + fn begin(&self) -> miniserde::ser::Fragment { + use miniserde::ser::{Fragment, Seq, Serialize}; + use self::TargetKind::*; + + enum KindsStream<'a> { + Lib(slice::Iter<'a, LibKind>), + // double reference to enable cast to &dyn Serialize + Single(Option<&'static &'static str>), + } + + impl<'a> Seq for KindsStream<'a> { + fn next(&mut self) -> Option<&Serialize> { + Some(match self { + KindsStream::Lib(kinds) => kinds.next()?, + KindsStream::Single(single) => single.take()?, + }) + } + } + + Fragment::Seq(Box::new(match self { + Lib(kinds) => KindsStream::Lib(kinds.iter()), + Bin => KindsStream::Single(Some(&"bin")), + ExampleBin | ExampleLib(_) => KindsStream::Single(Some(&"example")), + Test => KindsStream::Single(Some(&"test")), + CustomBuild => KindsStream::Single(Some(&"custom-build")), + Bench => KindsStream::Single(Some(&"bench")), + })) + } +} + impl fmt::Debug for TargetKind { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { use self::TargetKind::*; @@ -261,25 +301,69 @@ struct SerializedTarget<'a> { /// See https://doc.rust-lang.org/reference/linkage.html crate_types: Vec<&'a str>, name: &'a str, - src_path: &'a PathBuf, - edition: &'a str, + src_path: &'a Path, + edition: String, #[serde(rename = "required-features", skip_serializing_if = "Option::is_none")] required_features: Option>, } -impl ser::Serialize for Target { - fn serialize(&self, s: S) -> Result { +impl Target { + fn to_serialized_target(&self) -> SerializedTarget { SerializedTarget { kind: &self.kind, crate_types: self.rustc_crate_types(), name: &self.name, - src_path: &self.src_path.path().to_path_buf(), - edition: &self.edition.to_string(), + src_path: self.src_path.path(), + edition: self.edition.to_string(), required_features: self .required_features .as_ref() .map(|rf| rf.iter().map(|s| &**s).collect()), - }.serialize(s) + } + } +} + +impl ser::Serialize for Target { + fn serialize(&self, s: S) -> Result { + self.to_serialized_target().serialize(s) + } +} + +impl miniserde::Serialize for Target { + fn begin(&self) -> miniserde::ser::Fragment { + use miniserde::ser::{Fragment, Map, Serialize}; + + struct TargetStream<'a> { + data: SerializedTarget<'a>, + src_path: Cow<'a, str>, + state: usize, + } + + impl<'a> Map for TargetStream<'a> { + fn next(&mut self) -> Option<(Cow, &Serialize)> { + let state = self.state; + self.state += 1; + match state { + 0 => Some((Cow::Borrowed("kind"), self.data.kind)), + 1 => Some((Cow::Borrowed("crate_types"), &self.data.crate_types)), + 2 => Some((Cow::Borrowed("name"), &self.data.name)), + 3 => Some((Cow::Borrowed("src_path"), &self.src_path)), + 4 => Some((Cow::Borrowed("edition"), &self.data.edition)), + 5 => Some(( + // skipped if None + Cow::Borrowed("required-features"), + self.data.required_features.as_ref()?, + )), + _ => None, + } + } + } + + Fragment::Map(Box::new(TargetStream { + data: self.to_serialized_target(), + src_path: self.src_path.path().to_string_lossy(), + state: 0, + })) } } diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index 7bb64e8a112..b255fe86899 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::cmp::Ordering; use std::fmt::{self, Formatter}; use std::hash::Hash; @@ -5,6 +6,7 @@ use std::hash; use std::path::Path; use std::sync::Arc; +use miniserde; use semver; use serde::de; use serde::ser; @@ -40,6 +42,17 @@ impl ser::Serialize for PackageId { } } +impl miniserde::Serialize for PackageId { + fn begin(&self) -> miniserde::ser::Fragment { + miniserde::ser::Fragment::Str(Cow::Owned(format!( + "{} {} ({})", + self.inner.name, + self.inner.version, + self.inner.source_id.to_url(), + ))) + } +} + impl<'de> de::Deserialize<'de> for PackageId { fn deserialize(d: D) -> Result where diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 6792ba12c38..3133e26ba18 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -41,6 +41,8 @@ extern crate libc; extern crate libgit2_sys; #[macro_use] extern crate log; +#[macro_use] +extern crate miniserde; extern crate num_cpus; extern crate opener; extern crate rustfix; diff --git a/src/cargo/util/machine_message.rs b/src/cargo/util/machine_message.rs index 3104d4b6060..2be08e7b5aa 100644 --- a/src/cargo/util/machine_message.rs +++ b/src/cargo/util/machine_message.rs @@ -1,23 +1,58 @@ -use serde::ser; -use serde_json::{self, Value}; +use std::borrow::Cow; + +use miniserde::json; +use miniserde::ser::{self, Fragment, Serialize}; use core::{PackageId, Target}; -pub trait Message: ser::Serialize { +pub trait Message: Serialize { fn reason(&self) -> &str; } pub fn emit(t: &T) { - let mut json: Value = serde_json::to_value(t).unwrap(); - json["reason"] = json!(t.reason()); - println!("{}", json); + struct Wrapper<'a> { + reason: &'a str, + message: &'a Message, + } + + impl<'a> Serialize for Wrapper<'a> { + fn begin(&self) -> Fragment { + Fragment::Map(Box::new(StreamMessage { + reason: Some(&self.reason), + value: match Serialize::begin(self.message) { + Fragment::Map(map) => map, + _ => panic!("machine_message::emit expected a JSON map"), + }, + })) + } + } + + struct StreamMessage<'a> { + // double reference to enable cast to &dyn Serialize + reason: Option<&'a &'a str>, + value: Box, + } + + impl<'a> ser::Map for StreamMessage<'a> { + fn next(&mut self) -> Option<(Cow, &Serialize)> { + match self.reason.take() { + Some(reason) => Some((Cow::Borrowed("reason"), reason)), + None => self.value.next(), + } + } + } + + println!("{}", json::to_string(&Wrapper { + reason: t.reason(), + message: t, + })); } -#[derive(Serialize)] +#[derive(MiniSerialize)] pub struct FromCompiler<'a> { pub package_id: &'a PackageId, pub target: &'a Target, - pub message: serde_json::Value, + pub message: json::Value, } impl<'a> Message for FromCompiler<'a> { @@ -26,7 +61,7 @@ impl<'a> Message for FromCompiler<'a> { } } -#[derive(Serialize)] +#[derive(MiniSerialize)] pub struct Artifact<'a> { pub package_id: &'a PackageId, pub target: &'a Target, @@ -45,7 +80,7 @@ impl<'a> Message for Artifact<'a> { /// This is different from the regular `Profile` to maintain backwards /// compatibility (in particular, `test` is no longer in `Profile`, but we /// still want it to be included here). -#[derive(Serialize)] +#[derive(MiniSerialize)] pub struct ArtifactProfile { pub opt_level: &'static str, pub debuginfo: Option, @@ -54,7 +89,7 @@ pub struct ArtifactProfile { pub test: bool, } -#[derive(Serialize)] +#[derive(MiniSerialize)] pub struct BuildScript<'a> { pub package_id: &'a PackageId, pub linked_libs: &'a [String], From 14a34d4cc583fc68cb511052e1a25ff403abb995 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sun, 16 Sep 2018 00:33:01 -0700 Subject: [PATCH 2/2] Test deep recursion fix --- tests/testsuite/check.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/testsuite/check.rs b/tests/testsuite/check.rs index 1fd1ef8b11d..3322986ad67 100644 --- a/tests/testsuite/check.rs +++ b/tests/testsuite/check.rs @@ -1,3 +1,5 @@ +use std::fmt::{self, Write}; + use glob::glob; use support::install::exe; use support::is_nightly; @@ -690,3 +692,24 @@ fn does_not_use_empty_rustc_wrapper() { let p = project().file("src/lib.rs", "").build(); p.cargo("check").env("RUSTC_WRAPPER", "").run(); } + +#[test] +fn error_from_deep_recursion() -> Result<(), fmt::Error> { + let mut big_macro = String::new(); + writeln!(big_macro, "macro_rules! m {{")?; + for i in 0..130 { + writeln!(big_macro, "({}) => {{ m!({}); }};", i, i + 1)?; + } + writeln!(big_macro, "}}")?; + writeln!(big_macro, "m!(0);")?; + + let p = project().file("src/lib.rs", &big_macro).build(); + p.cargo("check --message-format=json") + .with_status(101) + .with_stdout_contains( + "[..]\"message\":\"recursion limit reached while expanding the macro `m`\"[..]", + ) + .run(); + + Ok(()) +}