Resolve Face eagerly when constructing StyledString

[topolarity]

This is step 1 to eliminating our type-piracy struggles

StyledStrings needs to put some type that it owns into its AnnotatedStrings so that we have a right to hook into the display logic (and the display logic can know which copy of StyledStrings to delegate to).

It is also a semantic change to how constructing StyledStrings behaves, but overall I think it's a lot more intuitive to have a StyledString actually compute its style information at construction rather than defer it to be looked up at display time (let know if you agree @tecosaur)

Resolves #87

Made on top of #95, since that will probably be landed first.

[topolarity]

The tests will need adjusting for this - I haven't done that yet sorry

They currently expect that most Faces are unresolved in the AnnotatedStrings we produce

[topolarity]

I could use some help filling this logic in here

if key might be :face and we want that to display properly, we need to force resolution here too

[tecosaur]

Great to see this work take shape! I'm yet to go through all of your recent flurry of PRs, but from this and a few others, I've developed two linked concerns that I think are worth discussing.

Both concerns stem from the more eager use of getface. The obvious (and seemingly intended) consequence of this is that details are resolved when the StyledString is constructed. There are two implications of this I am least wary of.

Baking in different values, unexpectedly

Firstly, if a string is constructed at compile time/before customisations are loaded, a later use of the same face can look different. Here's an example to illustrate:

# in some package Foo

@define_faces!(:my_foo => Face(foreground=:red, underline=:yellow)) # Assuming future work

const reused_string_or_similar = styled"... {my_foo:stuff} ..."

function something_a(io::IO)
    ...
    println(io, reused_string_or_similar)
    ...
end

function something_b(io::IO)
    ...
    println(io, styled"... {my_foo:other stuff} ...")
    ...
end

In this scenario, should my_foo be customised then something_a differ from something_b in output style, in what I'd consider a surprising way.

Inability to customise downstream

One of the features of the current design is to remap faces on the fly. There are particular use-cases I see this enabling. For instance, say JuliaSyntaxHighlighting is used to bake in static highlighting (an idea that's of some interest to the Documenter crowd). To do this, and produce output for all the various themes you currently could do something like:

const DOCUMENTER_THEME_X = Dict(:julia_number => Face(...), ...)
const DOCUMENTER_THEME_Y = Dict(:julia_number => Face(...), ...)

# ...

function html_highlight_julia(code::String, themes::Vector{Dict{Symbol, Face}})
    hl_code = highlight(code)
    map(themes) do theme
        withfaces(theme) do
            sprint(show, MIME("text/html"), hl_code)
        end
    end
end

Here the code is highlighted once, and then styled with multiple different themes. From what I can tell, this pattern wouldn't be possible with this PR. In this particular example, you could accept the overhead to re-parse the code and apply the faces for each theme before display, but are also other cases where you might want to pass some content across functions/packages this would be a more major impediment.

Closing thoughts

I see how this gets us closer to avoiding type piracy by avoiding a Base-owned type as the value, making it possible to call the annotation owner's specialised printing function.

However, I'm not quite onboard with the specific tradeoffs made here, and wonder if there's a better balance that could be struck. Considering this together with our discussions on effectively implementing custom face namespaces, I wonder if a FaceRef type or similar could be introduced to allow us to avoid the problems with Symbol, without the consequences I've outlined above.

[topolarity]

Firstly, if a string is constructed at compile time/before customisations are loaded, a later use of the same face can look different. Here's an example to illustrate:

I think @define_faces! should update the FACES dictionary even for top-level code.

For me, it's just as surprising that this doesn't work:

@define_faces!(:my_foo => Face(foreground=:red, underline=:yellow)) # Assuming future work
const face = getface(:my_foo) # my_foo... wasn't defined yet?
const str = make_annotated("Test string", face)

It's fine to have @define_faces! update the FACES dictionary at pre-compile time - it just also needs to add an __init__ hook since otherwise the entry will be lost at runtime

In this particular example, you could accept the overhead to re-parse the code and apply the faces for each theme before display, but are also other cases where you might want to pass some content across functions/packages this would be a more major impediment.

I think that use case can be handled just by a LazyStyledString equivalent of some kind? I think the lazy approach is OK, but it should be specifically opt-in and a FaceRef would be a useful type to implement it internally.

There are other design problems to make the fully-lazy approach behave correctly though..

The merge(...) we have right now partially resolves inheritance as part of its implementation, but that's not the right behavior since it means you stop seeing future updates to inherited Faces and freeze their definition (but only sometimes). To make these fully lazy, you need to defer the resolution of inheritance as well, which adds a lot of additional complexity / inefficiency (or surprising merges)

The eager solution lets you perform the merge up-front once and optimize on that concrete style info - that is a good thing IMO

[topolarity]

In this particular example, you could accept the overhead to re-parse the code and apply the faces for each theme before display, but are also other cases where you might want to pass some content across functions/packages this would be a more major impediment.

For that use case, you can use an AnnotatedString (which doesn't know how to display itself yet), and provide a styled(::AnnotatedString) method that applies a set of styling / resolves Faces before you display it?

That way, you still have the same functionality effectively, but it's obvious to the user that something important is happening when styled(...) is called (it is resolving Face definitions and baking them into the string)

That seems better to me than resolving implicitly at every display (which is also a lot of repeated, unnecessary computation)

[tecosaur]

I think @define_faces! should update the FACES dictionary even for top-level code.

Indeed, but that doesn't prevent the behaviour I describe.

For me, it's just as surprising that this doesn't work:

I'm not quite sure what you're referring to here, if we presume @define_faces! acts like a better version of the addface! of today, getface(:my_foo) works.

I think that use case can be handled just by a LazyStyledString equivalent of some kind? I think the lazy approach is OK, but it should be specifically opt-in and a FaceRef would be a useful type to implement it internally.

We could branch out into lazy and non-lazy versions, but if we can reasonably get away with just one that seems preferable to me.

The merge(...) we have right now partially resolves inheritance as part of its implementation, but that's not the right behavior since it means you stop seeing future updates to inherited Faces and freeze their definition (but only sometimes). To make these fully lazy, you need to defer the resolution of inheritance as well, which adds a lot of additional complexity / inefficiency.

I don't quite see the respect in which inheritance is non-lazy currently, could you elaborate?

The eager solution lets you perform the merge up-front once and optimize on that concrete style info - that is a good thing IMO.

I'm tempted to see how far we can get by having a single lazy form, but offering a freeze/bake/etc. function that fully resolves all faces.

[topolarity]

I don't quite see the respect in which inheritance is non-lazy currently, could you elaborate?

Yeah I'm referring to this code:

StyledStrings.jl/src/faces.jl

Lines 527 to 528 in 729f56c

	b_inheritance = map(fname -> get(Face, FACES.current[], fname), Iterators.reverse(b.inherit))
	b_resolved = merge(foldl(merge, b_inheritance), b_noinherit)

I'm tempted to see how far we can get by having a single lazy form, but offering a freeze/bake/etc. function that fully resolves all faces.

Yeah - We're on the same page about the two "types" of strings at least (and the reify operation)

I just have a very hard time that it's called a "StyledString" and it's not... actually styled yet - it's just marked-up / tagged

We can have those lazy semantics, but if you want the string to know how to resolve its style automatically, we need to add a styler field (and/or type parameter) to AnnotatedString so that it has something to ask to resolve its tags. That's required by the Base / StyledStrings split unfortunately.

edit: Actually I think you're right that FaceRef would solve that better (and leaves room for a Palette reference, if we want to go that direction) - if we can fix the merge, that might be the way to go.