Skip to content

strings: type assert in the generic nextind, prevind methods#57608

Merged
fingolfin merged 6 commits intoJuliaLang:masterfrom
nsajko:Base_strings_prevind_nextind_type_assert
Mar 3, 2025
Merged

strings: type assert in the generic nextind, prevind methods#57608
fingolfin merged 6 commits intoJuliaLang:masterfrom
nsajko:Base_strings_prevind_nextind_type_assert

Conversation

@nsajko
Copy link
Copy Markdown
Member

@nsajko nsajko commented Mar 2, 2025 •
edited
Loading

The type assertions are valid according to the doc strings of these functions in the case of AbstractString.

Should prevent some invalidation on loading user code.

Fixes #57605

@nsajko
Copy link
Copy Markdown
Member Author

nsajko commented Mar 2, 2025

Decreases the number of invalidations on loading the following code from 1996 to 1472:

struct I <: Integer end
Base.Int(::I) = 7

@nsajko nsajko force-pushed the Base_strings_prevind_nextind_type_assert branch from 009b773 to 6b0037f Compare March 2, 2025 15:58
@Moelf
Copy link
Copy Markdown
Contributor

Moelf commented Mar 2, 2025

But why are these assertions needed in the first place, the method signature already fixed these them to be Int, seems weird

nsajko added 6 commits March 2, 2025 22:10
@nsajko
strings: type assert Int in the generic nextind, prevind methods
0f43d99 
The type assertions are valid according to the doc strings of these two
functions in the case of `AbstractString`.

Should prevent some invalidation of user code.

Fixes JuliaLang#57605
@nsajko
add test
f8c883f
@nsajko
stronger assertions
1c86f6d
@nsajko
delete unnecessary type asserts on i
17e384e
@nsajko
instead of doing n::Int do isvalid(...)::Bool
400ca90
@nsajko
also type assert thisind and ncodeunits calls
7b174df 
Maybe it prevents more invalidation.
@nsajko nsajko force-pushed the Base_strings_prevind_nextind_type_assert branch from 5ce1cc4 to 7b174df Compare March 2, 2025 21:10
@nsajko
Copy link
Copy Markdown
Member Author

nsajko commented Mar 2, 2025

But why are these assertions needed in the first place

For good return type inference, we only needed the assertion on n, as pointed out by @jishnub on Slack, the assertion on i was not necessary.

The current PR removes even the type assertion on n in favor of asserting at an earlier point, on the isvalid call: the type of n isn't known if the return type of isvalid isn't known.

@nsajko nsajko changed the title strings: type assert Int in the generic nextind, prevind methods strings: type assert in the generic nextind, prevind methods Mar 2, 2025
@fingolfin fingolfin merged commit 6c9c336 into JuliaLang:master Mar 3, 2025
9 checks passed
@nsajko nsajko deleted the Base_strings_prevind_nextind_type_assert branch March 3, 2025 12:01
@nsajko nsajko added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 labels Mar 3, 2025
KristofferC pushed a commit that referenced this pull request Mar 4, 2025
@nsajko @KristofferC
strings: type assert in the generic nextind, prevind methods (#57608
38644b3 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
@KristofferC KristofferC mentioned this pull request Mar 4, 2025
Merged
KristofferC pushed a commit that referenced this pull request Mar 11, 2025
@nsajko @KristofferC
strings: type assert in the generic nextind, prevind methods (#57608
7f4e026 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
@KristofferC KristofferC mentioned this pull request Mar 11, 2025
Merged
71 tasks
KristofferC pushed a commit that referenced this pull request Mar 11, 2025
@nsajko @KristofferC
strings: type assert in the generic nextind, prevind methods (#57608
1a1dd13 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
@KristofferC KristofferC mentioned this pull request Mar 11, 2025
Merged
75 tasks
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Mar 24, 2025
KristofferC pushed a commit that referenced this pull request Mar 25, 2025
@nsajko @KristofferC
strings: type assert in the generic nextind, prevind methods (#57608
6b4e426 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
@nsajko @KristofferC
strings: type assert in the generic nextind, prevind methods (#57608
4362960 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
@nsajko @KristofferC
strings: type assert in the generic nextind, prevind methods (#57608
7f9d33d 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Apr 10, 2025
KristofferC pushed a commit that referenced this pull request Jun 5, 2025
@nsajko @KristofferC
strings: type assert in the generic nextind, prevind methods (#57608
230c7b6 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
KristofferC pushed a commit that referenced this pull request Jul 3, 2025
@nsajko
strings: type assert in the generic nextind, prevind methods (#57608
94ec475 
)

The type assertions are valid according to the doc strings of these
functions in the case of `AbstractString`.

Should prevent some invalidation on loading user code.

Fixes #57605

(cherry picked from commit 6c9c336)
@DilumAluthge DilumAluthge mentioned this pull request Sep 13, 2025
Merged
71 tasks
@KristofferC KristofferC mentioned this pull request Nov 20, 2025
Merged
@KristofferC KristofferC mentioned this pull request Dec 12, 2025
Merged
@KristofferC KristofferC mentioned this pull request Jan 3, 2026
Merged
@github-actions github-actions bot removed the backport 1.10 Change should be backported to the 1.10 release label Jan 18, 2026
@KristofferC KristofferC mentioned this pull request Mar 5, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees

No one assigned

Labels

invalidations strings "Strings!"

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

suboptimal inference for nextind, prevind on AbstractString

4 participants

@nsajko @Moelf @fingolfin @KristofferC