Allow types other than `Int` in `randstring` #54402

Seelengrab · 2024-05-08T09:13:01Z

What remains is that the effectively allowed input still needs to be convertible to Int and non-negative. So things like typemax(Int128) won't work.

Fixes JuliaLang#54397 What remains is that the effectively allowed input still needs to be convertible to `Int`. So things like `typemax(Int128)` won't work, and neither will negative values.

KristofferC · 2024-05-08T10:33:00Z

stdlib/Random/src/misc.jl

    global randstring

    function randstring(r::AbstractRNG, chars=b, n::Integer=8)
+        _n = convert(Int, n)


I am not sure it is worth doing in this specific instance but presumably structuring it like:

f(x::Integer) = f(convert(Int, n)) f(x::Int) = ...

would reduce compilation time since you would not compile the main function body for multiple integer types.

For cases where lots of different types are passed to randstring, that's certainly the case, yeah. I'm not sure how common that would be though, there's lots of better ways to control the length of a generated string other than through the type.

I think @KristofferC is just referring to the amount of code that is generated by using function barriers or:
https://docs.julialang.org/en/v1/manual/performance-tips/#kernel-functions

Yes, I'm referring to the likelihood of hitting that & the additional compilation overhead being a significant slowdown. If this is a bottleneck, I'd first recommend switching to a different scheme for generating the length (with just Int) than optimize the compilation overhead.

Right, and the problem is that this PR would allow someone to have a type unstable call site with various Integer types for length, rather than error as it currently does.

Why would that inherently be a problem? We don't prevent type instabilities in user code in other places either.

I think Krostoffer's convert approach is a good idea. Yes, it's ok to just allow a different integer type flow through but in this case Int covers the entire range of reasonable values and converting avoids additional compilation.

There is no other integer type flowing through though; the code in this PR already calls _convert(Int, ..), just directly instead of in a function barrier. This function is a total of 11 lines long, I seriously doubt that this leads to a bottleneck in either compilation time or binary size. At that size, I'm willing to bet that the inlining pass costs more than potential dual compilation if anyone calls this with an UInt8..

Ok, but every little bit counts. Why not just change it?

Because I agree with the comment by Kristoffer that it's probably not worth doing in this case. Is it a problem that I agree with the reviewer? As it stands, this just seems like premature optimization of a hypothetical to me.

sjkelly · 2024-05-09T10:46:54Z

It is a problem because as this PR is currently written you will generate a lot of redundant code. You should accept Kristoffer's suggestion for the signatures as it will improve the quality of this PR.

…

On Thu, May 9, 2024, 02:07 Sukera ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In stdlib/Random/src/misc.jl <#54402 (comment)>: > @@ -71,13 +71,14 @@ let b = UInt8['0':'9';'A':'Z';'a':'z'] global randstring function randstring(r::AbstractRNG, chars=b, n::Integer=8) + _n = convert(Int, n) Why would that inherently be a problem? We don't prevent type instabilities in user code in other places either. — Reply to this email directly, view it on GitHub <#54402 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAP5MDAUQXLRZEXJIYX554LZBMHALAVCNFSM6AAAAABHMSEKMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBXGIYTEMJQHA> . You are receiving this because you commented.Message ID: ***@***.***>

Seelengrab · 2024-05-09T12:06:54Z

Since Kristoffer mentioned that it's probably not worth it in this PR, I've chosen to keep the code simpler to follow without having to disentangle niche dispatch/compilation optimizations.

Seelengrab · 2024-06-11T15:52:58Z

Does this need anything else (cc @KristofferC)?

rfourquet · 2025-05-30T10:55:49Z

I'm inclined to merge as is, and if someone is motivated to make a follow-up PR to make the suggested change (a similar change could also be made to #58533 at the same time), that could be merged as well. This PR is somewhat a fix and doesn't make anything worse, so it should be included in the next release in some form or another.

nsajko · 2025-06-01T11:04:56Z

stdlib/Random/src/misc.jl

    global randstring

    function randstring(r::AbstractRNG, chars=b, n::Integer=8)
+        _n = convert(Int, n)


How about this:

Suggested change

_n = convert(Int, n)

n = convert(Int, n)::Int

Then the rest of the diff can be dropped.

wdyt @Seelengrab ?

Won't this cause a type instability, since the original argument was ::Integer? Or does this assertion override the originally inferred type for n?

I believe it's fine yes.

There is no type instability. When code like n = f(n) gets lowered to SSA form, there are two different variables, for example n@_1 and n@_2.

See for yourself with code_lowered or code_typed.

However I'd also suggest just calling the Int constructor instead of convert:

Suggested change

_n = convert(Int, n)

n = Int(n)::Int

nsajko · 2025-06-01T11:05:14Z

stdlib/Random/src/misc.jl

            return str
        else
-            v = Vector{T}(undef, n)
+            v = Vector{T}(undef, _n)


Suggested change

v = Vector{T}(undef, _n)

v = Vector{T}(undef, n)

nsajko · 2025-06-01T11:05:29Z

stdlib/Random/src/misc.jl

+            str = Base._string_n(_n)
+            GC.@preserve str rand!(r, UnsafeView(pointer(str), _n), chars)


Suggested change

str = Base._string_n(_n)

GC.@preserve str rand!(r, UnsafeView(pointer(str), _n), chars)

str = Base._string_n(n)

GC.@preserve str rand!(r, UnsafeView(pointer(str), n), chars)

Allow types other than Int in randstring

1259b36

Fixes JuliaLang#54397 What remains is that the effectively allowed input still needs to be convertible to `Int`. So things like `typemax(Int128)` won't work, and neither will negative values.

Seelengrab added the randomness Random number generation and the Random stdlib label May 8, 2024

KristofferC reviewed May 8, 2024

View reviewed changes

rfourquet approved these changes Mar 27, 2025

View reviewed changes

nsajko reviewed Jun 1, 2025

View reviewed changes

adienes added the status: waiting for PR author label Oct 5, 2025

		str = Base._string_n(_n)
		GC.@preserve str rand!(r, UnsafeView(pointer(str), _n), chars)

Uh oh!

Allow types other than Int in randstring #54402

Are you sure you want to change the base?

Allow types other than Int in randstring #54402

Conversation

Seelengrab commented May 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Seelengrab May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjkelly May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Seelengrab Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjkelly commented May 9, 2024 via email

Uh oh!

Seelengrab commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Seelengrab commented Jun 11, 2024

Uh oh!

rfourquet commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Seelengrab Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsajko Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Allow types other than `Int` in `randstring` #54402

Allow types other than `Int` in `randstring` #54402

Seelengrab May 8, 2024 •

edited

Loading

sjkelly May 8, 2024 •

edited

Loading

Seelengrab Jun 11, 2024 •

edited

Loading

Seelengrab commented May 9, 2024 •

edited

Loading

Seelengrab Oct 21, 2025 •

edited

Loading

nsajko Oct 21, 2025 •

edited

Loading