Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce WithDeserialized trait, which does not work #11

Closed
wants to merge 2 commits into from

Conversation

fasterthanlime
Copy link
Contributor

If anyone could explain:

  • Why I can't get this trait to work, no matter what I fiddle with
  • What changes would be required to the JsonDeserialize trait to make it work

I would be ever so thankful.

Note that I've already tried to get rid of one of the two lifetime parameters on JsonDeserialize, or pass one or both of them onto the function itself, and to use GATs with an Output (which breaks ToRustValue), etc. — none of that works.

The main reason why I have two lifetime parameters on JsonDeserialize is because JsonValue<'s> is invariant over 's.

@fasterthanlime
Copy link
Contributor Author

Specifically the error is:

CleanShot 2024-07-31 at 16 58 10@2x

From this bit of code:

CleanShot 2024-07-31 at 16 58 29@2x

Which you would use like this:

CleanShot 2024-07-31 at 16 58 45@2x

(sorry no alt text, it's all in the diff though!)

@fasterthanlime
Copy link
Contributor Author

Note that this pattern works in isolation, outside of the WithDeserialized trait. The recommended pattern is:

let input = String::from(r#"{"name": "John Doe", "age": 30}"#);
let value = merde_json::from_str(&input)?;
let my_struct = MyStruct::json_deserialize(Some(&value));
println!("{:?}", my_struct);

...where every binding borrows from the previous one.

The difficulty here is to express the extension trait itself.

@horazont
Copy link

diff --git a/src/lib.rs b/src/lib.rs
index c8687c8..c6e0448 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1552,24 +1552,22 @@ impl std::fmt::Debug for Fantome<'_, '_> {
 
 /// Allows deserializing and interacting with a borrowed JSON value all
 /// in one go.
-pub trait WithDeserialized<'src, 'val, T>
+pub trait WithDeserialized<'src, T>
 where
     Self: 'src,
-    T: JsonDeserialize<'src, 'val> + 'val,
-    'src: 'val,
+    for<'x> T: JsonDeserialize<'src, 'x> + 'x
 {
     /// Parses as JSON, deserializes as a Rust type `T`, then
     /// calls `f` with the deserialized value.
-    fn with_deserialized(&'val self, f: impl FnOnce(&T)) -> Result<(), MerdeJsonError>;
+    fn with_deserialized(&self, f: impl FnOnce(&T)) -> Result<(), MerdeJsonError>;
 }
 
-impl<'src, 'val, T> WithDeserialized<'src, 'val, T> for &'src str
+impl<'src, T> WithDeserialized<'src, T> for &'src str
 where
     Self: 'src,
-    T: JsonDeserialize<'src, 'val> + 'val,
-    'src: 'val,
+    for<'x> T: JsonDeserialize<'src, 'x> + 'x,
 {
-    fn with_deserialized(&'val self, f: impl FnOnce(&T)) -> Result<(), MerdeJsonError> {
+    fn with_deserialized(&self, f: impl FnOnce(&T)) -> Result<(), MerdeJsonError> {
         let json_value = from_str(self)?;
         let deserialized: T = json_value.to_rust_value()?;
         let _ = f(&deserialized);

@horazont
Copy link

The trait constraint was wrong, because what you need is that JsonDeserialize<'_, 'x> is implemented for any value of 'x. You cannot put a more specific constraint, because the lifetime you really need the implementation for is the lifetime of a temporary value inside with_deserialized.

As you cannot name that lifetime, the only solution (that I know of) is to use a HRTB which says "I need this to work for any lifetime, no matter how it's related to your other type parameters."

@horazont
Copy link

Note that this only passes cargo build --- still trying to figure out how to make cargo test pass after fixing the obvious things…

@fogti
Copy link

fogti commented Jul 31, 2024

That won't work (see that the tests fail), because it needs to work for any lifetime, which is restricted above by 'src and an unnamable one (the one of local variable json_value), and I see no way of making that work with current Rust.

@fasterthanlime
Copy link
Contributor Author

That won't work (see that the tests fail), because it needs to work for any lifetime, which is restricted above by 'src and an unnamable one (the one of local variable json_value), and I see no way of making that work with current Rust.

Alright cool, that's my assessment too — was just curious if I missed something obvious.

@horazont
Copy link

Riiight… Trying to fix the tests I got rustc to ICE, I think I'm done for the day.

That won't work (see that the tests fail), because it needs to work for any lifetime, which is restricted above by 'src and an unnamable one (the one of local variable json_value), and I see no way of making that work with current Rust.

That was my gut feeling, too, and even though the build passes, the failing tests make me think you're right about it.

@pacak
Copy link

pacak commented Jul 31, 2024

Riiight… Trying to fix the tests I got rustc to ICE, I think I'm done for the day.

Report it!

@fogti
Copy link

fogti commented Jul 31, 2024

In order to properly get this to work, one would need to use GATs in JsonDeserialize, I suppose. Unfortunately, it would completely mess with type-inference, too...

which means one would have to introduce another trait to guide translation of some type to some "non-bounded object", similar to the mess of the Try /Residual API.

@fasterthanlime
Copy link
Contributor Author

In order to properly get this to work, one would need to use GATs in JsonDeserialize, I suppose. Unfortunately, it would completely mess with type-inference, too...

Right, I did try that earlier!

But then ToRustValue completely blows up (and so do the impl_deserialize macros): because then if you do this:

let x: u64 = some_json_value.to_rust_value()?;

You get this:

image

Which is to say: as long as impl T for JsonSerialize returns T, rustc is happy enough to do type inference. But if you return a nother <T as JsonSerialize>::Output type (that happens to be "T with different lifetimes"), Rust is unhappy.

Come to think of it, maybe that's the easier approach? I wonder if rustc devs would consider that a bug.

@fogti
Copy link

fogti commented Jul 31, 2024

yeah, it would probably make it required in practical usage to then use some newly introduced WithDeserialized trait in order to not have to specify dozens of generics along the way...

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Aug 1, 2024

Here’s an idea:

https://x.com/bushrat011899/status/1818770596727341313?s=46

Instead of using + 'val as the lifetime bound, have you tried using a + Captures<'val> style of bound instead? There's a subtle difference in the lifetime resolution process that I'm not good enough to explain, but this trick solved a similar problem for me.

I had a hard time finding code when I was trying this myself, but it's based on this feature:

https://rust-lang.github.io/rfcs/3498-lifetime-capture-rules-2024.html

You basically define a trait Captures which says any type implementing it has the same lifetime as T (e.g., &'a ()). This "fixes" the + 'a syntax (outlives)

@fasterthanlime fasterthanlime reopened this Aug 1, 2024
@fasterthanlime
Copy link
Contributor Author

Closing now, this can be revisited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants