Add path parameter to biome_deserialize::json::deserialize_from_json_str

[NicholasLYang]

Hey, we're using biome_deserialize to process our turbo.json files and preserve span info. However, we also need to keep the path along with the span, since there can be multiple turbo.json files. I noticed that there is a name field that is passed through the deserializer and therefore any visitor. Currently it's set to an empty string. Could we add a parameter to deserialize_from_json_str to allow it to be customized? Or another function with that field set?

Happy to submit a PR for this.

Thanks,
Nicholas

[ematipico]

Hey @NicholasLYang , looking at the code, I think it’s a bug. Feel free to submit a PR,

[Conaclos]

Hi @NicholasLYang,

We could definitively add a name parameter to deserialize_from_json_str. This looks like a miss of opportunity.

Note that name has been added to name the deserialised value. This allows us to improve our diagnostics if an error occurs. All of our built-in deserializers pass name through deserializers and visitors, except for collections, in particular Vec, which sets the name of its deserialised items to the empty string. You could work around the problem by using the new-type idiom and implementing a visitor that passes the name to the Vvec's items. Alternatively, we could refactor the codebase and opt for a generic type instead of passing a string.

EDIT: could you give more context about your use-case?

[NicholasLYang]

Thanks for the replies @ematipico and @Conaclos! Basically we want to record span info, but we also need the file name since we don't want to confuse spans from different files. I want to pass that file name in and have it be added to the span info. Currently I'm deserializing and then doing a second pass to add the file name, which is obviously not ideal

[ematipico]

The name inside the deserializer isn't the file name, but the name of the field that the visitor is deserializing, that's why sometimes it is set to an empty string. For cases like maps or arrays, we don't have a named field

It seems you are talking about the diagnostics emitted during deserialisation. We could add a new function called deserialize_from_json_str_and_path, so you can pass the path of the file.

Although, I still don't understand what you mean by "second pass to add the file name". Do you mean that you need to create new diagnostics that contain the file name?

[NicholasLYang]

Sorry, the second pass part is not really relevant. I'm just saying that currently I create a data structure with the file path field set to None and then after I deserialize, I go through the data structure and manually set that field to the correct path.

The name inside the deserializer isn't the file name, but the name of the field that the visitor is deserializing, that's why sometimes it is set to an empty string. For cases like maps or arrays, we don't have a named field

Gotcha. If name is not meant to be the path, then it makes sense that it'd be empty. Perhaps it should be an Option then? Really, what I would love to have is some way of passing context through the deserializer, something like:

 pub trait Deserializable: Sized {
     /// Returns the deserialized form of `value`, or `None` if it failed.
     /// Any diagnostics emitted during deserialization are appended to `diagnostics`.
     /// `name` corresponds to the name used in a diagnostic to designate the value.
     fn deserialize(
+        ctx: DeserializeContext,
         value: &impl DeserializableValue,
         name: &str,
         diagnostics: &mut Vec<DeserializationDiagnostic>,
     ) -> Option<Self>;
 }

So that if I implement Deserialize, I can add a path value:

impl<T: Deserializable> Deserializable for Spanned<T> {
    fn deserialize(
        ctx: DeserializeContext,
        value: &impl DeserializableValue,
        name: &str,
        diagnostics: &mut Vec<DeserializationDiagnostic>,
    ) -> Option<Self> {
        let range = value.range();
        let value = T::deserialize(value, name, diagnostics)?;
        Some(Spanned {
            value,
            range: Some(range.into()),
            path: ctx.get("path"),
            text: None,
        })
    }
}

[Conaclos]

Taking a second though on your idea: We could indeed introduce a trait DeserializeContext. We could also move the diagnostics in the context in order to avoid the multiplication of parameters. This could also allow changing how the diagnostics are stored (for example using a SmallVec instead of Vec, ...).

impl<T: Deserializable> Deserializable for Spanned<T> {
    fn deserialize(
+       ctx: &mut impl DeserializeContext,
        value: &impl DeserializableValue,
        name: &str,
-       diagnostics: &mut Vec<DeserializationDiagnostic>,
    ) -> Option<Self> {
        // ...
        ctx.report(...);
    }
}

[Conaclos]

I added a name parameter to deserialize_from_json_str in the last released version.

[Conaclos]

@NicholasLYang
Do you still need a way of passing path to all deserializers?
I worked on a breaking refactor in #4166.
The path could be retrieved using ctx.id().