Skip to content

Commit 2a8bb6e

Browse files
authored
Rollup merge of #144218 - Noratrieb:target-spec-json-de-jank, r=fee1-dead
Use serde for target spec json deserialize The previous manual parsing of `serde_json::Value` was a lot of complicated code and extremely error-prone. It was full of janky behavior like sometimes ignoring type errors, sometimes erroring for type errors, sometimes warning for type errors, and sometimes just ICEing for type errors (the icing on the top). Additionally, many of the error messages about allowed values were out of date because they were in a completely different place than the FromStr impls. Overall, the system caused confusion for users. I also found the old deserialization code annoying to read. Whenever a `key!` invocation was found, one had to first look for the right macro arm, and no go to definition could help. This PR replaces all this manual parsing with a 2-step process involving serde. First, the string is parsed into a `TargetSpecJson` struct. This struct is a 1:1 representation of the spec JSON. It already parses all the enums and is very simple to read and write. Then, the fields from this struct are copied into the actual `Target`. The reason for this two-step process instead of just serializing into a `Target` is because of a few reasons 1. There are a few transformations performed between the two formats 2. The default logic is implemented this way. Otherwise all the default field values would have to be spelled out again, which is suboptimal. With this logic, they fall out naturally, because everything in the json struct is an `Option`. Overall, the mapping is pretty simple, with the vast majority of fields just doing a 1:1 mapping that is captured by two macros. I have deliberately avoided making the macros generic to keep them simple. All the `FromStr` impls now have the error message right inside them, which increases the chance of it being up to date. Some "`from_str`" impls were turned into proper `FromStr` impls to support this. The new code is much less involved, delegating all the JSON parsing logic to serde, without any manual type matching. This change introduces a few breaking changes for consumers. While it is possible to use this format on stable, it is very much subject to change, so breaking changes are expected. The hope is also that because of the way stricter behavior, breaking changes are easier to deal with, as they come with clearer error messages. 1. Invalid types now always error, everywhere. Previously, they would sometimes error, and sometimes just be ignored (which meant the users JSON was still broken, just silently!) 2. This now makes use of `deny_unknown_fields` instead of just warning on unused fields, which was done previously. Serde doesn't make it easy to get such warning behavior, which was the primary reason that this now changed. But I think error behavior is very reasonable too. If someone has random stale fields in their JSON, it is likely because these fields did something at some point but no longer do, and the user likely wants to be informed of this so they can figure out what to do. This is also relevant for the future. If we remove a field but someone has it set, it probably makes sense for them to take a look whether they need this and should look for alternatives, or whether they can just delete it. Overall, the JSON is made more explicit. This is the only expected breakage, but there could also be small breakage from small mistakes. All targets roundtrip though, so it can't be anything too major. fixes #144153
2 parents 40482a2 + dad96b1 commit 2a8bb6e

File tree

21 files changed

+678
-849
lines changed

21 files changed

+678
-849
lines changed

Cargo.lock

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4563,7 +4563,10 @@ dependencies = [
45634563
"rustc_macros",
45644564
"rustc_serialize",
45654565
"rustc_span",
4566+
"serde",
4567+
"serde_derive",
45664568
"serde_json",
4569+
"serde_path_to_error",
45674570
"tracing",
45684571
]
45694572

@@ -4955,6 +4958,16 @@ dependencies = [
49554958
"serde",
49564959
]
49574960

4961+
[[package]]
4962+
name = "serde_path_to_error"
4963+
version = "0.1.17"
4964+
source = "registry+https://github.com/rust-lang/crates.io-index"
4965+
checksum = "59fab13f937fa393d08645bf3a84bdfe86e296747b506ada67bb15f10f218b2a"
4966+
dependencies = [
4967+
"itoa",
4968+
"serde",
4969+
]
4970+
49584971
[[package]]
49594972
name = "serde_spanned"
49604973
version = "0.6.9"

compiler/rustc_session/src/config.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -343,12 +343,12 @@ impl LinkSelfContained {
343343
if let Some(component_to_enable) = component.strip_prefix('+') {
344344
self.explicitly_set = None;
345345
self.enabled_components
346-
.insert(LinkSelfContainedComponents::from_str(component_to_enable)?);
346+
.insert(LinkSelfContainedComponents::from_str(component_to_enable).ok()?);
347347
Some(())
348348
} else if let Some(component_to_disable) = component.strip_prefix('-') {
349349
self.explicitly_set = None;
350350
self.disabled_components
351-
.insert(LinkSelfContainedComponents::from_str(component_to_disable)?);
351+
.insert(LinkSelfContainedComponents::from_str(component_to_disable).ok()?);
352352
Some(())
353353
} else {
354354
None

compiler/rustc_session/src/options.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1296,7 +1296,7 @@ pub mod parse {
12961296
}
12971297

12981298
pub(crate) fn parse_linker_flavor(slot: &mut Option<LinkerFlavorCli>, v: Option<&str>) -> bool {
1299-
match v.and_then(LinkerFlavorCli::from_str) {
1299+
match v.and_then(|v| LinkerFlavorCli::from_str(v).ok()) {
13001300
Some(lf) => *slot = Some(lf),
13011301
_ => return false,
13021302
}

compiler/rustc_target/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ rustc_fs_util = { path = "../rustc_fs_util" }
1212
rustc_macros = { path = "../rustc_macros" }
1313
rustc_serialize = { path = "../rustc_serialize" }
1414
rustc_span = { path = "../rustc_span" }
15+
serde = "1.0.219"
16+
serde_derive = "1.0.219"
1517
serde_json = "1.0.59"
18+
serde_path_to_error = "0.1.17"
1619
tracing = "0.1"
1720
# tidy-alphabetical-end
1821

compiler/rustc_target/src/json.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,3 +114,18 @@ impl ToJson for rustc_abi::CanonAbi {
114114
self.to_string().to_json()
115115
}
116116
}
117+
118+
macro_rules! serde_deserialize_from_str {
119+
($ty:ty) => {
120+
impl<'de> serde::Deserialize<'de> for $ty {
121+
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
122+
where
123+
D: serde::Deserializer<'de>,
124+
{
125+
let s = String::deserialize(deserializer)?;
126+
FromStr::from_str(&s).map_err(serde::de::Error::custom)
127+
}
128+
}
129+
};
130+
}
131+
pub(crate) use serde_deserialize_from_str;

compiler/rustc_target/src/spec/json.rs

Lines changed: 373 additions & 652 deletions
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)