Emit warnings for unused fields in custom targets. #85775
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
adamrk
force-pushed
the
warn-unused-target-fields
branch
from
419130d to
0476cc7
Compare
adamrk
changed the title
adamrk
changed the title
adamrk
force-pushed
the
warn-unused-target-fields
branch
from
0476cc7 to
2890879
Compare
|
Just a outside question, would it be feasible to switch to serde for target specs? It would make changes like this much easier, and I think remove a substantial amount of code. |
From what I saw for this change it seems like it could be done, but I didn't look to carefully at the parsing of specific fields. Happy to work on it if people think it makes sense. |
|
@matthewjasper should I be doing something to get this moving along? Sorry, this is my first time send a patch to rustc, so not exactly sure on the process. And I don't mean to rush you or anything - just want to make sure that this isn't waiting on me. |
|
r? @nagisa |
compiler/rustc_serialize/src/json.rs
Outdated
| @@ -1204,6 +1204,15 @@ impl Json { | |||
| } | |||
| } | |||
|
|
|||
| /// If the Json value is an Object, deletes the value associated with the | |||
| /// provided key from the Object and returns it. Otherwise, returns None. | |||
| pub fn remove(&mut self, key: &str) -> Option<Json> { | |||
There was a problem hiding this comment.
I think it is worthwhile to find a name this method that implies the method operates on maps (remove_key? pop_key? etc.)
There was a problem hiding this comment.
Makes sense. Went with remove_key since remove is what we call on the underlying map.
| } | ||
|
|
||
| pub fn warning_message(&self) -> String { | ||
| format!("Some fields from target json were unused: {:?}", self.unused_fields) |
There was a problem hiding this comment.
Worth spending an effort to avoid using debug repr of vector here and making it more prose like. (e.g. one of the
this, that and another fields in the target.json were not used
the target.json file contains unknown fields: this, that, another
or similar.)
Additionally, the warning and error messages should not generally begin with a capital letter because they are already preceded by a error: or warning: .
There was a problem hiding this comment.
Done! Although I'm not sure if we should use target.json since that won't actually be the name of the file?
| .map(|a| a.as_string().unwrap().to_string()) | ||
| .collect(); | ||
| if let Some(j) = obj.remove(&name){ | ||
| if let Some(v) = Json::as_array(&j) { |
There was a problem hiding this comment.
I think this really wants to be removed (i.e. not warned against) if the value was definitely used. Right now keys that we expect to be arrays, but weren't used because they were not arrays are not warned against.
There was a problem hiding this comment.
Sounds good. I ended up splitting this into a separate warnings for unrecognized fields and recognized ones with the wrong json type.
There was a problem hiding this comment.
If you don't mind doing this, it would probably be better to adapt these to be a proper error instead – there's no reason for us to let this through, I feel. We already error for most of the other keys, even in cases where type is correct but the value isn't (e.g. MergeFunctions/RelocModel).
There was a problem hiding this comment.
Hm, actually I misread the code, we still proceed executing if the type of the value was wrong (but fail if it was right, but with a wrong value). This entire thing probably wants a refactor 😅
There was a problem hiding this comment.
Yeah definitely agree that most of these should be hard errors instead. I made everything warnings in this case just to avoid breaking changes.
What would the process be for changing them to errors? Would it need to be part of the 2021 edition?
There was a problem hiding this comment.
Custom target definitions are an “unstable” feature, so introducing breaking changes here is okay without a process as extensive as an edition. Perhaps MCP at most, though I would say that even that is probably excessive in a situation where we're simply no longer accepting what was invalid in the first place.
adamrk
force-pushed
the
warn-unused-target-fields
branch
from
2890879 to
196303c
Compare
|
Thanks for taking a look @nagisa! |
adamrk
force-pushed
the
warn-unused-target-fields
branch
2 times, most recently
from
a8cb2e0 to
05eb88d
Compare
adamrk
force-pushed
the
warn-unused-target-fields
branch
from
05eb88d to
88b01f1
Compare
|
@bors r+ |
|
📌 Commit 88b01f1 has been approved by |
bors
added
S-waiting-on-bors
|
Thanks @nagisa! It looks like this'll be blocked until a maintainer approves the CI pipeline? |
|
No, bors will test the PR regardless once the PR gets to the top of its queue. |
|
☀️ Test successful - checks-actions |
Add a warning which lists any fields in a custom target
jsonfile that aren't used. Currently unrecognized fields are ignored so, for example, a typo in thejsonwill silently produce a target which isn't the one intended.