[errors] de-specialize Transient in favor of Tags.

common/errors/tags.go

Index: common/errors/tags.go
diff --git a/common/errors/tags.go b/common/errors/tags.go
new file mode 100644
index 0000000000000000000000000000000000000000..fc584bf80d950c71566808c2a947e6648266e57b
--- /dev/null
+++ b/common/errors/tags.go
@@ -0,0 +1,235 @@
+// Copyright 2017 The LUCI Authors. All rights reserved.

[iannucci, 2017/06/24 00:01:11]

This is where the bulk of the new logic lives.

+// Use of this source code is governed under the Apache License, Version 2.0
+// that can be found in the LICENSE file.
+
+package errors
+
+import (
+	"fmt"
+	"reflect"
+	"sync"
+)
+
+type (
+	tagKey int
+
+	// TagKey objects are used for doing map lookups in GetTags, as well as
+	// checking for the presence and/or value of a tag on a specific error.
+	//
+	// The return values of NewBoolTag and NewTagger should be used directly as
+	// TagKeys.
+	TagKey interface {
+		isTagKey()
+
+		// returns true if the error contains a tag with this key.
+		In(err error) bool
+
+		// returns the value associated with this tag, if this error is tagged with
+		// this key.
+		ValueIn(err error) (val interface{}, ok bool)
+	}
+
+	// TagValue represents a (tag, value) to be used with Annotate.Tag, or may be
+	// applied to an error directly with the Apply method.
+	TagValue interface {
+		getKey() tagKey
+		getValue() interface{}
+
+		// Applies this TagValue to the provided error. This is equivalent to
+		//   Annotate(err).Tag(tagValue).Err()
+		Apply(err error) error
+	}
+
+	tagValue struct {
+		key   tagKey
+		value interface{}
+	}
+
+	// BoolTag is both a TagKey and a TagValue (whose data value is nil). This is
+	// obtained from NewBoolTag.
+	BoolTag interface {

[dnj, 2017/06/24 14:53:55]

I'd:
1) Make the NewBoolTag method BoolTag.
2) Have it return interface{TagKey, TagValue}
3) Delete this interface.

I think the naming is better, and I don't think there is any good reason to
export this type, is there?

[iannucci, 2017/06/24 20:16:10]

Done.

+		TagKey
+		TagValue
+	}
+
+	// Tagger has a With method to mint TagValues with associated with its
+	// underlying tag. This is obtained from NewTagger.
+	Tagger interface {
+		TagKey
+		With(value interface{}) TagValue
+	}
+
+	tagDetail struct {
+		description   string
+		requiresValue bool
+		validate      func(v interface{})
+	}
+)
+
+var (
+	tagsLock sync.RWMutex
+	curTag   tagKey
+	tags     = map[tagKey]tagDetail{}
+)
+
+func getTagDetail(key tagKey) tagDetail {
+	tagsLock.RLock()
+	detail, ok := tags[key]
+	tagsLock.RUnlock()
+	if !ok {
+		panic("unknown tag")
+	}
+	return detail
+}
+
+// tagKey is actually secretly both a TagKey, a TagValue, and a Tagger. This is
+// done so that the tags map can be a very simple, direct, int lookup.
+//
+// Technically a user could cast a Tagger to a BoolTag, which would be strange,
+// however preventing this cast makes the whole thing much more complex.

[dnj, 2017/06/24 14:53:55]

(Unless you delete the BoolTag interface)

[iannucci, 2017/06/24 20:16:10]

They could still cast it to TagValue

+func (t tagKey) isTagKey()             {}
+func (t tagKey) getKey() tagKey        { return t }
+func (t tagKey) getValue() interface{} { return nil }
+func (t tagKey) String() string {
+	return fmt.Sprintf("errors.TagKey{%q}", getTagDetail(t).description)
+}
+
+func (t tagKey) Apply(err error) error {
+	detail := getTagDetail(t)
+	if detail.requiresValue {
+		panic(fmt.Sprintf("tag %q requires a value", detail.description))
+	}
+	if err == nil {
+		return nil
+	}
+	a := &Annotator{err, stackContext{FrameInfo: stackFrameInfoForError(1, err)}}

[dnj, 2017/06/24 14:53:54]

We're dong a full captureStack every time we apply a tag to an error? Should
tags have an associated stack? Applying several tags to an annotated error could
result in a lot of stack captures.

[iannucci, 2017/06/24 20:16:09]

If you apply it singly, then yes, if you apply many tags there's only a single
stack capture (using errors.Annotate.Tag(...))

+	return a.Tag(t).Err()
+}
+
+func (t tagKey) With(val interface{}) TagValue {

[dnj, 2017/06/24 14:53:55]

I don't like the binary "Apply" XOR "With" as an interface. Either split this
into two interfaces (one with value, one without) so we can statically verify
usage, or make no-value versions of these functions just treat the value as
"nil".

I'm extra concerned here b/c oftentimes obscure error generation paths are the
ones that aren't tested too much, and someone could get a random production
panic because they mess up something that could be compile-time checked.

[iannucci, 2017/06/24 20:16:10]

I actually don't understand this comment at all... What do you mean by XOR?

That said, I rewrote all of this to be type safe!! Go didn't make it easy, but I
did it :).

+	detail := getTagDetail(t)
+	if !detail.requiresValue {
+		panic(fmt.Sprintf("tag %q doesn't allow a value", detail.description))
+	}
+
+	switch k := reflect.ValueOf(val).Kind(); {
+	case k < reflect.Array || k == reflect.String:

[dnj, 2017/06/24 14:53:54]

I think it would be better to just enumerate the "reflect.*" types that *are*
allowed. A user getting a panic right now would have to pull up the "reflect"
godocs and read the enumeration values to understand this.

That or explicitly document allowed types in the "With" function.

[iannucci, 2017/06/24 20:16:09]

ALL OF THIS CODE IS GONE NOW!

+	default:
+		panic("Tagger.With(value) only takes simple types for `value`, not " + k.String())

[dnj, 2017/06/24 14:53:55]

Why? I get that you're trying to not have this be used as a data channel, but
really if someone wants to do something dumb like that, they're going to in some
way or another. Why is it your job to stop them? :)

Also concerned about loss of static checking. For example, someone has:

func whatever() error {
  v := thing() // Returns int
  result, err := otherThing()
  if err != nil {
    return errors.Annotate(err).Tag(MyTag.With(v))
  }
  return consumeThing(v)
}

Yay, this works! Except the owner of "thing" library changes the return type
from a simple type to a complex type. Code still compiles, but PANIC! This isn't
good, and errors are too primitive to open this door IMO.

[iannucci, 2017/06/24 20:16:10]

I was trying to prevent people from putting mutable data into an error so they
don't:

  value := []byte{...}
  error := ...Tag.With(value).Apply(err)
  // modify value here or elsewhere, probably not by pulling it out of the
error,
  // but because something else has a ref to value

But this is all gone now :)

+	}
+	if detail.validate != nil {

[dnj, 2017/06/24 14:53:55]

Similar concerns for validation, as either validation or the type being passed
in could change w/out any warning and make it into production code.

If we really want to inspect this, let's set up a situation where users can
declared typed tags:

type MyTag struct {
  errors.BasicTagger
  Value MyType
}

func (t *MyTag) With(val MyType) TagValue { ... }

[iannucci, 2017/06/24 20:16:10]

Done.

+		detail.validate(val)
+	}
+	return tagValue{t, val}
+}
+
+func (t tagKey) In(err error) bool {
+	_, ok := t.ValueIn(err)
+	return ok
+}
+
+// ValueIn implements TagKey.

[dnj, 2017/06/24 14:53:55]

Comment not needed, or comment other methods uniformly.

[iannucci, 2017/06/24 20:16:10]

Acknowledged.

+func (t tagKey) ValueIn(err error) (value interface{}, ok bool) {

[dnj, 2017/06/24 14:53:55]

Is it OK that this traverses MultiError? Probably, but just wanted to check.

[iannucci, 2017/06/24 20:16:09]

yes, I have a test for it too

+	Walk(err, func(err error) bool {
+		if sc, isSC := err.(stackContexter); isSC {
+			if value, ok = sc.stackContext().Tags[t]; ok {
+				return false
+			}
+		}
+		return true
+	})
+	return
+}
+
+func (t tagValue) getKey() tagKey        { return t.key }
+func (t tagValue) getValue() interface{} { return t.value }
+func (t tagValue) Apply(err error) error {
+	if err == nil {
+		return nil
+	}
+	a := &Annotator{err, stackContext{FrameInfo: stackFrameInfoForError(1, err)}}

[dnj, 2017/06/24 14:53:55]

(Same, full stack trace seems like a not great thing to do for each "Apply" if
one has already been taken)

[iannucci, 2017/06/24 20:16:10]

If you need to apply more than one tag, use the Annotator interface.

+	return a.Tag(t).Err()
+}
+
+// NewBoolTag creates a new tag with a fixed "nil" value.
+//
+// You should use this in the top level context of your package like:
+//   var myTag = errors.NewBoolTag("this error is a user error")
+//
+// You could then use it like:
+//   err = myTag.Apply(err)
+//   myTag.In(err) // == true
+//   myTag.ValueIn(err) // == (nil, true)
+func NewBoolTag(description string) BoolTag {

[dnj, 2017/06/24 14:53:55]

Note that there's nothing stopping someone from naively doing this within a
handler or loop or something else stupid and causing a memory leak.

I don't really have a good way to adapt this model to accommodate that, but it
seems possible enough that it's concerning to me.

[iannucci, 2017/06/24 20:16:10]

That's why I had "New" here. It would also mean that it would be impossible to
check to see if the error had the tag too, so I'm not too worried about it (as
in: they could apply the tag but not read it back).

+	tagsLock.Lock()
+	defer tagsLock.Unlock()
+
+	t := curTag + 1
+	curTag++
+	tags[t] = tagDetail{description: description}
+
+	return t
+}
+
+// NewTagger creates a new Tagger which can be added to errors with
+// a corresponding value.
+//
+// Values used with this tag are must be a simple type (i.e. has a reflect.Kind
+// which is a base data type like bool, string, or int).
+//
+// The `validate` function is an optional argument which allows you to further
+// validate values assocaited with this tag. The function should panic if the
+// validation fails.
+//
+// You should use this in the top level context of your package like:
+//   type CodeType int
+//   var CodeTag = errors.NewTagger(
+//     "has a response code", func(v interface) {
+//       v.(CodeType)  // panics if v is not CodeType
+//     })
+//
+// You could then use it like:
+//   err = CodeTag.With(CodeType(100)).Apply(err)
+//   CodeTag.In(err) // == true
+//   CodeTag.ValueIn(err) // == (CodeType(100), true)
+func NewTagger(description string, validate ...func(v interface{})) Tagger {
+	var valFn func(interface{})
+	switch len(validate) {
+	case 0:
+	case 1:
+		valFn = validate[0]
+	default:
+		panic("NewTagger takes exactly 0 or 1 validate function")
+	}
+
+	tagsLock.Lock()
+	defer tagsLock.Unlock()
+
+	t := curTag + 1
+	curTag++
+	tags[t] = tagDetail{description, true, valFn}
+
+	return t
+}
+
+// GetTags returns a map of all TagKeys set in this error to their value.
+//
+// A nil value means that the tag is present, but has a nil associated value.
+//
+// This is done in a depth-first traversal of the error stack, with the
+// most-recently-set value of the tag taking precendence.
+func GetTags(err error) map[TagKey]interface{} {
+	ret := map[TagKey]interface{}{}
+	Walk(err, func(err error) bool {
+		if sc, ok := err.(stackContexter); ok {
+			ctx := sc.stackContext()
+			for k, v := range ctx.Tags {
+				if _, ok := ret[k]; !ok {
+					ret[k] = v
+				}
+			}
+		}
+		return true
+	})
+	return ret
+}