Skip to content

Conversation

nsajko
Copy link
Member

@nsajko nsajko commented Mar 2, 2025

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
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
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
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
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
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
)

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 11, 2025
)

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
71 tasks
KristofferC pushed a commit that referenced this pull request Mar 11, 2025
)

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
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
)

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
)

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
)

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
)

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
)

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.10 Change should be backported to the 1.10 release invalidations strings "Strings!"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

suboptimal inference for nextind, prevind on AbstractString

4 participants