Issue 167420043: code review 167420043: go.tools/oracle: add whicherrs query mode (Closed)
Descriptionx/tools/oracle: add whicherrs query mode
The whicherrs query mode takes the position of an error and returns the set of constants, globals and types visible from within the scope of the error being queried.
It is meant to be used as a shortcut to find out which errors should be handled for a given functions call.
Patch Set 1 #Patch Set 2 : diff -r 6fc790e5bfa623b690e2ca62c1f64370d86a874e https://code.google.com/p/go.tools #Patch Set 3 : diff -r 6fc790e5bfa623b690e2ca62c1f64370d86a874e https://code.google.com/p/go.tools #Patch Set 4 : diff -r 7ff397840258469cc590e818fa09ccc1819cf5f1 https://code.google.com/p/go.tools #
Total comments: 61
Patch Set 5 : diff -r a81b057fa6e0681031c81674e00ad6bbb9f2e18f https://code.google.com/p/go.tools #Patch Set 6 : diff -r a81b057fa6e0681031c81674e00ad6bbb9f2e18f https://code.google.com/p/go.tools #
Total comments: 12
Patch Set 7 : diff -r a81b057fa6e0681031c81674e00ad6bbb9f2e18f https://code.google.com/p/go.tools #Patch Set 8 : diff -r a81b057fa6e0681031c81674e00ad6bbb9f2e18f https://code.google.com/p/go.tools #Patch Set 9 : diff -r a81b057fa6e0681031c81674e00ad6bbb9f2e18f https://code.google.com/p/go.tools #
MessagesTotal messages: 12
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.tools
Sign in to reply to this message.
Couldn't this be generalized to work with all interfaces, not just the error interface?
Sign in to reply to this message.
On 2014/11/10 07:18:12, Dominik Honnef wrote: > Couldn't this be generalized to work with all interfaces, not just the error > interface? The pointsto query already does this. The main difference between this mode and pointsto is that this mode filters the output down to just the things you care about for handling errors.
Sign in to reply to this message.
forgot to CC adonovan.
Sign in to reply to this message.
On 2014/11/12 14:26:37, DMorsing wrote: > forgot to CC adonovan. Ping.
Sign in to reply to this message.
Hi Daniel, this looks great! Thanks for contributing it, and sorry it took me so long to look at it. I wonder if it would be useful to also report the set of calls to fmt.Errorf or errors.New that could end up in the selected value? (It would require making the PTA context-sensitive for these calls.) https://codereview.appspot.com/167420043/diff/50001/oracle/serial/serial.go File oracle/serial/serial.go (right): https://codereview.appspot.com/167420043/diff/50001/oracle/serial/serial.go#n... oracle/serial/serial.go:232: type WhichErrs struct { docstring https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... File oracle/testdata/src/main/whicherrs.go (right): https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:3: const cErr errCtype = "blah" const constErr errType = "blah" https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:5: type errCtype string Declare this first. Call it errType https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:11: var someerr error = errCtype("foo") Call this errVar. Perhaps we should also test: var errVar2 errType = "foo" i.e. the global var has a concrete type. https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:13: var i int // make sure function can return all kinds of errors Make this a parameter of genErr. (Don't worry, the analysis isn't smart enough to do interprocedural constant propagation.) https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go File oracle/whicherrs.go (right): https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:20: var errtype = types.Universe.Lookup("error").Type() var builtinErrorType https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:53: errtype := types.Universe.Lookup("error").Type() delete https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:54: if typ != errtype { if !types.Interface(typ, builtinErrorType) { (Don't use !=, since users can declare interface{Error()string} if they want, perhaps as part of a narrower error interface.) https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:55: return nil, fmt.Errorf("expression is not an error: %s", expr) ast.Exprs don't print nicely. Just say: fmt.Errorf("selection is not an expression of type 'error'") https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:77: globals: make([]*ssa.Global, 0), delete x 3. Don't bother making empty slices. (And FYI, this is shorter: []*ssa.Global{}) https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:82: // find the instruction which inited the "Find"... "initialized" https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:144: if concs := pts.DynamicTypes(); concs.Len() > 0 { You don't need the Len()>0 check. Iterate works on empty maps. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:145: concs.Iterate(func(conc types.Type, pta interface{}) { s/pta/_/ https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:146: // go/types is a bit annoying here. Yes, it is a little awkward. Within go/types, the operation below is called deref(), but outside it, the function called deref() strips off a Pointer constructor, even if it is beneath the Named constructor, which is nearly always what clients want. Except here. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:154: // by unnamed types, but in that case, we can't assert to Actually you can type-assert to an unnamed type, but I agree we don't really care about them. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:170: if !name.Exported() && name.Pkg() != qpos.info.Pkg { isAccessibleFrom https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:182: // Finds a list of package level variables which are errors and visible within // findVisibleErrs returns a mapping from each package-level variable of type "error" to nil. (Needn't mention scope; that restriction impliclity applies to everything.) https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:187: for _, member := range pkg.Members { for _, mem https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:194: gbltype = gbltype.(*types.Pointer).Elem() delete https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:195: if gbltype != errtype { if !types.IsIdentical(deref(gbltype), builtinErrorType) { But: consider globals of concrete types assignable to 'error' too? https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:208: // Finds a list of package level constants which are errors and visible within // findVisibleConsts returns a mapping from each package-level constant assignable to type "error", to nil. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:213: for _, member := range pkg.Members { for _, mem https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:214: consts, ok := member.(*ssa.NamedConst) obj, ok := https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:219: if !types.Implements(consttype, errtype.Underlying().(*types.Interface)) { if !types.Implements(obj.Type(), builtinErrorType) https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:222: if !consts.Object().Exported() && qpos.info.Pkg != consts.Package().Object { if !isAccessibleFrom(obj, qpos.info.Pkg) { Could you also make describe.go:537 use isAccessibleFrom? Thanks. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:232: type sortGlobals []*ssa.Global type membersByPosAndString []ssa.Member Then you can use the same slice type for both res.globals and res.consts. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:237: return cmp < 0 || (cmp == 0 && a[i].String() < a[j].String()) redundant parens x 3 Use a linebreak after || if you like. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:259: type etype struct { errorType https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:260: typ types.Type // a concrete type N or *N that implements error https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:261: name *types.TypeName // the named type N https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:261: name *types.TypeName obj *types.TypeName https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:274: printf(r.qpos, "error may point to these globals:") "this error" x 3 https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:286: printf(r.qpos, "error may contain these types:") "these dynamic types"
Sign in to reply to this message.
On 2014/12/04 17:22:45, adonovan wrote: > Hi Daniel, this looks great! Thanks for contributing it, and sorry it took me > so long to look at it. > > I wonder if it would be useful to also report the set of calls to fmt.Errorf or > errors.New that could end up in the selected value? (It would require making > the PTA context-sensitive for these calls.) > > https://codereview.appspot.com/167420043/diff/50001/oracle/serial/serial.go > File oracle/serial/serial.go (right): > > https://codereview.appspot.com/167420043/diff/50001/oracle/serial/serial.go#n... > oracle/serial/serial.go:232: type WhichErrs struct { > docstring > > https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... > File oracle/testdata/src/main/whicherrs.go (right): > > https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... > oracle/testdata/src/main/whicherrs.go:3: const cErr errCtype = "blah" > const constErr errType = "blah" > > https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... > oracle/testdata/src/main/whicherrs.go:5: type errCtype string > Declare this first. Call it errType > > https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... > oracle/testdata/src/main/whicherrs.go:11: var someerr error = errCtype("foo") > Call this errVar. > > Perhaps we should also test: > var errVar2 errType = "foo" > i.e. the global var has a concrete type. > > https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... > oracle/testdata/src/main/whicherrs.go:13: var i int // make sure function can > return all kinds of errors > Make this a parameter of genErr. (Don't worry, the analysis isn't smart enough > to do interprocedural constant propagation.) > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go > File oracle/whicherrs.go (right): > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:20: var errtype = types.Universe.Lookup("error").Type() > var builtinErrorType > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:53: errtype := types.Universe.Lookup("error").Type() > delete > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:54: if typ != errtype { > if !types.Interface(typ, builtinErrorType) { > > (Don't use !=, since users can declare interface{Error()string} if they want, > perhaps as part of a narrower error interface.) > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:55: return nil, fmt.Errorf("expression is not an error: %s", > expr) > ast.Exprs don't print nicely. Just say: > > fmt.Errorf("selection is not an expression of type 'error'") > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:77: globals: make([]*ssa.Global, 0), > delete x 3. > Don't bother making empty slices. > > (And FYI, this is shorter: []*ssa.Global{}) > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:82: // find the instruction which inited the > "Find"... "initialized" > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:144: if concs := pts.DynamicTypes(); concs.Len() > 0 { > You don't need the Len()>0 check. Iterate works on empty maps. > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:145: concs.Iterate(func(conc types.Type, pta interface{}) { > s/pta/_/ > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:146: // go/types is a bit annoying here. > Yes, it is a little awkward. Within go/types, the operation below is called > deref(), but outside it, the function called deref() strips off a Pointer > constructor, even if it is beneath the Named constructor, which is nearly always > what clients want. Except here. > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:154: // by unnamed types, but in that case, we can't assert > to > Actually you can type-assert to an unnamed type, but I agree we don't really > care about them. > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:170: if !name.Exported() && name.Pkg() != qpos.info.Pkg { > isAccessibleFrom > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:182: // Finds a list of package level variables which are > errors and visible within > // findVisibleErrs returns a mapping from each package-level variable of type > "error" to nil. > > (Needn't mention scope; that restriction impliclity applies to everything.) > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:187: for _, member := range pkg.Members { > for _, mem > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:194: gbltype = gbltype.(*types.Pointer).Elem() > delete > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:195: if gbltype != errtype { > if !types.IsIdentical(deref(gbltype), builtinErrorType) { > > But: consider globals of concrete types assignable to 'error' too? > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:208: // Finds a list of package level constants which are > errors and visible within > // findVisibleConsts returns a mapping from each package-level constant > assignable to type "error", to nil. > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:213: for _, member := range pkg.Members { > for _, mem > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:214: consts, ok := member.(*ssa.NamedConst) > obj, ok := > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:219: if !types.Implements(consttype, > errtype.Underlying().(*types.Interface)) { > if !types.Implements(obj.Type(), builtinErrorType) > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:222: if !consts.Object().Exported() && qpos.info.Pkg != > consts.Package().Object { > if !isAccessibleFrom(obj, qpos.info.Pkg) { > > Could you also make describe.go:537 use isAccessibleFrom? Thanks. > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:232: type sortGlobals []*ssa.Global > type membersByPosAndString []ssa.Member > > Then you can use the same slice type for both res.globals and res.consts. > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:237: return cmp < 0 || (cmp == 0 && a[i].String() < > a[j].String()) > redundant parens x 3 > Use a linebreak after || if you like. > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:259: type etype struct { > errorType > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:260: typ types.Type > // a concrete type N or *N that implements error > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:261: name *types.TypeName > // the named type N > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:261: name *types.TypeName > obj *types.TypeName > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:274: printf(r.qpos, "error may point to these globals:") > "this error" x 3 > > https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... > oracle/whicherrs.go:286: printf(r.qpos, "error may contain these types:") > "these dynamic types" P.S. please add this to the oracle.el file: (defun go-oracle-whicherrs () "Show globals, constants and types to which the selected expression (of type 'error') may refer." (interactive) (go-oracle--run "whicherrs"))
Sign in to reply to this message.
PTAL https://codereview.appspot.com/167420043/diff/50001/oracle/serial/serial.go File oracle/serial/serial.go (right): https://codereview.appspot.com/167420043/diff/50001/oracle/serial/serial.go#n... oracle/serial/serial.go:232: type WhichErrs struct { On 2014/12/04 17:22:43, adonovan wrote: > docstring Done. https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... File oracle/testdata/src/main/whicherrs.go (right): https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:3: const cErr errCtype = "blah" On 2014/12/04 17:22:43, adonovan wrote: > const constErr errType = "blah" Done. https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:5: type errCtype string On 2014/12/04 17:22:43, adonovan wrote: > Declare this first. Call it errType Done. https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:11: var someerr error = errCtype("foo") On 2014/12/04 17:22:43, adonovan wrote: > Call this errVar. > > Perhaps we should also test: > var errVar2 errType = "foo" > i.e. the global var has a concrete type. Done. https://codereview.appspot.com/167420043/diff/50001/oracle/testdata/src/main/... oracle/testdata/src/main/whicherrs.go:13: var i int // make sure function can return all kinds of errors On 2014/12/04 17:22:43, adonovan wrote: > Make this a parameter of genErr. (Don't worry, the analysis isn't smart enough > to do interprocedural constant propagation.) Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go File oracle/whicherrs.go (right): https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:20: var errtype = types.Universe.Lookup("error").Type() On 2014/12/04 17:22:43, adonovan wrote: > var builtinErrorType Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:53: errtype := types.Universe.Lookup("error").Type() On 2014/12/04 17:22:44, adonovan wrote: > delete Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:54: if typ != errtype { On 2014/12/04 17:22:43, adonovan wrote: > if !types.Interface(typ, builtinErrorType) { > > (Don't use !=, since users can declare interface{Error()string} if they want, > perhaps as part of a narrower error interface.) Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:55: return nil, fmt.Errorf("expression is not an error: %s", expr) On 2014/12/04 17:22:44, adonovan wrote: > ast.Exprs don't print nicely. Just say: > > fmt.Errorf("selection is not an expression of type 'error'") Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:77: globals: make([]*ssa.Global, 0), On 2014/12/04 17:22:44, adonovan wrote: > delete x 3. > Don't bother making empty slices. > > (And FYI, this is shorter: []*ssa.Global{}) Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:82: // find the instruction which inited the On 2014/12/04 17:22:43, adonovan wrote: > "Find"... "initialized" Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:144: if concs := pts.DynamicTypes(); concs.Len() > 0 { On 2014/12/04 17:22:44, adonovan wrote: > You don't need the Len()>0 check. Iterate works on empty maps. Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:145: concs.Iterate(func(conc types.Type, pta interface{}) { On 2014/12/04 17:22:44, adonovan wrote: > s/pta/_/ Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:170: if !name.Exported() && name.Pkg() != qpos.info.Pkg { On 2014/12/04 17:22:44, adonovan wrote: > isAccessibleFrom Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:182: // Finds a list of package level variables which are errors and visible within On 2014/12/04 17:22:44, adonovan wrote: > // findVisibleErrs returns a mapping from each package-level variable of type > "error" to nil. > > (Needn't mention scope; that restriction impliclity applies to everything.) Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:187: for _, member := range pkg.Members { On 2014/12/04 17:22:43, adonovan wrote: > for _, mem Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:194: gbltype = gbltype.(*types.Pointer).Elem() On 2014/12/04 17:22:43, adonovan wrote: > delete Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:195: if gbltype != errtype { On 2014/12/04 17:22:44, adonovan wrote: > if !types.IsIdentical(deref(gbltype), builtinErrorType) { > > But: consider globals of concrete types assignable to 'error' too? Done. Considered it, but the usual idiom of error handling is creating an error of type error at package level. e.g. io.EOF, io.ErrUnexpectedEOF are both errors, not some io specific error. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:208: // Finds a list of package level constants which are errors and visible within On 2014/12/04 17:22:44, adonovan wrote: > // findVisibleConsts returns a mapping from each package-level constant > assignable to type "error", to nil. Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:213: for _, member := range pkg.Members { On 2014/12/04 17:22:43, adonovan wrote: > for _, mem Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:219: if !types.Implements(consttype, errtype.Underlying().(*types.Interface)) { On 2014/12/04 17:22:43, adonovan wrote: > if !types.Implements(obj.Type(), builtinErrorType) builtinErrorType is a Named. Have to go through this unwieldy cast in order to get it to be an *Interface https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:222: if !consts.Object().Exported() && qpos.info.Pkg != consts.Package().Object { On 2014/12/04 17:22:44, adonovan wrote: > if !isAccessibleFrom(obj, qpos.info.Pkg) { > > Could you also make describe.go:537 use isAccessibleFrom? Thanks. Done. describe line checks on a name being accessible, not an object. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:237: return cmp < 0 || (cmp == 0 && a[i].String() < a[j].String()) On 2014/12/04 17:22:44, adonovan wrote: > redundant parens x 3 > Use a linebreak after || if you like. Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:260: typ types.Type On 2014/12/04 17:22:43, adonovan wrote: > // a concrete type N or *N that implements error Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:261: name *types.TypeName On 2014/12/04 17:22:44, adonovan wrote: > // the named type N Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:261: name *types.TypeName On 2014/12/04 17:22:44, adonovan wrote: > obj *types.TypeName Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:274: printf(r.qpos, "error may point to these globals:") On 2014/12/04 17:22:44, adonovan wrote: > "this error" x 3 Done. https://codereview.appspot.com/167420043/diff/50001/oracle/whicherrs.go#newco... oracle/whicherrs.go:286: printf(r.qpos, "error may contain these types:") On 2014/12/04 17:22:44, adonovan wrote: > "these dynamic types" Done.
Sign in to reply to this message.
https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go File oracle/whicherrs.go (right): https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:29: // can be queried recursively somehow. The analysis can do this but the API doesn't expose it right now. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:30: delete blank https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:128: // These values are all make interfaces You should check the type assertion. They could be tagged objects created by reflection, if that analysis feature is enabled. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:201: constants := make(map[ssa.Const]*ssa.NamedConst) This code isn't wrong, but storing an ssa.Const by value (not pointer) is a little odd---ssa.Values aren't supposed to be copied, identity is usually important. You should be ok just storing the *ssa.Const in the map. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:209: if !types.Implements(consttype, builtinErrorType.Underlying().(*types.Interface)) { Since we know the RHS is an interface, we can just use: types.AssignableTo(consttype, builtinErrorType) and avoid the cast. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:241: // a concrete type N or *N that implements error I usually put noun-phrase comments inline: typ types.Type // concrete type N or *N that implements error obj *types.TypeName// the named type N
Sign in to reply to this message.
PTAL https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go File oracle/whicherrs.go (right): https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:30: On 2014/12/05 02:03:39, adonovan wrote: > delete blank Done. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:128: // These values are all make interfaces On 2014/12/05 02:03:39, adonovan wrote: > You should check the type assertion. They could be tagged objects created by > reflection, if that analysis feature is enabled. Done. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:201: constants := make(map[ssa.Const]*ssa.NamedConst) On 2014/12/05 02:03:39, adonovan wrote: > This code isn't wrong, but storing an ssa.Const by value (not pointer) is a > little odd---ssa.Values aren't supposed to be copied, identity is usually > important. > > You should be ok just storing the *ssa.Const in the map. That wont work. *ssa.Const gets duplicated at each reference site and doesn't implement any sort of equality to the Value in the *ssa.NamedConst. Using the value instead of the pointer for the map makes this a comparison on type + value, which will work on each of the duplicated ssa.Const throughout the program. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:209: if !types.Implements(consttype, builtinErrorType.Underlying().(*types.Interface)) { On 2014/12/05 02:03:39, adonovan wrote: > Since we know the RHS is an interface, we can just use: > types.AssignableTo(consttype, builtinErrorType) > and avoid the cast. Done. https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:241: // a concrete type N or *N that implements error On 2014/12/05 02:03:39, adonovan wrote: > I usually put noun-phrase comments inline: > > typ types.Type // concrete type N or *N that implements error > obj *types.TypeName// the named type N Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go File oracle/whicherrs.go (right): https://codereview.appspot.com/167420043/diff/90001/oracle/whicherrs.go#newco... oracle/whicherrs.go:201: constants := make(map[ssa.Const]*ssa.NamedConst) On 2014/12/05 13:03:59, DMorsing wrote: > On 2014/12/05 02:03:39, adonovan wrote: > > This code isn't wrong, but storing an ssa.Const by value (not pointer) is a > > little odd---ssa.Values aren't supposed to be copied, identity is usually > > important. > > > > You should be ok just storing the *ssa.Const in the map. > > That wont work. *ssa.Const gets duplicated at each reference site and doesn't > implement any sort of equality to the Value in the *ssa.NamedConst. > > Using the value instead of the pointer for the map makes this a comparison on > type + value, which will work on each of the duplicated ssa.Const throughout the > program. You're quite right; I had a TODO item for a long time to canonicalize equal Const values but eventually got rid of it.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=e41382aa4a20&repo=tools *** x/tools/oracle: add whicherrs query mode The whicherrs query mode takes the position of an error and returns the set of constants, globals and types visible from within the scope of the error being queried. It is meant to be used as a shortcut to find out which errors should be handled for a given functions call. LGTM=adonovan R=golang-codereviews, dominik.honnef, adonovan CC=golang-codereviews https://codereview.appspot.com/167420043
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
