serum-errors / go-serum-analyzer

Other
7 stars 2 forks source link

fixes go.18+ compatibility #5

Closed TripleDogDare closed 1 year ago

TripleDogDare commented 1 year ago

go1.18 changes subtle things about the type system. Most likely do to the addition of generics. The serum analyzer is a casualty of that. This should fix the immediate problems although it's not entirely clear to me why or if there's another option for handling it.

Testing

go1.18 test ./... on master will result in errors for Cause functions that should be ignored. Example:

    analysistest.go:446: 001/main.go:220:1: unexpected diagnostic: function "Cause" is exported, but does not declare any error codes

Run the tests with both go1.17 AND go1.18+ to see the difference on both master and this branch. The master branch still has other errors that may be unimplemented features (use a flag/skip?) or may need to be fixed. This PR does not address that issue. Here's the expected output on master for go1.17

$ go1.17 test ./...
--- FAIL: TestVerifyAnalyzer (1.19s)
    analysistest.go:446: 001/main.go:153:1: unexpected diagnostic: function "DereferenceAssignment" has a mismatch of declared and actual error codes: unused codes: [other-error]
    analysistest.go:446: 001/main.go:164:1: unexpected diagnostic: function "DereferenceAssignment2" has a mismatch of declared and actual error codes: unused codes: [other-error]
    analysistest.go:446: multifile/file1.go:18:9: unexpected diagnostic: function "StringError" in dot-imported package does not declare error codes
    analysistest.go:446: multifile/file1.go:17:1: unexpected diagnostic: function "DoString" has a mismatch of declared and actual error codes: unused codes: [string-error]
    analysistest.go:446: multipackage/multipackage.go:139:9: unexpected diagnostic: function "StringError" in package "inner1" does not declare error codes
    analysistest.go:446: multipackage/multipackage.go:138:1: unexpected diagnostic: function "DoString" has a mismatch of declared and actual error codes: unused codes: [string-error]
FAIL
FAIL    github.com/serum-errors/go-serum-analyzer/analysis  1.203s
ok      github.com/serum-errors/go-serum-analyzer/analysis/scc  (cached)
?       github.com/serum-errors/go-serum-analyzer/cmd/go-serum-analyzer [no test files]
FAIL
warpfork commented 1 year ago

Interesting. I confirm that this patch fixes problems relating to failure to recognize the "error with cause" type. And I also do not entirely understand why. :sweat_smile:

It's changing the pattern of the "error with cause" interface to expect the "Cause" method to match the universally pre-declared error type instead of merely matching anything interface { Error() string } (which is functionally equivalent).

I don't understand why the latter worked in Go before 1.17 and wouldn't in 1.18.

That said, I don't think this change removes any meaningful functionality or flexibility, and it evidently solves problems, and therefore it seems reasonable to merge it.