Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid method overwriting in julia1-compat branch #57

Open
kimikage opened this issue May 7, 2024 · 15 comments
Open

Avoid method overwriting in julia1-compat branch #57

kimikage opened this issue May 7, 2024 · 15 comments

Comments

@kimikage
Copy link

kimikage commented May 7, 2024

StyledStrings v1.0 currently does method overwriting.

## From basic.jl
function (*)(s1::Union{AbstractChar, AbstractString}, ss::Union{AbstractChar, AbstractString}...)
if _isannotated(s1) || any(_isannotated, ss)
annotatedstring(s1, ss...)
else
string(s1, ss...)
end
end
# From io.jl
join(iterator) = _join_preserve_annotations(iterator)
join(iterator, delim) = _join_preserve_annotations(iterator, delim)
join(iterator, delim, last) = _join_preserve_annotations(iterator, delim, last)

At least for *, we should be able to avoid overwriting by defining the following (or something like):

function (*)(s1::Union{AnnotatedChar, AnnotatedString}, s2::Union{AbstractChar, AbstractString})
    annotatedstring(s1, s2)
end

function (*)(s1::Union{AbstractChar, AbstractString}, s2::Union{AnnotatedChar, AnnotatedString})
    annotatedstring(s1, s2)
end

function (*)(s1::Union{AnnotatedChar, AnnotatedString}, s2::Union{AnnotatedChar, AnnotatedString})
    annotatedstring(s1, s2)
end

function (*)(s1::Union{AbstractChar, AbstractString}, s2::Union{AbstractChar, AbstractString}, ss::Union{AbstractChar, AbstractString}...)
    if _isannotated(s1) || _isannotated(s2) || any(_isannotated, ss)
        annotatedstring(s1, s2, ss...)
    else
        string(s1, s2, ss...)
    end
end

Edit:
An alternative is to require that the first 2 or 3 arguments should contain Union{AnnotatedChar, AnnotatedString}.

@tecosaur
Copy link
Collaborator

tecosaur commented May 7, 2024

Hmmm, that's an interesting suggestion. At a glance, that looks viable, but I don't think we can do something similar with join unfortunately.

@kimikage
Copy link
Author

kimikage commented May 7, 2024

I think limited support would be more beneficial (i.e., less troublesome) than method overwriting.

@tecosaur
Copy link
Collaborator

tecosaur commented May 7, 2024

Maybe, the goal I went into with this though was to make it behave as close to the 1.11 stdlib as possible, and it's extremely close. As far as I'm aware, the problem with method overwriting here is he latency impact, is there anything beyond that?

@kimikage
Copy link
Author

kimikage commented May 7, 2024

Whether or not method overwriting is performed is a greater difference than missing annotations in some cases.
The downstream package can avoid the latter if necessary, but not control the overwriting in StyledStrings.

@tecosaur
Copy link
Collaborator

tecosaur commented May 7, 2024

Right, but what's the negative impact of the method overwriting? AFAIU it's just the latency.

@kimikage
Copy link
Author

kimikage commented May 7, 2024

This is a tautology, but it is a problem because method overwriting is a problem that is worthy of a warning.

Older julia in particular are somewhat fragile with precompilation.
I think other core developers are more knowledgeable about the specific issues. So it might be good to ask the opinions of the experts on Slack, Zulip, or Discourse.

Also, I think we need to clarify the relationship between this package and Compat.jl.

@kimikage
Copy link
Author

kimikage commented May 7, 2024

In the first place, the join is buggy.
JuliaLang/julia#51914

any(_isannotated, args) # `any(_isannotated, iterator)` ?
julia> join(("a", styled"{bold:b}"), ", ")|>typeof
String

julia> join(["a", styled"{bold:b}"], ", ")|>typeof
Base.AnnotatedString{String}

julia> versioninfo()
Julia Version 1.12.0-DEV.469
Commit 0d1d4ba068 (2024-05-06 21:04 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 8 × 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
  WORD_SIZE: 64
  LLVM: libLLVM-17.0.6 (ORCJIT, tigerlake)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)

@tecosaur
Copy link
Collaborator

tecosaur commented May 7, 2024

Right, but the overwriting doesn't happen during precompilation. Chatting about method overwriting on Triage with Jeff, he said method overwriting is a fine thing to do.

Also, I think we need to clarify the relationship between this package and Compat.jl.

There is no relation, I can't say I see the need?

@tecosaur
Copy link
Collaborator

tecosaur commented May 7, 2024

In the first place, the join is buggy.

Looks like that's worthy of an issue in JuliaLang/julia, either the join check will need to be tweaked or _isannotated needs another method.

@kimikage
Copy link
Author

kimikage commented May 7, 2024

My point here (not in JuliaLang/julia) is that even with limited support for join, no major problems have arisen so far.

The following is just a PoC, but it works minimally.

join(iterator, delim::Union{AbstractString, AbstractChar}) = _join_preserve_annotations(iterator, delim)
join(iterator, delim::Union{AbstractString, AbstractChar}, last) = _join_preserve_annotations(iterator, delim, last)

Edit:
Or more conservatively:

join(iterator, delim::Union{AnnotatedChar, AnnotatedString}) = _join_preserve_annotations(iterator, delim)
join(iterator, delim::Union{AnnotatedChar, AnnotatedString}, last) = _join_preserve_annotations(iterator, delim, last)

@kimikage
Copy link
Author

kimikage commented May 7, 2024

Also, I think we need to clarify the relationship between this package and Compat.jl.

There is no relation, I can't say I see the need?

The AnnotatedString is practically for StyledStrings, but is not the property of StyledStrings.
What is being done here is, in principle, piracy.
I do not believe that the piracy itself is the problem. However, I do think some statement should be issued about not doing it on Compat.jl.

@tecosaur
Copy link
Collaborator

tecosaur commented May 7, 2024

The AnnotatedString is practically for StyledStrings, but is not the property of StyledStrings.

Ah, right I follow now.

@tecosaur
Copy link
Collaborator

tecosaur commented May 7, 2024

any(_isannotated, args) # `any(_isannotated, iterator)` ?

Not quite, we do want any(_isannotated, args), but it might be worth adding || any(_isannotated, iterator).

@kimikage
Copy link
Author

kimikage commented May 7, 2024

xref: https://discourse.julialang.org/t/method-overwriting-in-styledstrings-jl-v1-0-julia1-compat/113935
(for general discussions)

tecosaur added a commit to tecosaur/julia that referenced this issue Jun 24, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, _isannotated can
erroneously return false. To properly check such cases, we need to see
if any of the elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we reasonably expect most
  iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time,
  and so in any case other than the ambiguous non-concrete, this check
  remains able to be done at compile-time.
- Should the iterator be stateful and non-concrete, the check can
  consume some amount of the iterator before join is called.

If it weren't for the edge-case of stateful iterators, I'd say this
would be a fairly clean/straightforward fix, but since they exist I'm
currently tempted to think the best compromise is just to avoid checking
any Iterators.Stateful iterators. If somebody can work out a nicer
compromise, that would be grand.

Reported-by: kimikage <kimikage.ceo@gmail.com>
tecosaur added a commit to tecosaur/julia that referenced this issue Jun 24, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, _isannotated can
erroneously return false. To properly check such cases, we need to see
if any of the elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we reasonably expect most
  iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time,
  and so in any case other than the ambiguous non-concrete one, this
  check remains able to be done at compile-time.
- Should the iterator be stateful and non-concrete, the check can
  consume some amount of the iterator before join is called.

If it weren't for the edge-case of stateful iterators, I'd say this
would be a fairly clean/straightforward fix, but since they exist I'm
currently tempted to think the best compromise is just to avoid checking
any Iterators.Stateful iterators. If somebody can work out a nicer
compromise, that would be grand.

Reported-by: kimikage <kimikage.ceo@gmail.com>
tecosaur added a commit to tecosaur/julia that referenced this issue Jul 4, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, _isannotated will return
false. To properly check such cases, we need to see if any of the
elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we reasonably expect most
  iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time,
  and so in any case other than the ambiguous non-concrete one, this
  check remains able to be done at compile-time.
- Should the iterator be stateful and non-concrete, the check can
  consume some amount of the iterator before join is called.

With this change, join always preserves annotations. The compromise made
is that iterators with non-concrete eltypes can result in join inferring
a union return type (i.e. type instability with non-concrete iterators),
but that doesn't seem too bad to me.

Reported-by: kimikage <kimikage.ceo@gmail.com>
tecosaur added a commit to tecosaur/julia that referenced this issue Jul 4, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, _isannotated will return
false. To properly check such cases, we need to see if any of the
elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we reasonably expect most
  iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time,
  and so in any case other than the ambiguous non-concrete one, this
  check remains able to be done at compile-time.

With this change, join always preserves annotations. The compromise made
is that iterators with non-concrete eltypes can result in join inferring
a union return type (i.e. type instability with non-concrete iterators),
but that doesn't seem too bad to me.

Reported-by: kimikage <kimikage.ceo@gmail.com>
tecosaur added a commit to tecosaur/julia that referenced this issue Jul 22, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, _isannotated will return
false. To properly check such cases, we need to see if any of the
elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we reasonably expect most
  iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time,
  and so in any case other than the ambiguous non-concrete one, this
  check remains able to be done at compile-time.

With this change, join always preserves annotations. The compromise made
is that iterators with non-concrete eltypes can result in join inferring
a union return type (i.e. type instability with non-concrete iterators),
but that doesn't seem too bad to me.

Reported-by: kimikage <kimikage.ceo@gmail.com>
fingolfin pushed a commit to JuliaLang/julia that referenced this issue Jul 24, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, `_isannotated` will
return false. To properly check such cases, we need to see if any of the
elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we can reasonably expect most
iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time, and
so in any case other than the ambiguous non-concrete one, this check
remains able to be done at compile-time.

With this change, join always preserves annotations. The compromise made
is that iterators with non-concrete eltypes can result in join inferring
a union return type (i.e. type instability with non-concrete iterators),
but that doesn't seem too bad to me (I don't see how we could be
completely type stable without concrete types here).
KristofferC pushed a commit to JuliaLang/julia that referenced this issue Aug 2, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, `_isannotated` will
return false. To properly check such cases, we need to see if any of the
elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we can reasonably expect most
iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time, and
so in any case other than the ambiguous non-concrete one, this check
remains able to be done at compile-time.

With this change, join always preserves annotations. The compromise made
is that iterators with non-concrete eltypes can result in join inferring
a union return type (i.e. type instability with non-concrete iterators),
but that doesn't seem too bad to me (I don't see how we could be
completely type stable without concrete types here).

(cherry picked from commit 462d7f4)
@KristofferC
Copy link
Member

FWIW, here is some snoop compile output:

julia> using SnoopCompileCore

julia> invalidations = @snoop_invalidations using StyledStrings

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
...
 deleting join(iterator) @ Base strings/io.jl:356 invalidated:
   backedges: 1: superseding join(iterator) @ Base strings/io.jl:356 with MethodInstance for join(::Vector{Char}) (1 children)
             ...
              8: superseding join(iterator) @ Base strings/io.jl:356 with MethodInstance for join(::Tuple{String}) (596 children)
   mt_disable: MethodInstance for join(::Vector{String})
               +5 more

 deleting join(iterator, delim) @ Base strings/io.jl:357 invalidated:
   backedges:  1: superseding join(iterator, delim) @ Base strings/io.jl:357 with MethodInstance for join(::Vector{Symbol}, ::Char) (1 children)
              ...
              11: superseding join(iterator, delim) @ Base strings/io.jl:357 with MethodInstance for join(::Base.Generator{Base.Iterators.Filter{Base.BinaryPlatforms.var"#14#16", Dict{String, String}}, Base.BinaryPlatforms.var"#13#15"}, ::String) (359 children)
              12: superseding join(iterator, delim) @ Base strings/io.jl:357 with MethodInstance for join(::Vector{SubString{String}}, ::String) (620 children)
              13: superseding join(iterator, delim) @ Base strings/io.jl:357 with MethodInstance for join(::Vector{Any}, ::String) (4123 children)
   mt_disable: MethodInstance for join(::Tuple{Symbol}, ::String)

   inserting *(s1::Union{AbstractChar, AbstractString}, s2::Union{AbstractChar, AbstractString}, ss::Union{AbstractChar, AbstractString}...) @ StyledStrings.AnnotatedStrings ~/.julia/packages/StyledStrings/fsZJu/src/strings/basic.jl:21 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Union{Nothing, String}, String} triggered MethodInstance for Pkg.Operations.var"#download_source#44"(::Bool, ::typeof(Pkg.Operations.download_source), ::Pkg.Types.Context) (0 children)
            ....
              13: superseding *(s1::Union{AbstractChar, AbstractString}, ss::Union{AbstractChar, AbstractString}...) @ Base strings/basic.jl:260 with MethodInstance for *(::String, ::String, ::String) (114 children)
              14: superseding *(s1::Union{AbstractChar, AbstractString}, ss::Union{AbstractChar, AbstractString}...) @ Base strings/basic.jl:260 with MethodInstance for *(::String, ::String, ::String, ::String, ::String, ::String, ::String) (1160 children)
              15: superseding *(s1::Union{AbstractChar, AbstractString}, ss::Union{AbstractChar, AbstractString}...) @ Base strings/basic.jl:260 with MethodInstance for *(::String, ::String) (6902 children)

lazarusA pushed a commit to lazarusA/julia that referenced this issue Aug 17, 2024
As raised in
<JuliaLang/StyledStrings.jl#57 (comment)>,
when the eltype of an iterator is non-concrete, `_isannotated` will
return false. To properly check such cases, we need to see if any of the
elements of the iterator are annotated.

This is a bit of an interesting case, since:
- Most of the time it shouldn't be hit, we can reasonably expect most
iterables to infer as producing concrete types
- The eltype of the iterator is (generally) known at compile-time, and
so in any case other than the ambiguous non-concrete one, this check
remains able to be done at compile-time.

With this change, join always preserves annotations. The compromise made
is that iterators with non-concrete eltypes can result in join inferring
a union return type (i.e. type instability with non-concrete iterators),
but that doesn't seem too bad to me (I don't see how we could be
completely type stable without concrete types here).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants