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

feat: add rule for deprecated rand.Seed in Go 1.20 #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dnwe
Copy link
Contributor

@dnwe dnwe commented Feb 10, 2023

@dnwe
Copy link
Contributor Author

dnwe commented Feb 10, 2023

Although now I think about this, that autofix doesn't really make sense because rand.Seed is explicitly initialising the global generator (with the assumption being that the global generator is used somewhere else in the program) whereas the replacement rand.New(rand.NewSource(...)) relies upon you using the returned Rand from that call

@dgryski
Copy link
Owner

dgryski commented Mar 2, 2023

You don't to have a "suggested" fix. You can just leave it as a warning, or maybe reference courses of action in the message.

@dnwe
Copy link
Contributor Author

dnwe commented Mar 2, 2023

@dgryski yep — although I think staticcheck will already report warnings for any use of funcs marked // Deprecated: so the benefit here was mostly being able to autofix things.

I wonder if we can do anything better with a pair of rules here as I'm assuming there were two historical uses of rand.Seed:

  1. people were calling rand.Seed with a timestamp or something "random" to result a non-deterministic sequence of pseudo-randomness from the generator — those calls can now be removed from the source in Go 1.20 and newer as it is already seeded randomly at startup and we could do that removal with an autofix
  2. people were calling rand.Seed with a constant number to explicitly cause deterministic sequence of randomness, they can given a warning with rand.New(rand.NewSource(...)) pointed to as the solution

I'm not sure how easily we could detect case 1 though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants