Skip to content

feat: add validjson linter #3530

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat: add validjson linter #3530

wants to merge 1 commit into from

Conversation

leonklingele
Copy link
Contributor

@leonklingele leonklingele commented Jan 31, 2023 •
edited
Loading

The validjson linter ensures that structs with json tags have valid types, suitable for JSON serialization.

type ValidJsonTest struct {
	A int                           `json:"A"`
	B int                           `json:"B,omitempty"`
	C int                           `json:",omitempty"`
	D int                           `json:"-"`
	E chan<- int                    `json:"E"` // want "struct field has json tag but non-serializable type chan<- int"
	F chan<- int                    `json:"-"`
	G chan<- int                    `json:"-,"` // want "struct field has json tag but non-serializable type chan<- int"
	H ChanAlias                     `json:"H"`  // want "struct field has json tag but non-serializable type a.ChanAlias"
	I map[int]int                   `json:"I"`
	J map[int64]int                 `json:"J"`
	K map[Key]int                   `json:"K"` // want "struct field has json tag but non-serializable type"
	L map[struct{ A, B string }]int `json:"L"` // want "struct field has json tag but non-serializable type"
	M map[string]int                `json:"M"`
	N map[StrAlias]int              `json:"N"`
	O func()                        `json:"O"` // want "struct field has json tag but non-serializable type func()"
	P complex64                     `json:"P"` // want "struct field has json tag but non-serializable type complex64"
	Q complex128                    `json:"Q"` // want "struct field has json tag but non-serializable type complex128"
	R StrAlias                      `json:"R"`
	S map[Textable]int              `json:"S"`
}

https://github.com/matthewloring/validjson

Fixes #3477

@ldez ldez added enhancement New feature or improvement linter: new Support new linter labels Feb 1, 2023
@ldez
Copy link
Member

ldez commented Feb 1, 2023 •
edited
Loading

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

  • It must have a link to the linter repository.
  • It must provide a short description of the linter.

Linter

  • It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
  • It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
  • The linter repository must have a CI and tests.
  • It must use go/analysis.
  • It must have a valid tag, ex: v1.0.0, v0.1.0.
  • It must not contain init().
  • It must not contain panic(), log.fatal(), os.exit(), or similar.
  • It must not have false positives/negatives. (the team will help to verify that)
  • It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

  • They must have at least one std lib import.
  • They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

  • The linter must be added to the list of available linters (alphabetical case-insensitive order).
    • enable and disable options
  • If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
    • The values must be different from the default ones.
    • The default values must be defined in a comment.
    • The option must have a short description.

Others Requirements

  • The files (tests and linter) inside golangci-lint must have the same name as the linter.
  • The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
  • The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
  • The load mode (WithLoadMode(...)):
    • if the linter doesn't use types: goanalysis.LoadModeSyntax
    • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
  • The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

  • The linter should not use SSA. (currently, SSA does not support generics)
  • The linter repository should have a readme and linting.
  • The linter should be published as a binary. (useful to diagnose bug origins)

The golangci-lint team will edit this comment to check the boxes before and during the review.

This checklist does not imply that we will accept the linter.

@ldez ldez added the feedback required Requires additional feedback label Feb 1, 2023
alexandear
alexandear reviewed Feb 2, 2023
View reviewed changes
test/testdata/validjson.go
return nil
}

type ValidJsonTest struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should change the name?

Suggested change
type ValidJsonTest struct {
type ValidJSONTest struct {

@Antonboom
Copy link
Contributor

Looks like part of https://github.com/breml/errchkjson 🤔
(cc) @breml

@breml
Copy link
Contributor

breml commented Oct 1, 2023

@Antonboom I only had a quick look, but yes, I think this is already covered by errchkjson.

@ldez ldez added declined and removed feedback required Requires additional feedback labels Oct 2, 2023
@ldez ldez closed this Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexandear alexandear alexandear left review comments

Assignees
No one assigned
Labels
declined enhancement New feature or improvement linter: new Support new linter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add "validjson" linter
5 participants
@leonklingele @ldez @Antonboom @breml @alexandear