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

Make linter for stable sorting #4016

Open
Tracked by #4622
ValarDragon opened this issue Jan 14, 2023 · 4 comments
Open
Tracked by #4622

Make linter for stable sorting #4016

ValarDragon opened this issue Jan 14, 2023 · 4 comments

Comments

@ValarDragon
Copy link
Member

Background

We should make a simple linter to just check for instances of sort.Slice, and replacing it with sort.SliceStable, and sort.Sort -> sort.Stable.

(The application of this change is breaking)

Unstable sort behavior can (validly) change between golang minor releases, so should never be relied upon. Pretty basic issue to get eliminated from our concern list. cref #3762

Ideally this could be in our golangci-linter. If thats hard for whatever reason we can add a custom golang linting step.

Suggested Design

Some basic regex to handle these.

Acceptance Criteria

We have a linter warning for any unstable sort usage.

@cipherzzz
Copy link
Contributor

@ValarDragon - I can pick this up

@ValarDragon
Copy link
Member Author

ValarDragon commented Mar 3, 2023

Couldn't find a convenient linter for golang that bans certain regex expressions. So to do this, I think we should:

  • Find the list of golang sorts that are not stable from https://pkg.go.dev/sort
  • Make a grep or a simple CLI command that will return any usage of these, across all go files in the repo.
  • Require as a step in make lint running this command, ensuring there are 0 such usages. If there are any usages, have an error directing to use the stable variant of that sort.
  • Check in CI that this is failing in our repo, and fix the failures.

@hoangdv2429
Copy link
Contributor

Can I pick this up ?

@hoangdv2429
Copy link
Contributor

@ValarDragon I think we need AST for this as grep Slice and Sort will not be sufficient. What do you think ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Triage 🔍
Development

No branches or pull requests

4 participants