maxmustermann/pulumi
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Alter diag.Message to discourage format mistakes

Browse source 
This change alters diag.Message to not format strings and, instead,
encourages developers to use the Infof, Errorf, and Warningf varargs
functions.  It also tests that arguments are never interepreted as
format strings.
This commit is contained in:
joeduffy 2017-06-02 18:35:53 -07:00
parent 52b2e62157
commit f552832a7a
5 changed files with 48 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
pkg/diag/diag.go
View file

@ -15,10 +15,6 @@
package diag
import (
"fmt"
)
// ID is a unique diagnostics identifier.
type ID int
@ -31,8 +27,8 @@ type Diag struct {
}
// Message returns an anonymous diagnostic message without any source or ID information.
func Message(msg string, args ...interface{}) *Diag {
return &Diag{Message: fmt.Sprintf(msg, args...)}
func Message(msg string) *Diag {
return &Diag{Message: msg}
}
// Diagable can be used to determine a diagnostic's position.

27
pkg/diag/sink_test.go
View file

@ -23,15 +23,19 @@ import (
"github.com/stretchr/testify/assert"
)
func TestCounts(t *testing.T) {
t.Parallel()
func discardSink() Sink {
// Create a new default sink with /dev/null writers to avoid spamming the test log.
sink := newDefaultSink(FormatOptions{}, map[Category]io.Writer{
return newDefaultSink(FormatOptions{}, map[Category]io.Writer{
Info: ioutil.Discard,
Error: ioutil.Discard,
Warning: ioutil.Discard,
})
}
func TestCounts(t *testing.T) {
t.Parallel()
sink := discardSink()
const numEach = 10
@ -55,3 +59,18 @@ func TestCounts(t *testing.T) {
assert.Equal(t, sink.Warnings(), numEach, "expected warnings post to stay at numEach")
}
}
// TestEscape ensures that arguments containing format-like characters aren't interpreted as such.
func TestEscape(t *testing.T) {
t.Parallel()
sink := discardSink()
// Passing % chars in the argument should not yield %!(MISSING)s.
s := sink.Stringify(Message("%s"), Error, "lots of %v %s %d chars")
assert.Equal(t, "error: lots of %v %s %d chars\n", s)
// Passing % chars in the format string, on the other hand, should.
smiss := sink.Stringify(Message("lots of %v %s %d chars"), Error)
assert.Equal(t, "error: lots of %!v(MISSING) %!s(MISSING) %!d(MISSING) chars\n", smiss)
}

4
pkg/resource/plugin.go
View file

@ -72,8 +72,8 @@ func newPlugin(ctx *Context, bins []string, prefix string) (*plugin, error) {
lbl string
cb func(string)
}{
plug.Stderr: {"stderr", func(line string) { ctx.Diag.Errorf(diag.Message(line)) }},
plug.Stdout: {"stdout", func(line string) { ctx.Diag.Infof(diag.Message(line)) }},
plug.Stderr: {"stderr", func(line string) { ctx.Diag.Errorf(diag.Message("%s"), line) }},
plug.Stdout: {"stdout", func(line string) { ctx.Diag.Infof(diag.Message("%s"), line) }},
}
runtrace := func(t io.Reader) {
ts := tracers[t]

38
pkg/tools/lumidl/check.go
View file

@ -71,8 +71,8 @@ func (chk *Checker) Check(name tokens.PackageName, pkginfo *loader.PackageInfo)
default:
ok = false
cmdutil.Sink().Errorf(
diag.Message("%v is an unrecognized Go declaration type: %v",
objname, reflect.TypeOf(obj)).At(chk.diag(obj)))
diag.Message("%v is an unrecognized Go declaration type: %v").At(chk.diag(obj)),
objname, reflect.TypeOf(obj))
}
}
@ -175,7 +175,9 @@ func (chk *Checker) CheckConst(c *types.Const, file *File, decl ast.Decl) (*Cons
chk.EnumValues[t] = append(chk.EnumValues[t], c.Val().String())
} else {
cmdutil.Sink().Errorf(
diag.Message("enums must be string-backed; %v has type %v", c, named).At(chk.diag(decl)))
diag.Message("enums must be string-backed; %v has type %v").At(chk.diag(decl)),
c, named,
)
}
} else {
cmdutil.Sink().Errorf(
@ -224,9 +226,9 @@ func (chk *Checker) CheckType(t *types.TypeName, file *File, decl ast.Decl) (Mem
}, true
}
cmdutil.Sink().Errorf(
diag.Message("type alias %v is not a valid IDL alias type (must be bool, float64, or string)",
t.Name()).At(chk.diag(decl)))
cmdutil.Sink().Errorf(diag.Message(
"type alias %v is not a valid IDL alias type (must be bool, float64, or string)").At(
chk.diag(decl)))
case *types.Map, *types.Slice:
return &Alias{
member: memb,
@ -257,11 +259,11 @@ func (chk *Checker) CheckType(t *types.TypeName, file *File, decl ast.Decl) (Mem
contract.Assert(!cmdutil.Sink().Success())
default:
cmdutil.Sink().Errorf(
diag.Message("%v is an illegal underlying type: %v", s, reflect.TypeOf(s)).At(chk.diag(decl)))
diag.Message("%v is an illegal underlying type: %v").At(chk.diag(decl)), s, reflect.TypeOf(s))
}
default:
cmdutil.Sink().Errorf(
diag.Message("%v is an illegal Go type kind: %v", t.Name(), reflect.TypeOf(typ)).At(chk.diag(decl)))
diag.Message("%v is an illegal Go type kind: %v").At(chk.diag(decl)), t.Name(), reflect.TypeOf(typ))
}
return nil, false
}
@ -292,32 +294,32 @@ func (chk *Checker) CheckStructFields(t *types.TypeName, s *types.Struct,
if opts.Name == "" {
ok = false
cmdutil.Sink().Errorf(
diag.Message("field %v.%v is missing a `lumi:\"<name>\"` tag directive",
t.Name(), fld.Name()).At(chk.diag(fld)))
diag.Message("field %v.%v is missing a `lumi:\"<name>\"` tag directive").At(chk.diag(fld)),
t.Name(), fld.Name())
}
if opts.Out && !isres {
ok = false
cmdutil.Sink().Errorf(
diag.Message("field %v.%v is marked `out` but is not a resource property",
t.Name(), fld.Name()).At(chk.diag(fld)))
diag.Message("field %v.%v is marked `out` but is not a resource property").At(chk.diag(fld)),
t.Name(), fld.Name())
}
if opts.Replaces && !isres {
ok = false
cmdutil.Sink().Errorf(
diag.Message("field %v.%v is marked `replaces` but is not a resource property",
t.Name(), fld.Name()).At(chk.diag(fld)))
diag.Message("field %v.%v is marked `replaces` but is not a resource property").At(chk.diag(fld)),
t.Name(), fld.Name())
}
if _, isptr := fld.Type().(*types.Pointer); !isptr && opts.Optional {
ok = false
cmdutil.Sink().Errorf(
diag.Message("field %v.%v is marked `optional` but is not a pointer in the IDL",
t.Name(), fld.Name()).At(chk.diag(fld)))
diag.Message("field %v.%v is marked `optional` but is not a pointer in the IDL").At(chk.diag(fld)),
t.Name(), fld.Name())
}
if err := chk.CheckIDLType(fld.Type(), opts); err != nil {
ok = false
cmdutil.Sink().Errorf(
diag.Message("field %v.%v is an not a legal IDL type: %v",
t.Name(), fld.Name(), err).At(chk.diag(fld)))
diag.Message("field %v.%v is an not a legal IDL type: %v").At(chk.diag(fld)),
t.Name(), fld.Name(), err)
}
}
}

2
pkg/tools/lumidl/options.go
View file

@ -53,7 +53,7 @@ func ParsePropertyOptions(tag string) PropertyOptions {
case "out":
opts.Out = true
default:
cmdutil.Sink().Errorf(diag.Message("unrecognized tag `lumi:\"%v\"`", key))
cmdutil.Sink().Errorf(diag.Message("unrecognized tag `lumi:\"%v\"`"), key)
}
}
}