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

proposal: Go 2: math/rand: make globalRand public and drop package methods #25551

Closed
wking opened this issue May 24, 2018 · 2 comments
Closed
Labels
FrozenDueToAge Proposal v2 A language change or incompatible library change
Milestone

Comments

@wking
Copy link
Contributor

wking commented May 24, 2018

We could rename globalRand to GlobalRand now, and drop the package methods (e.g. rand.ExpFloat64) as part of Go 2.

The benefits I see to making the default instance public are:

  • Maintainers would get a simpler implementation (eventually). The backing code has 83 lines invested in the exposure itself, which we could drop in a stroke at Go 2. And while the tests seem to generally use local Rand instances, at least one benchmark has decided to use the global function.

    Before Go 2, the additional maintainer load seems small (a one-off s/globalRand/GlobalRand/ in rand.go and time to write up the godoc comment for the new GlobalRand.

  • Users who don't need multiple Rand would gain the ability to pass the public global instance into functions that took a Rand argument and structures that take a Rand property. Checking in the Go corpus v0.1, shows 163 such functions:

    go-corpus-0.01 $ (cd src && for DIR in . * github.com/*; do echo -n "${DIR}: "; grep -r '^func .*rand\.Rand' "${DIR}" | wc -l; done) | grep -v ' 0$'
    .: 163
    github.com: 146
    go.uber.org: 1
    golang.org: 8
    google.golang.org: 1
    k8s.io: 7
    github.com/CodisLabs: 1
    github.com/attic-labs: 19
    github.com/aws: 1
    github.com/boltdb: 2
    github.com/coreos: 1
    github.com/cznic: 1
    github.com/docker: 5
    github.com/dropbox: 1
    github.com/ethereum: 18
    github.com/gonum: 26
    github.com/gravitational: 1
    github.com/hashicorp: 1
    github.com/hyperledger: 7
    github.com/influxdata: 2
    github.com/kelseyhightower: 1
    github.com/miekg: 2
    github.com/onsi: 2
    github.com/openshift: 13
    github.com/openzipkin: 1
    github.com/oschwald: 1
    github.com/pingcap: 15
    github.com/prometheus: 6
    github.com/streadway: 2
    github.com/syndtr: 13
    github.com/yudai: 4
  • Everyone gets a simpler API once we hit Go 2 and can drop the package-level methods. Currently the math/rand docs are a longer read than they need to be because each Rand method is documented in two places.

Potential drawbacks include:

  • Increased exposure for folks who miss the need to seed their generator if they want different values (lots of discussion in math/rand: Deterministic random by default is dangerous #11871), because they would be able to stuff an unseeded global Rand into functions and structures that called for a Rand. I think this is mitigated by the current math/rand docs that recommend seeding and point at crypto/rand for security-sensitive work.

  • A few extra characters (e.g. rand.Intn -> rand.GlobalRand.Intn) for folks calling the global in Go 2+.

Personally I think the "simpler API" arguments are fairly strong. The "user convenience" argument is weaker, because calling rand.New(rand.NewSource(yourSeed())) is not much more difficult than using rand.GlobalRand after having previously called rand.GlobalRand.Seed(yourSeed()). But I don't think the arguments against are very strong either, so on the whole I come out in favor of the global. I'm not a maintainer though, and I may be missing things.

I also wouldn't be surprised if this had been proposed before, but I have been unable to turn up references to it (either in this repo's issues or in the golang-dev@ logs). The current private globalRand dates back to 3342574 (2009-10-15). The commit message has OCL and CL tags, which may be related to external review (like the modern Reviewed-on), but if so, I'm not sure where that review is stored.

@gopherbot gopherbot added this to the Proposal milestone May 24, 2018
@wking
Copy link
Contributor Author

wking commented May 24, 2018

Actually, because rand.New(rand.NewSource(yourSeed())) is so easy, I'd also be ok just dropping the package functions without replacement in Go 2. Even less to maintain, and we already have an example showing this approach. Is there a strong argument for having a global Rand instance at all, regardless of whether it's public or private? Perhaps that reason is "there's currently no public generator for locking sources" (#21393, #24121). If so, we probably need to deal with those issues before getting to this one.

@ianlancetaylor ianlancetaylor changed the title proposal: Go 2: math/rand: Make globalRand public and drop package methods proposal: Go 2: math/rand: make globalRand public and drop package methods May 24, 2018
@ianlancetaylor ianlancetaylor added the v2 A language change or incompatible library change label May 24, 2018
@ianlancetaylor
Copy link
Contributor

Folding this specific issue into the more general #26263 .

@golang golang locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

3 participants