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

Fix annotated join with non-concrete eltype iters #54919

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

tecosaur
Copy link
Contributor

@tecosaur tecosaur commented Jun 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).

Before

image

After

image

I'd be keen to hear people's thoughts here.

@tecosaur tecosaur added strings "Strings!" bugfix This change fixes an existing bug labels Jun 24, 2024
@tecosaur tecosaur added the backport 1.11 Change should be backported to release-1.11 label Jul 4, 2024
@tecosaur tecosaur force-pushed the fix-annot-join-nonconcrete branch 2 times, most recently from 87c93b9 to 6a1fec0 Compare July 4, 2024 16:39
@tecosaur tecosaur mentioned this pull request Jul 4, 2024
42 tasks
@tecosaur
Copy link
Contributor Author

The CI failures seems to come from something with Sockets on the windows CI agents, and what appears to be an unrelated failure on test x86_64-linux-gnuassertrr-net (I see poptask in the logs, and nothing string related).

@adienes
Copy link
Contributor

adienes commented Jul 22, 2024

iiuc, the tradeoff is

  • now there is a type instability, possibly hurting performance if a concrete type is not inferred
  • before there was a semantic ambiguity, possibly changing behavior if a concrete type is not inferred

if that's the case, I would much prefer to risk performance than to risk behavior

@tecosaur
Copy link
Contributor Author

tecosaur commented Jul 22, 2024

@adienes Yep. This results in a Union when a concrete eltype can't be inferred, but I expect any performance-sensitive applications of join likely have concrete eltypes already, and otherwise can just wrap the result in String(...) or AnnotatedString(...) to be type stable again.

We could maybe avoid more non-concrete eltypes with @default_eltype instead of eltype? That could be explored in a subsequent PR though.

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 <[email protected]>
@tecosaur tecosaur force-pushed the fix-annot-join-nonconcrete branch from 6a1fec0 to ab26f4f Compare July 22, 2024 14:23
@fingolfin fingolfin merged commit 462d7f4 into JuliaLang:master Jul 24, 2024
7 checks passed
KristofferC added a commit that referenced this pull request Jul 26, 2024
Backported PRs:
- [x] #54201 <!-- Fix generic triangular solves with empty matrices -->
- [x] #54358 <!-- Create `jl_clear_coverage_data` to dynamically reset
coverage -->
- [x] #54908 <!-- LazyString in interpolated error messages in
threadingconstructs -->
- [x] #54952 <!-- LAPACK: Avoid repr call in `chkvalidparam` -->
- [x] #54898 <!-- fix concurrent module loading return value -->
- [x] #55082 <!-- Add fast method for copyto!(::Memory, ::Memory) -->
- [x] #55084 <!-- Use triple quotes in TOML.print when string contains
newline -->
- [x] #55141 <!-- Update the aarch64 devdocs to reflect the current
state of its support -->
- [x] #54955 <!-- don't throw EOFError from sleep -->
- [x] #54871 <!-- Make warn missed transformations pass optional -->
- [x] #55178 <!-- Compat for `Base.@nospecializeinfer` -->
- [x] #55197 <!-- compat notice for a[begin] indexing -->
- [x] #54917 <!-- Fix potential underrun with annotation merging -->
- [x] #55209 <!-- correction to compat notice for a[begin] -->
- [x] #55203 <!-- document mutable struct const fields -->
- [x] #54791 <!-- Bump libblastrampoline to v5.10.1 -->
- [x] #54950 <!-- SuiteSparse: Bump version -->
- [x] #54956 <!-- Fix accidental early evaluation of imported `using`
binding -->
- [x] #54996 <!-- inference: add missing `MustAlias` widening in
`_getfield_tfunc` -->
- [x] #55070 <!-- LinearAlgebra: LazyString in error messages for
Diagonal/Bidiagonal -->
- [x] #54574 <!-- Make ScopedValues public -->
- [x] #54739 <!-- finish implementation of upgradable stdlibs -->
- [x] #54965 <!-- RFC: Make `include_dependency(path;
track_content=true)` the default -->
- [x] #53286 <!-- Raise an error when using `include_dependency` with
non-existent file or directory -->
- [x] #55066 <!-- fix loading of repeated/concurrent modules -->
- [x] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [x] #55218 <!-- Artifacts: use a different way of getting the UUID of
a module -->
- [x] #54891 <!-- #54739-related fixes for loading stdlibs -->
- [x] #55072 <!-- trace-compile: don't generate `precompile` statements
for OpaqueClosure methods -->
- [x] #55188 <!-- Make Core.TypeofUnion use the type method table -->

Need manual backport:
- [ ] #55052 <!-- Fix `(l/r)mul!` with `Diagonal`/`Bidiagonal` -->


Contains multiple commits, manual intervention needed:

Non-merged PRs with backport label:
- [ ] #55169 <!-- `propertynames` for SVD respects private argument -->
- [ ] #55148 <!-- Random: Mark unexported public symbols as public -->
- [ ] #55017 <!-- TOML: Make `Dates` a type parameter -->
- [ ] #55013 <!-- [docs] change docstring to match code -->
- [ ] #54919 <!-- Fix annotated join with non-concrete eltype iters -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #50813 <!-- More doctests for Sockets and capitalization fix -->
- [ ] #50157 <!-- improve docs for `@inbounds` and
`Base.@propagate_inbounds` -->
- [ ] #41244 <!-- Fix shell `cd` error when working dir has been deleted
-->
KristofferC pushed a commit that referenced this pull request 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 KristofferC mentioned this pull request Aug 2, 2024
68 tasks
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Aug 2, 2024
@KristofferC
Copy link
Member

KristofferC commented Aug 2, 2024

Considering #55279, this should probably not be backported?

@KristofferC KristofferC added the backport 1.11 Change should be backported to release-1.11 label Aug 2, 2024
@tecosaur
Copy link
Contributor Author

tecosaur commented Aug 2, 2024

Mmm, it would be worth doing if #55279 is resolved, but it doesn't look like that's going to be easy/quick (to me at least).

tecosaur added a commit to JuliaLang/StyledStrings.jl that referenced this pull request Aug 5, 2024
@tecosaur tecosaur deleted the fix-annot-join-nonconcrete branch August 11, 2024 04:45
lazarusA pushed a commit to lazarusA/julia that referenced this pull request 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).
tecosaur added a commit to JuliaLang/StyledStrings.jl that referenced this pull request Aug 24, 2024
KristofferC added a commit that referenced this pull request Aug 26, 2024
Backported PRs:
- [x] #54962 <!-- Add timing to precompile trace compile -->
- [x] #55180 <!-- compress jit debuginfo for easy memory savings -->
- [x] #54919 <!-- Fix annotated join with non-concrete eltype iters -->
- [x] #55013 <!-- [docs] change docstring to match code -->
- [x] #55017 <!-- TOML: Make `Dates` a type parameter -->
- [x] #54033 <!-- Fix a bug in `stack`'s DimensionMismatch error message
-->
- [x] #55242 <!-- fix at-main docstring to not code quote a compat box
-->
- [x] #55261 <!-- Make `jl_*affinity` tests more portable -->
- [x] #54736 <!-- specificity: ensure fast-path in `sub/eq_msp` handle
missing `UnionAll` wrapper correctly. -->
- [x] #55299 <!-- typeintersect: fix bounds merging during inner
`intersect_all`. -->
- [x] #55302 <!-- Add `lbt_forwarded_funcs()` to debug LBT forwarding
issues -->
- [x] #55148 <!-- Random: Mark unexported public symbols as public -->
- [x] #55303 <!-- avoid overflowing show for OffsetArrays around typemax
-->
- [x] #55317 <!-- Restrict argument to `isleapyear(::Integer)` -->
- [x] #55327 <!-- Profile: Fix stdlib paths -->
- [x] #55330 <!-- [libblastrampoline] Bump to v5.11.0 -->
- [x] #55310 <!-- Preserve structure in scaling triangular matrices by
NaN -->
- [x] #55329 <!-- mapreduce: don't inbounds unknown functions -->
- [x] #55356 <!-- Profile: close files when assembling heap snapshot -->
- [x] #55371 <!-- Fix tr for block SymTridiagonal -->
- [x] #55307 <!-- Make REPL.TerminalMenus public -->
- [x] #55362 <!-- inference: fix missing LimitedAccuracy markers -->
- [x] #55306 <!-- AllocOpt: Fix stack lowering where alloca continas
boxed and unboxed data -->
- [x] #55395 <!-- fix #55389: type-unstable `join` -->
- [x] #55226 <!-- re-add `unsafe_convert` for Reinterpret and Reshaped
array -->
- [x] #55405 <!-- handle unbound vars in NTuple fields -->
- [x] #55365 <!-- ml-matches: ensure all methods are included -->
- [x] #55428 <!-- codegen: move undef freeze before promotion point -->
- [x] #55419 <!-- `stale_cachefile`: handle if the expected cache file
is missing -->
- [x] #55470 <!-- Add push! implementation for AbstractArray depending
only on resize! -->
- [x] #55483 <!-- fix hierarchy level of "API reference" in `Dates`
documentation -->
- [x] #55268 <!-- simplify complex atanh and remove singularity
perturbation -->
- [x] #55441 <!-- fix Event to use normal Condition variable -->
- [x] #55413 <!-- subtyping: fast path for lhs union and rhs typevar -->
- [x] #55492 <!-- build: add missing dependencies for expmap -->
- [x] #55507 <!-- Fix fast getptls ccall lowering. -->
- [x] #55424 <!-- add missing clamp function for IOBuffer -->
- [x] #55504 <!-- Update symmetric docstring to reflect the type of uplo
-->
- [x] #55107 <!-- Make the memory GEP an inbounds GEP since the bounds
check has happened somewhere else -->
- [x] #55411 <!-- Vendor the terminfo database for use with
base/terminfo.jl -->
- [x] #55452 <!-- Do not load `ScopedValues` with `using` -->
- [x] #55407 <!-- Remove deprecated non string API for LLVM pass
pipeline and parse all options -->
- [x] #55461 <!-- 🤖 [master] Bump the StyledStrings stdlib from d7496d2
to f6035eb -->
- [x] #55433 <!-- Backport #55407
to 1.11 -->
- [x] #55225 <!-- [1.11 backport] trace-compile: don't generate
`precompile` statements for OpaqueClosure methods (#55072) -->
- [x] #55212 <!-- Make `Base.depwarn()` public -->
- [x] #552
- [x] #55052 <!-- Fix `(l/r)mul!` with `Diagonal`/`Bidiagonal` -->
- [x] #55251 <!-- Restrict binary ops for Diagonal and Symmetric to
Number eltypes -->95 <!-- LAPACK: Aggressive constprop to concretely
infer syev!/syevd! -->
- [x] #55522 <!-- Fix tr for Symmetric/Hermitian block matrices -->

Need manual backport:
- [x] #55342 <!-- Ensure bidiagonal setindex! does not read indices in
error message -->

Contains multiple commits, manual intervention needed:

- [ ] #55336 <!-- codegen: take gc roots (and alloca alignment) more
seriously -->


Non-merged PRs with backport label:
- [ ] #55506 <!-- Fix indexing in _mapreducedim for OffsetArrays -->
- [ ] #55500 <!-- make jl_thread_suspend_and_get_state safe -->
- [ ] #55499 <!-- propagate the terminal's `displaysize` to the
`IOContext` used by the REPL -->
- [ ] #55458 <!-- Allow for generically extracting unannotated string
-->
- [ ] #55457 <!-- Make AnnotateChar equality consider annotations -->
- [ ] #55453 <!-- Privatise the annotations API, for StyledStrings -->
- [ ] #55443 <!-- Add test for upper/lower/titlecase and fix call -->
- [ ] #55355 <!-- relocation: account for trailing path separator in
depot paths -->
- [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows -->
- [ ] #55169 <!-- `propertynames` for SVD respects private argument -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #50813 <!-- More doctests for Sockets and capitalization fix -->
- [ ] #50157 <!-- improve docs for `@inbounds` and
`Base.@propagate_inbounds` -->
- [ ] #41244 <!-- Fix shell `cd` error when working dir has been deleted
-->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants