cloudwego/thriftgo

Generated code can't pass staticcheck

Closed this issue · 4 comments

Description

thriftgo generated code can't pass staticcheck, so we must write an extra config file staticcheck.conf to ignore some checks, but staticcheck will ignore all golang code files under the same directory tree which the config file placed

Reproduction

assume we have a thrift file named 'foo.thrift' and below is the content

namespace golang foo

struct Foo {
    1: required string name,
    2: required i64 age,
}

execute shell commands below

go mod init code.byted.org/demo
thriftgo -g go -o ./ foo.thrift
go mod tidy
go mod edit -replace  github.com/apache/thrift=github.com/apache/thrift@v0.13.0
go mod tidy

staticcheck ./...

and will get the output

foo/foo.go:113:67: error strings should not be capitalized (ST1005)

Recommandation

  1. add //lint:ignore directive to generator/golang/templates/struct.go::StructLikeRead
RequiredFieldNotSetError:
	//lint:ignore ST1005 {some reasons}
	return thrift.NewTProtocolExceptionWithType(thrift.INVALID_DATA, fmt.Errorf("Required field %s is not set", fieldIDToName_ModelSync[fieldId]))
  1. use lowercase word instead
return thrift.NewTProtocolExceptionWithType(thrift.INVALID_DATA, fmt.Errorf("required field %s is not set", fieldIDToName_ModelSync[fieldId]))

Reference

lsjbd commented

That's awkward. Files generated by thriftgo all contains a header comment // Code generated by thriftgo (x.y.z). DO NOT EDIT. and staticcheck should ignore them as most linters do. Can you check if this is an issue of staticcheck ?

This is a known issue of staticcheck, but the author recommends 'generator' to fix the problems, and I also find a code review comment about error string: https://github.com/golang/go/wiki/CodeReviewComments#error-strings, so does this is a bad smell of thriftgo or there have some reasons to use capitalized in error string?

lsjbd commented

OK, we will fix the error string issue in the next version.

But after all, staticcheck does not follow the common way for ignoring generated files but asks all potential generators to meet its requirement. I think this is irrational.

And, DO NOT EDIT is recommended by the go officials. See golang/go#41196.

lsjbd commented

This issue has been solved in v0.1.4.