From 2c6284427485136a315fa3eec85f5c28a356930f Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 7 Oct 2016 17:13:10 -0700 Subject: [PATCH] Improve error reporting Report module name and variant and property names in errors. Change-Id: I747f840402497b2c92bf8c565d7c50af33e6ab0e --- bootstrap/command.go | 4 +- context.go | 87 +++++++++++++++++++++++++++----------------- module_ctx.go | 34 +++++++++++------ unpack.go | 10 ++--- unpack_test.go | 2 +- 5 files changed, 85 insertions(+), 52 deletions(-) diff --git a/bootstrap/command.go b/bootstrap/command.go index 636dcb0..70684ac 100644 --- a/bootstrap/command.go +++ b/bootstrap/command.go @@ -174,7 +174,9 @@ func fatalErrors(errs []error) { for _, err := range errs { switch err := err.(type) { - case *blueprint.Error: + case *blueprint.BlueprintError, + *blueprint.ModuleError, + *blueprint.PropertyError: fmt.Printf("%serror:%s %s\n", red, unred, err.Error()) default: fmt.Printf("%sinternal error:%s %s\n", red, unred, err) diff --git a/context.go b/context.go index 7ab28f9..c59b107 100644 --- a/context.go +++ b/context.go @@ -105,11 +105,37 @@ type Context struct { // An Error describes a problem that was encountered that is related to a // particular location in a Blueprints file. -type Error struct { +type BlueprintError struct { Err error // the error that occurred Pos scanner.Position // the relevant Blueprints file location } +// A ModuleError describes a problem that was encountered that is related to a +// particular module in a Blueprints file +type ModuleError struct { + BlueprintError + module *moduleInfo +} + +// A PropertyError describes a problem that was encountered that is related to a +// particular property in a Blueprints file +type PropertyError struct { + ModuleError + property string +} + +func (e *BlueprintError) Error() string { + return fmt.Sprintf("%s: %s", e.Pos, e.Err) +} + +func (e *ModuleError) Error() string { + return fmt.Sprintf("%s: %s: %s", e.Pos, e.module, e.Err) +} + +func (e *PropertyError) Error() string { + return fmt.Sprintf("%s: %s: %s: %s", e.Pos, e.module, e.property, e.Err) +} + type localBuildActions struct { variables []*localVariable rules []*localRule @@ -229,11 +255,6 @@ type mutatorInfo struct { parallel bool } -func (e *Error) Error() string { - - return fmt.Sprintf("%s: %s", e.Pos, e.Err) -} - // NewContext creates a new Context object. The created context initially has // no module or singleton factories registered, so the RegisterModuleFactory and // RegisterSingletonFactory methods must be called before it can do anything @@ -514,7 +535,7 @@ func (c *Context) parse(rootDir, filename string, r io.Reader, if len(errs) > 0 { for i, err := range errs { if parseErr, ok := err.(*parser.ParseError); ok { - err = &Error{ + err = &BlueprintError{ Err: parseErr.Err, Pos: parseErr.Pos, } @@ -787,7 +808,7 @@ func (c *Context) findBuildBlueprints(dir string, build []string, globPattern := filepath.Join(dir, file) matches, matchedDirs, err := pathtools.Glob(globPattern) if err != nil { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("%q: %s", globPattern, err.Error()), Pos: buildPos, }) @@ -795,7 +816,7 @@ func (c *Context) findBuildBlueprints(dir string, build []string, } if len(matches) == 0 { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("%q: not found", globPattern), Pos: buildPos, }) @@ -809,12 +830,12 @@ func (c *Context) findBuildBlueprints(dir string, build []string, if err != nil { errs = append(errs, err) } else if !exists { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("%q not found", foundBlueprints), }) continue } else if dir { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("%q is a directory", foundBlueprints), }) continue @@ -835,7 +856,7 @@ func (c *Context) findSubdirBlueprints(dir string, subdirs []string, subdirsPos globPattern := filepath.Join(dir, subdir) matches, matchedDirs, err := pathtools.Glob(globPattern) if err != nil { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("%q: %s", globPattern, err.Error()), Pos: subdirsPos, }) @@ -843,7 +864,7 @@ func (c *Context) findSubdirBlueprints(dir string, subdirs []string, subdirsPos } if len(matches) == 0 && !optional { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("%q: not found", globPattern), Pos: subdirsPos, }) @@ -916,7 +937,7 @@ func getLocalStringListFromScope(scope *parser.Scope, v string) ([]string, scann return ret, assignment.EqualsPos, nil case *parser.Bool, *parser.String: - return nil, scanner.Position{}, &Error{ + return nil, scanner.Position{}, &BlueprintError{ Err: fmt.Errorf("%q must be a list of strings", v), Pos: assignment.EqualsPos, } @@ -934,7 +955,7 @@ func getStringFromScope(scope *parser.Scope, v string) (string, scanner.Position case *parser.String: return value.Value, assignment.EqualsPos, nil case *parser.Bool, *parser.List: - return "", scanner.Position{}, &Error{ + return "", scanner.Position{}, &BlueprintError{ Err: fmt.Errorf("%q must be a string", v), Pos: assignment.EqualsPos, } @@ -1050,7 +1071,7 @@ func (c *Context) convertDepsToVariation(module *moduleInfo, } } if newDep == nil { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("failed to find variation %q for module %q needed by %q", variationName, dep.module.properties.Name, module.properties.Name), Pos: module.pos, @@ -1085,7 +1106,7 @@ func (c *Context) processModuleDef(moduleDef *parser.Module, } return nil, []error{ - &Error{ + &BlueprintError{ Err: fmt.Errorf("unrecognized module type %q", moduleDef.Type), Pos: moduleDef.TypePos, }, @@ -1126,11 +1147,11 @@ func (c *Context) addModule(module *moduleInfo) []error { if group, present := c.moduleGroups[name]; present { return []error{ - &Error{ + &BlueprintError{ Err: fmt.Errorf("module %q already defined", name), Pos: module.pos, }, - &Error{ + &BlueprintError{ Err: fmt.Errorf("<-- previous definition here"), Pos: group.modules[0].pos, }, @@ -1223,7 +1244,7 @@ func (c *Context) findMatchingVariant(module *moduleInfo, group *moduleGroup) *m func (c *Context) addDependency(module *moduleInfo, tag DependencyTag, depName string) []error { if depName == module.properties.Name { - return []error{&Error{ + return []error{&BlueprintError{ Err: fmt.Errorf("%q depends on itself", depName), Pos: module.pos, }} @@ -1235,7 +1256,7 @@ func (c *Context) addDependency(module *moduleInfo, tag DependencyTag, depName s module.missingDeps = append(module.missingDeps, depName) return nil } - return []error{&Error{ + return []error{&BlueprintError{ Err: fmt.Errorf("%q depends on undefined module %q", module.properties.Name, depName), Pos: module.pos, @@ -1254,7 +1275,7 @@ func (c *Context) addDependency(module *moduleInfo, tag DependencyTag, depName s return nil } - return []error{&Error{ + return []error{&BlueprintError{ Err: fmt.Errorf("dependency %q of %q missing variant %q", depGroup.modules[0].properties.Name, module.properties.Name, c.prettyPrintVariant(module.dependencyVariant)), @@ -1264,7 +1285,7 @@ func (c *Context) addDependency(module *moduleInfo, tag DependencyTag, depName s func (c *Context) findReverseDependency(module *moduleInfo, destName string) (*moduleInfo, []error) { if destName == module.properties.Name { - return nil, []error{&Error{ + return nil, []error{&BlueprintError{ Err: fmt.Errorf("%q depends on itself", destName), Pos: module.pos, }} @@ -1272,7 +1293,7 @@ func (c *Context) findReverseDependency(module *moduleInfo, destName string) (*m destInfo, ok := c.moduleGroups[destName] if !ok { - return nil, []error{&Error{ + return nil, []error{&BlueprintError{ Err: fmt.Errorf("%q has a reverse dependency on undefined module %q", module.properties.Name, destName), Pos: module.pos, @@ -1283,7 +1304,7 @@ func (c *Context) findReverseDependency(module *moduleInfo, destName string) (*m return m, nil } - return nil, []error{&Error{ + return nil, []error{&BlueprintError{ Err: fmt.Errorf("reverse dependency %q of %q missing variant %q", destName, module.properties.Name, c.prettyPrintVariant(module.dependencyVariant)), @@ -1300,7 +1321,7 @@ func (c *Context) addVariationDependency(module *moduleInfo, variations []Variat module.missingDeps = append(module.missingDeps, depName) return nil } - return []error{&Error{ + return []error{&BlueprintError{ Err: fmt.Errorf("%q depends on undefined module %q", module.properties.Name, depName), Pos: module.pos, @@ -1329,7 +1350,7 @@ func (c *Context) addVariationDependency(module *moduleInfo, variations []Variat } if found { if module == m { - return []error{&Error{ + return []error{&BlueprintError{ Err: fmt.Errorf("%q depends on itself", depName), Pos: module.pos, }} @@ -1338,7 +1359,7 @@ func (c *Context) addVariationDependency(module *moduleInfo, variations []Variat // that module is earlier in the module list than this one, since we always // run GenerateBuildActions in order for the variants of a module if depGroup == module.group && beforeInModuleList(module, m, module.group.modules) { - return []error{&Error{ + return []error{&BlueprintError{ Err: fmt.Errorf("%q depends on later version of itself", depName), Pos: module.pos, }} @@ -1349,7 +1370,7 @@ func (c *Context) addVariationDependency(module *moduleInfo, variations []Variat } } - return []error{&Error{ + return []error{&BlueprintError{ Err: fmt.Errorf("dependency %q of %q missing variant %q", depGroup.modules[0].properties.Name, module.properties.Name, c.prettyPrintVariant(newVariant)), @@ -1498,7 +1519,7 @@ func (c *Context) updateDependencies() (errs []error) { // for generating the errors. The cycle list is in // reverse order because all the 'check' calls append // their own module to the list. - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("encountered dependency cycle:"), Pos: cycle[len(cycle)-1].pos, }) @@ -1507,7 +1528,7 @@ func (c *Context) updateDependencies() (errs []error) { curModule := cycle[0] for i := len(cycle) - 1; i >= 0; i-- { nextModule := cycle[i] - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf(" %q depends on %q", curModule.properties.Name, nextModule.properties.Name), @@ -1975,7 +1996,7 @@ func (c *Context) generateModuleBuildActions(config interface{}, if module.missingDeps != nil && !mctx.handledMissingDeps { var errs []error for _, depName := range module.missingDeps { - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("%q depends on undefined module %q", module.properties.Name, depName), Pos: module.pos, @@ -2446,7 +2467,7 @@ func (c *Context) ModuleErrorf(logicModule Module, format string, args ...interface{}) error { module := c.moduleInfo[logicModule] - return &Error{ + return &BlueprintError{ Err: fmt.Errorf(format, args...), Pos: module.pos, } diff --git a/module_ctx.go b/module_ctx.go index 88bb7eb..22896c2 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -193,7 +193,7 @@ func (d *baseModuleContext) error(err error) { func (d *baseModuleContext) Errorf(pos scanner.Position, format string, args ...interface{}) { - d.error(&Error{ + d.error(&BlueprintError{ Err: fmt.Errorf(format, args...), Pos: pos, }) @@ -202,9 +202,12 @@ func (d *baseModuleContext) Errorf(pos scanner.Position, func (d *baseModuleContext) ModuleErrorf(format string, args ...interface{}) { - d.error(&Error{ - Err: fmt.Errorf(format, args...), - Pos: d.module.pos, + d.error(&ModuleError{ + BlueprintError: BlueprintError{ + Err: fmt.Errorf(format, args...), + Pos: d.module.pos, + }, + module: d.module, }) } @@ -217,11 +220,15 @@ func (d *baseModuleContext) PropertyErrorf(property, format string, pos = d.module.pos } - format = property + ": " + format - - d.error(&Error{ - Err: fmt.Errorf(format, args...), - Pos: pos, + d.error(&PropertyError{ + ModuleError: ModuleError{ + BlueprintError: BlueprintError{ + Err: fmt.Errorf(format, args...), + Pos: pos, + }, + module: d.module, + }, + property: property, }) } @@ -248,9 +255,12 @@ func (m *baseModuleContext) OtherModuleErrorf(logicModule Module, format string, args ...interface{}) { module := m.context.moduleInfo[logicModule] - m.errs = append(m.errs, &Error{ - Err: fmt.Errorf(format, args...), - Pos: module.pos, + m.errs = append(m.errs, &ModuleError{ + BlueprintError: BlueprintError{ + Err: fmt.Errorf(format, args...), + Pos: module.pos, + }, + module: module, }) } diff --git a/unpack.go b/unpack.go index babaf07..48d5105 100644 --- a/unpack.go +++ b/unpack.go @@ -63,7 +63,7 @@ func unpackProperties(propertyDefs []*parser.Property, for name, packedProperty := range propertyMap { result[name] = packedProperty.property if !packedProperty.unpacked { - err := &Error{ + err := &BlueprintError{ Err: fmt.Errorf("unrecognized property %q", name), Pos: packedProperty.property.ColonPos, } @@ -88,11 +88,11 @@ func buildPropertyMap(namePrefix string, propertyDefs []*parser.Property, // We've already added this property. continue } - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("property %q already defined", name), Pos: propertyDef.ColonPos, }) - errs = append(errs, &Error{ + errs = append(errs, &BlueprintError{ Err: fmt.Errorf("<-- previous definition here"), Pos: first.property.ColonPos, }) @@ -216,7 +216,7 @@ func unpackStructValue(namePrefix string, structValue reflect.Value, if proptools.HasTag(field, "blueprint", "mutated") { errs = append(errs, - &Error{ + &BlueprintError{ Err: fmt.Errorf("mutated field %s cannot be set in a Blueprint file", propertyName), Pos: packedProperty.property.ColonPos, }) @@ -228,7 +228,7 @@ func unpackStructValue(namePrefix string, structValue reflect.Value, if filterKey != "" && !proptools.HasTag(field, filterKey, filterValue) { errs = append(errs, - &Error{ + &BlueprintError{ Err: fmt.Errorf("filtered field %s cannot be set in a Blueprint file", propertyName), Pos: packedProperty.property.ColonPos, }) diff --git a/unpack_test.go b/unpack_test.go index ea4dbc7..f470356 100644 --- a/unpack_test.go +++ b/unpack_test.go @@ -244,7 +244,7 @@ var validUnpackTestCases = []struct { }, }, errs: []error{ - &Error{ + &BlueprintError{ Err: fmt.Errorf("filtered field nested.foo cannot be set in a Blueprint file"), Pos: mkpos(30, 4, 9), },