Report an error instead of panicking on invalid map types#1031
Open
danieleades wants to merge 1 commit into
Open
Report an error instead of panicking on invalid map types#1031danieleades wants to merge 1 commit into
danieleades wants to merge 1 commit into
Conversation
Add a fallible std::str::FromStr implementation for MapType and use it where invalid input is reachable: - MapType's Deserialize implementation now maps parse failures to a deserialization error rather than panicking, and deserializes an owned String so inputs requiring allocation (e.g. strings with escape sequences) work. - cargo-typify parses --map-type fallibly, so an invalid value produces a clean CLI error instead of a panic. MapType::new and the From<&str>/From<String> conversions keep their panicking behavior for compatibility; their panics are now documented. MapType is additionally re-exported from the typify crate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
MapType::newparses its input withsyn::parse_str(..).expect("valid ident"), and both theDeserializeimpl and theFrom<&str>/From<String>conversions funnel through it. As a result:cargo typify --map-type 'not a valid!!type'panics withexpectinstead of reporting an error.MapTypefrom an invalid string panics instead of returning a deserialization error. The impl also used<&str>::deserialize, which fails for inputs that need allocation (e.g. strings containing escape sequences).Fix
impl FromStr for MapType, carrying thesynparse error message asErr.Deserializeto deserialize an ownedStringand map parse failures toserde::de::Error::custom.--map-typefallibly incargo-typify::convert, so an invalid value produces a clean CLI error.MapTypeis re-exported from thetypifycrate to make that possible (with_map_typealready exposed it in its bound).FromStrrejects garbage and accepts::std::collections::BTreeMap;Deserializehandles escaped strings and errors on invalid input; a CLI integration test confirms an invalid--map-typeyields an error, not a panic.Trade-offs
The
FromStrimpl is additive.MapType::newand theFrom<&str>/From<String>conversions keep their panicking behavior for backwards compatibility, but now document it with# Panicssections and point atstr::parseas the non-panicking alternative.