Add ID to LocatorPayload::Transparent

[wkordalski]

This way we can pass information about transparent locator payloads being the same or different,
and thanks to it, Hayagriva can generate correct ibid vs ibid-with-locator states.

Related issue: #280

This PR adds a field to publicly available enum variant, and thus introduces a "major" breaking change.

[wkordalski]

@PgBiel Can I have your review?

[PgBiel]

After discussing with the team, we understood that these changes are directly tied to the Typst side of the fix, so we think you can keep this PR open, but in the meantime open a PR in the Typst side pointing to your hayagriva fork (with this PR's changes) showing that this PR can indeed be used to fix the original issue, e.g. by passing a content hash as the id and then showing in a test that it produces the expected behavior for ibid. Do you think you could do that? :)

[PgBiel]

If the intention is to use a hash here, we should use the same type used by Typst for hashes.

Suggested change

	Transparent(u64),
	Transparent(u128),

[PgBiel]

On second thought, let's try this approach so our hands aren't tied (since we already have a lifetime parameter anyway):

Suggested change

	Transparent(u64),
	Transparent(&'a dyn Eq),

[wkordalski]

Unfortunately, Eq is not dyn compatible (think about comparing &dyn Eq generated from two different types).

We can introduce generic parameter: LocatorPayload<'a, T: Eq = ()>, but I don't think that introducing more generic parameters is the way to go.

We can use u128 if you want, but I think that we will never need more than u64.

[PgBiel]

Ah, I forgot about that, thanks for catching it.

I agree in general we will never need more than usize, since we can't store more than usize::MAX citations, so that should be enough for an 1-to-1 map. With that said, we may need to store a map hash <-> id on the heap then, which is a bit annoying, but there probably won't be that many supplements in a document anyway.

So I think we can keep u64 for now. We should progress the Typst side before merging this PR.

[Drodt]

I would consider a generic parameter with default ‘u64‘ that must implement Copy. Then we know it is reasonably cheap.

[wkordalski]

I've made locator payload type generic (11e1095).

I don't like adding the transparent locator payload type across the whole hayagriva codebase and would rather stick to u64. But if you prefer the generic version, you can merge it.

We can additionally add the default type for the generic parameter, but it is not necessary. Typst doesn't need any change to handle the generic parameter — inference does a great job.

[wkordalski]

I've created draft PR to Typst: typst/typst#6171

It turned out that two text-only contents with the same text (but in different locations) generate different hashes (thus hashing supplement content is not a good idea).

The PR uses IDs generated from .plain_text() of supplements. We can also use hash of plain text, if you like.
This is not perfect solution, but user can use #text(size: 0pt)[identifier] trick to mark supplements as different if needed (this workaround is not possible on current main branch version).

I've also created a repository with a "test": https://github.com/wkordalski/typst-hayagriva-ibid
It contains all source files and output generated by Typst from main branch and from the draft PR I've created.
In the next few days I will try to figure out how to write tests in Typst way and rewrite the "test repository" to actual Typst test.

Update: We probably would like to merge #301 first.

[wkordalski]

Current status:

• Typst part is ready. It only waits (therefore draft status of PR) for merging this (Hayagriva) PR — and then — for next release of Hayagriva, so that we can remove [patch.crates-io] section that overrides Hayagriva version.
• We can discuss whether using plain_text() for supplement discrimination is the right choice. I think it is much better than current version and the one that introduces the least complexity.
• On Hayagriva side, we only need to decide whether we want generics or not.
  • Yes → we can add default parameters and merge the PR.
  • No → we can remove last three commits and merge the PR.

[PgBiel]

Thank you. The Typst side is currently blocked until further discussion with the main maintainer, who is currently busy with other tasks, so we will have to wait a bit. But I hope to give you further feedback soon.

[wkordalski]

Hi, I see some labels were attached to the Typst-side PR a week ago.

Has some decisions about future of the PRs been done? Can we move forward to finally fix the ibid problem or we are still waiting for the main maintainer decision?

[PgBiel]

Hello, we're currently cleaning the Typst PR backlog. Hope to give you a response soon.

[laurmaedje]

Unfortunately, Eq is not dyn compatible (think about comparing &dyn Eq generated from two different types).

We can introduce generic parameter: LocatorPayload<'a, T: Eq = ()>, but I don't think that introducing more generic parameters is the way to go.

I would suggest creating a custom type that wraps a PartialEq type and encapsulates the trait object. By using Arc, we can also keep it cheap to clone without needing a Clone impl on the underlying type. It would look like this:

Implementation

use std::any::Any;
use std::fmt::{self, Debug};
use std::sync::Arc;

#[derive(Clone)]
pub struct TransparentLocator(Arc<dyn Bounds>);

impl TransparentLocator {
    pub fn new<T>(value: T) -> Self
    where
        T: PartialEq + 'static,
    {
        Self(Arc::new(value))
    }
}

impl Debug for TransparentLocator {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.pad("TransparentLocator(..)")
    }
}

impl PartialEq for TransparentLocator {
    fn eq(&self, other: &Self) -> bool {
        self.0.dyn_eq(&*other.0)
    }
}

trait Bounds: 'static {
    fn as_any(&self) -> &dyn Any;
    fn dyn_eq(&self, other: &dyn Bounds) -> bool;
}

impl<T> Bounds for T
where
    T: PartialEq + 'static,
{
    fn as_any(&self) -> &dyn Any {
        self
    }

    fn dyn_eq(&self, other: &dyn Bounds) -> bool {
        let Some(other) = other.as_any().downcast_ref::<Self>() else {
            return false;
        };
        self == other
    }
}

fn main() {
    let a = TransparentLocator::new("hello");
    let b = TransparentLocator::new("hello");
    let c = TransparentLocator::new("world");
    let d = TransparentLocator::new(5);
    assert_eq!(a, b);
    assert_ne!(a, c);
    assert_ne!(a, d);
}

[wkordalski]

I would suggest creating a custom type that wraps a PartialEq type and encapsulates the trait object. By using Arc, we can also keep it cheap to clone without needing a Clone impl on the underlying type.

That's also a solution.

Would you like to have TransparentLocator: Eq or not?
LocatorPayload and some other types containing LocatorPayload are deriving Eq impl.

We can remove the #[derive(Eq)] as they are not used in Hayagriva (and then we also can remove #[derive(Hash)] as it usually makes sense only when type is also Eq).

Or we can wrap types that are Eq and make the whole TransparentLocator implement Eq.

What is the policy of Typst on adding small dependencies? There's dyn_eq crate that does exactly what suggested implementation does.

[laurmaedje]

I would not require Eq, in particular, as Typst's Content does not implement it, but also because it's just not necessary.

What is the policy of Typst on adding small dependencies? There's dyn_eq crate that does exactly what suggested implementation does.

I think there's no need to pull a crate for this. The code is already written and simple to maintain.