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

proposal: cmd/vet: diagnose marshal of field of type error #45108

Open
timothy-king opened this issue Mar 18, 2021 · 2 comments
Open

proposal: cmd/vet: diagnose marshal of field of type error #45108

timothy-king opened this issue Mar 18, 2021 · 2 comments
Projects
Milestone

Comments

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Mar 18, 2021

This proposal is to add a new vet checker that warns when an exported error field of a struct is marshaled. Currently what will happen is that as error is an interface this will use the marshaling of the underlying type. error values created by errors.New() and fmt.Errorf() are currently implemented by structs with unexported string fields. The result is that the marshaled value is "{}".

This behavior is well defined, but many users expect to see the "err.Error()" value. These would consider their code buggy. So this plausibly satisfies vet's correctness criteria.

Similar issues: #26946, #41241, #26889, #23549.

Example:
https://play.golang.org/p/XHoSKtGwAUN

package main

import (
	"encoding/json"
	"fmt"
)
type Response struct {
	Err error `json:"error"`
}
func main() {
	v := &Response{Err: fmt.Errorf("oops")}
	b, err := json.Marshal(v)
	if err != nil {
		panic(err)
	}
	fmt.Printf("Marshaled value: %s\n", b)
}

Output:

Marshaled value: {"error":{}}

The current focus is on json.Marshal(), but could be extended to similar standard library functions.

To avoid spurious reports the proposed conditions to warn would be when:

  • there is a json.Marshal(e) expression,
  • we can locally identify a non-interface type T for e, and
  • marshaling T reaches an exported error field (follow pointers, struct sub-fields, map fields, etc.) without traversing an interface or a type with MarshalJSON() defined on it.

These reports can potentially be considered false positives when all of the underlying types of the error values stored in the field have an MarshalJSON function defined or have a well defined serialization: type MyErr string; func (MyErr) Error() string {...}.

This would not report flows through interface{} variables for functions wrapping json.Marshall(). Example of this pattern but not necessarily for error values here.

@gopherbot gopherbot added this to the Proposal milestone Mar 18, 2021
@gopherbot gopherbot added the Proposal label Mar 18, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 18, 2021
@ianlancetaylor ianlancetaylor changed the title proposal: cmd/vet checker to warn when marshaling an exported error field proposal: cmd/vet: checker to warn when marshaling an exported error field Mar 18, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Apr 7, 2021

The hard part here is false positives. It is technically fine to json.Marshal a field of type error when it is nil or when it contains a JSON-able error. If you only trigger when you know it's a non-JSON-able error, that would be a possibility. But that might have too many false negatives.

@rsc rsc changed the title proposal: cmd/vet: checker to warn when marshaling an exported error field proposal: cmd/vet: diagnose marshal of field of type error Apr 7, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Apr 7, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants