Always delegate returning errors to the Visitor from Content[Ref]Deserializer #2811
Conversation
Mingun
changed the title
Mingun
force-pushed
the
visitor-driven
branch
from
0f4c50d
to
4976c64
Compare
|
Pinging is not changing our priorities in any useful way We know which PRs are open and if our free time allows we make some progress on it. |
|
Nothing personal, I just think, that if you don't have time to work on the project on which the whole Rust ecosystem is based, its time to request help for maintaining. Sad to say this, but looks like I will be forced to request unmaintained status for serde at RustSec. The current situation persist a long time and it seems nothing will be changed without active community support. |
|
Please fork it instead. That is much more sustainable in the long run |
|
Unfortunately, I cannot use my fork for all my direct and indirect dependencies, so this is only short-term solution. |
Cargo lets you do exactly this, see here. (and for non-cargo build systems it should also be possible) |
|
That is what I name "short-term solution" |
|
Why do you need to replace the derives of your deps? Are the issues you have not contained to a single derive expansion but affected by the way fields' types implement the serde traits? |
|
Not all fixes in |
|
They are effectively on the level of getting a change into libstd at this point. I have several times brought up things like a perf (build and run) suite and a crater-at-home extension that would help us have confidence in changes that are complex enough to have hidden breaking changes that are hard to find in reviews |
|
If you are too afraid to make changes, it means that it is time to release a new version, which will not be automatically updated in most cases (i.e. release version 2). The release of the new version has no disadvantages: you do not break clients of version 1, you can make any breaking changes (however, you are not required to do this). Since version 1 is effectively frozen, there is no problem to maintain two versions (which is a frequent excuse for not releasing a new version). You don't need to maintain version 1 (because you don't do it anyway), you only need to maintain version 2. So you only maintain one version anyway, but with the release of version 2 you can actually make changes without worrying too much about the consequences. |
|
Releasing a new version is just as easy as forking under a different name. If you believe that to be the way forward, there is nothing stopping you. There is no difference between a major version bump and a separate crate name as far as cargo is concerned. |
|
There has difference: when new version of existing crate is released, consumers can support both versions just by using correct semver string. Even if you do some breaking changes in some parts, but consumers does not depend on these parts, they can allow both versions (i. e. actually for them the changes are compatible). With the new crate you are always incompatible, without variants. In case of serde it is hightly likely that the first variant will be totally dominated. |
Mingun
force-pushed
the
visitor-driven
branch
from
4976c64
to
b157b5a
Compare
…dd comments about coverage
…internally_tagged_variant deserialize_untagged_variant in that place is called when deserialzie_with attribute is set. In that case it performs special actions that is better to use explicitly in deserialize_untagged_variant for readability
failures(1):
newtype_unit
…lize enums Helper methods visit_content_[seq|map] does the same as [Seq|Map]Deserializer::deserialize_any and used everywhere except here. Reuse them for consistency
…sibility of the Visitor
Examples of errors produced during deserialization of internally tagged enums in tests
if instead of a Seq/Map a Str("unexpected string") will be provided:
In tests/test_annotations.rs
flatten::enum_::internally_tagged::tuple:
before: `invalid type: string "unexpected string", expected tuple variant`
after : `invalid type: string "unexpected string", expected tuple variant Enum::Tuple`
flatten::enum_::internally_tagged::struct_from_map:
before: `invalid type: string "unexpected string", expected struct variant`
after : `invalid type: string "unexpected string", expected struct variant Enum::Struct`
Mingun
force-pushed
the
visitor-driven
branch
from
b157b5a
to
9009f63
Compare
The
Deserializerconception is to provide data from the format to the type,the type should decide if it can accept data or return an error.Deserializercan do internal conversions based on the hints (== which method was called), but it shouldn't return errors. Hints are only recommendations, so if it does not known how to convert data, it should pass to theVisitorthe most natural representation of its internal value.This PR removes almost all attempts to return error from
ContentDeserializerandContentRefDeserializer. Thanks to the refactoring done in #2805 it is clearly visible that we always can delegate todeserialize_anywhen no special handling is the content is required.Errors from
VariantAccesswere replaced by a call toVisitor::visit_unitbecause the error said that theUnitvariant was unexpected. Attempt to deserialize accept unit variant when type does not expect it, will result to the same error, but if unit will be expected, the type will be able to deserialized (derivedDeserializeimplementations never does that, so that is possible only if implementDeserializemanually).Also, this PR unifies behavior between unit structs and units, now both of them can be deserialized from the empty sequence. Previously only the unit struct could be deserialized, while the explanation for special handling for both types exactly the same. Related issue: #2340