Rework annotation ordering/optimisations

[tecosaur]

There are some annoyances/discrepancies with annotation handling that I originally tried to fix in #53794, #53800 and #53801.

@LilithHafner was kind enough to have a look at these, and after a long chat we came to the conclusion that this isn't the right approach to take, and that the order with which annotations are applied should be given primacy over annotation ranges.

This PR supersedes the aforementioned PRs, and should make the way annotations are ordered more sensible.

See the commit messages for more details.

[tecosaur]

Ah, tests are failing because I missed a use of Base.annotatedstring_optimize! in StyledStrings' tests. I'll fix that and re-bump the stdlib shortly.

[LilithHafner]

What part of annotations are semantically meaningful/visible?

IIUC the annotations

[(1:1, :a => 1), (2:2, :a => 1)] and [(1:2, :a => 1)] are semantically equivalent
[(1:1, :a => 1), (2:2, :a => 1)] and [ (2:2, :a => 1), (1:1, :a => 1)] are semantically equivalent
[(1:1, :a => 1), (1:1, :a => 2)] and [ (1:1, :a => 2), (1:1, :a => 1)] are semantically different
[(1:1, :a => 1), (1:1, :b => 2)] and [ (1:1, :a => 2), (1:1, :b => 1)] are semantically different

This should be documented. Specifically, a simple specification that describes what is and is not semantically visible from which one can derive the above (or a different result).

Ideally everything returned by annotations(::AnnotatedString) would be semantically meaningful, that's an easy semantic to document. (and https://www.hyrumslaw.com/)

(I can't review the correctness of an optimization without knowing what "correct" means)

[tecosaur]

Ah yep, we did also discuss a need for more info in the docstrings. I'll see about adding some of the elaboration needed.

[LilithHafner]

    for (region, annot) in str.annotations
        if region == fullregion
            push!(annotations, (firstindex(unannot):lastindex(unannot), annot))
        end
    end
    for offset in 0:len:(r-1)*len
        for (region, annot) in str.annotations
            if region != fullregion
                push!(annotations, (region .+ offset, annot))
            end
        end
    end

This is buggy in the new semantic because it re-orders annotations. Not necessarily blocking this PR, but an issue.

[LilithHafner]

This is O(n^2), but we can revise this function for perf later.

[tecosaur]

Yep, and I don't think this should be in the "hot path" of any common use-cases 🤞.

[LilithHafner]

This is also O(n^2) when it could be O(n), but perf can wait.

[LilithHafner]

And it will be O(n) in non-pathological cases.

[tecosaur]

Yea, this is written with the non-pathological case in mind, and if you consider the case where few-annotation insertions are repeatedly made, individual insertions should be pretty much O(1) with a constant factor based on the average "checked annotation depth" / average number of annotations each insertion.

[LilithHafner]

I think this should be merged and followup prs should address the performance of this PR's implementations and address the now-invalid-ness of the repeat function.

[tecosaur]

I've just adjusted repeat to be consistent with the new semantic,

fullregion = firstindex(str):lastindex(str)
if allequal(first, str.annotations) && first(first(str.annotations)) == fullregion
    newfullregion = firstindex(unannot):lastindex(unannot)
    for (_, annot) in str.annotations
        push!(annotations, (newfullregion, annot))
    end
else
    for offset in 0:len:(r-1)*len
        for (region, annot) in str.annotations
            push!(annotations, (region .+ offset, annot))
        end
    end
end

this change has been folded into 2656884453 * Remove strong ordering of annotation ranges

[tecosaur]

Now that #54308 has been merged, we need to resolve the merge conflict and adjust the test introduced there.

[tecosaur]

After CI, I think this should be good to merge if that also sounds good by @LilithHafner 🙂.

[LilithHafner]

1bed84f LGTM. I have not reviewed https://github.com/JuliaLang/julia/compare/1bed84f4ae6fe6fc9bb39612e274fd1630a51349..4c18472b3e56998f2784ecd5d9deea4cf5371850, but am open to suggestions to how I could do that without re-reviewing this PR from scratch.

[tecosaur]

The only change made here in the diff you link is

    @test chopprefix(sprint(show, str), "Base.") ==
-        "AnnotatedString{String}(\"some string\", [(1:4, :thing => 0x01), (1:11, :all => 0x03), (6:11, :other => 0x02)])"
+        "AnnotatedString{String}(\"some string\", [(1:4, :thing => 0x01), (6:11, :other => 0x02), (1:11, :all => 0x03)])"

i.e. correctly resolving the change introduced by #54308.

HTH.

[LilithHafner]

Okay, the diff in your comment also LGTM.

[IanButterworth]

@tecosaur I know you have a preference to not squash but for things that will get backported, it introduces unnecessary stumbling blocks in that automated process that would be better to avoid.

[tecosaur]

Thanks for mentioning that Ian, I do like to preserve more granular information when it seems sensible, but I wasn't aware that complicated the backporting process. I'll keep that in mind with the few remaining PRs I'm hoping to get backported 🙂.

(As an aside, is the automated backporting doing something other than cherry picking? Or is the hassle having to supply -m?)

[IanButterworth]

Yeah the automatic script doesn't handle adding -m. If you wanted to help with adding that it's here https://github.com/KristofferC/Backporter/blob/master/backporter.jl