Codify and Resolve modifiers

[T0mstone]

Closes #30.

The most important idea of this PR is that it codifies what a "modifier" is.
In that aspect, it stands on its own and makes sense even without considering the context of usage in typst.

For this, there is a new ModifierSet type that contains all the logic for comparing and composing modifiers.
This new type is designed to be generic, so that typst can reuse it with EcoString.
(In particular, typst's symbol::Repr::Modified would change to Modified(Arc<(List, ModifierSet<EcoString>)>))
Some private functions from typst-library/src/foundations/symbol.rs are lifted to public methods on ModifierSet:

• parts becomes ModifierSet::iter
• contained becomes ModifierSet::contains
• find becomes ModifierSet::best_match_in (with reversed argument order), which is generic since it was really easy to make it so and this prepares it for when we let symbols have more than one char per variant (cf. Emoji Variation selectors #25)

I've also taken the liberty of consolidating the types from lib.rs and build.rs. It bugged me that they were duplicated and this change didn't really do anything in terms of code size while arguably improving maintainability (unless you don't like macros, but even among macros this one is very tame).

There are some new functions (get, variants and modifiers) on codex::Symbol that somewhat mirror the ones on typst's Symbol, but aren't intended to replace them right now (for why, see the next paragraph). They just made sense to add anyway.
The only changes made to typst would be to start using ModifierSet, which would basically have the effect of having moved the relevant code from typst to codex.

Actually integrating typst's symbol::Repr::Modified into codex proved too complicated, so I left that for later (if we even need it). Further considerations regarding the integrating of this into typst is left to a future PR at typst/typst (only if this one is accepted, of course).

[MDLC01]

Like with #27, we can wait until Typst 0.13.0 is released to open the discussion and decide whether this should be part of Codex.

[MDLC01]

Related: #51.

[MDLC01]

I left a lot of comments and suggestions. Of course, feel free to disagree.

I really like the ModifierSet abstraction. If feels very clean. I'm not a fan of the include! trick and the macro, but it's probably fine.

Regarding the ModifierSet::iter and ModifierSet::<&'a str>::to_iter methods, I feel like this could be achieved more idiomatically with two separate impl<'a, S: Deref<Target = str>> IntoIterator for &'a ModifierSet<S> and impl<'a> IntoIterator for ModifierSet<&'a str>.

[T0mstone]

Thanks for the review!

I'm not a fan of the include! trick

The same effect could also be achieved by writing

#[path = ...]
mod shared;
use shared::*;

but that's three lines instead of one, which was the deciding factor.
I can also change it to that if you want, though.

[MDLC01]

I feel like this would be more idiomatic, but it doesn't matter that much.

Also, it may be useful for later to have something like impl<S: for<'a> std::ops::AddAssign<&'a str>> FromIterator for ModifierSet<S>. But if there is any issue implementing that, it does not matter for now.

[T0mstone]

Also, it may be useful for later to have something like impl<S: for<'a> std::ops::AddAssign<&'a str>> FromIterator for ModifierSet<S>.

For that, there's the issue of an empty iterator. Should it just require S: Default and assume that the default is the empty string?
Apart from that question, the implementation is pretty easy.

Edit: While the implementation might be easy, the API is totally full of caveats, since individual iterator items could contain the character .. So the more idiomatic thing would be FromIterator<ModifierSet<_>> for ModifierSet<_>, but for that to be a valid union, it would require the items to not intersect...

I think I'll leave this out for now, it requires too much additional design work.

[MDLC01]

Thanks!

[MDLC01]

@laurmaedje you may want to review this PR

[MDLC01]

@laurmaedje I'm taking the liberty to ping you again as you are more active in PR reviews now.

[laurmaedje]

Yes, I'll try to get to this in the next few days. (If I still haven't done it in a week or so, please feel free to ping me again and then I'll do it right away.)

[laurmaedje]

I've also taken the liberty of consolidating the types from lib.rs and build.rs. It bugged me that they were duplicated and this change didn't really do anything in terms of code size while arguably improving maintainability (unless you don't like macros, but even among macros this one is very tame).

I'm not a fan of macros, so I'm not sure I like this change, but in any case, I think it would be best to discuss this separately from this PR. The fact that I was unsure whether I liked that is part of why it took me so long to respond here. If I see things correctly, the ModifierSet is also not needed in the build script, it's just there to make the type a bit more semantic, right?

[T0mstone]

If I see things correctly, the ModifierSet is also not needed in the build script, it's just there to make the type a bit more semantic, right?

Not yet, but I'd need it for #27...

I think it would be best to discuss this separately from this PR

I'll see what I can do, but I'm not happy about this.

[T0mstone]

I've removed the macro, but kept ModifierSet shared since a ModifierSet type is needed in build.rs for debug-printing anyway.

Having done this change directly, I'm now actually unsure myself whether the macro was worth it, in a "both sides are bad" kind of way, so I probably won't make another PR for that, at least for now.

[T0mstone]

Also, minor stylistic question, but should shared.rs be called modifier_set.rs now that it only contains one thing?

[laurmaedje]

I think keeping it as shared.rs is fine and more future proof. Assuming all the Typst functionality can be implemented with this, the API looks fine to me!

[laurmaedje]

I think this is outdated since it uses new_unchecked

[T0mstone]

No, the point where it gets output is

codex/build.rs

Line 217 in 34edb09

Symbol::Multi(list) => write!(buf, "Multi(&{list:?})").unwrap(),

which uses the debug-formatting, which outputs ModifierSet(<string>).
Changing that would of course be possible, but it'd require constructing the slice manually instead of just using {:?} there.

[T0mstone]

Assuming all the Typst functionality can be implemented with this, the API looks fine to me!

Cool, I'll get working on a PR to typst then.

[T0mstone]

Before we merge: While writing the typst PR, I just ran into the issue of ModifierSet needing to implement Eq, PartialEq, Hash. I didn't derive those originally because ModifierSet is conceptually a set, so order shouldn't matter.

What should I do about those?

Edit: For now, I opted to just derive them too for the sake of efficiency and to add a note to the documentation. This has to be fine, since the equality-checks and hashes resulting from this are in principle identical to the string-based ones originally used in typst.

[T0mstone]

I also ended up needing a way to access the underlying string to call repr() on it for two errors. Is this new as_str method okay? An alternative would have been splitting and re-joining, which might have been fine in this case (since error messages aren't typically performance-sensitive). But as_str doesn't really hurt either, so idk.

Scratch that, I found a better way.

[T0mstone]

typst/docs needs a way to Display a ModifierSet, so I think the most idiomatic way of achieving that is just bringing as_str back.

[T0mstone]

Now I'm getting a panic for some reason. Please wait with merging this until I've dealt with all issues in typst/typst#6159.

[T0mstone]

Okay, bug fixed. This issue has brought up the need for some unit tests for ModifierSet. I'll write some more of those now, but feel free to also suggest some test cases.

[T0mstone]

Should be ready to merge now.

(And sorry for the inbox spam)