Skip to content

Commit 6270bca

Browse files
josh-newmanjcharum
authored andcommitted
bigslice: check Func argument types (for some calls)
Summary: Copies and updates the typecheck analyzer from github.com//pull/69. Right now, this can only check types if `session.Run` or `Must` calls a `bigslice.Func` in the same package, by identifier (that is, statically). However, I think this is already pretty useful. I think there are ways we can extend typechecking utility, for example by checking slice operations within a `Func`. But this is a first step. Test Plan: CI builds will execute the typechecker on existing bigslice code and ensure there are no unrelated breakages. [nogo is currently experimental](https://github.com/bazelbuild/rules_go/blob/v0.22.11/go/nogo.rst), however this usage only runs the one in-house analysis pass, so I'm hoping the impact of any bugs will be limited. But, it's easy to disable this (remove from toolchain registration) if necessary. There's an intentionally-broken test (marked as manual) to check that the typechecker detects and reports the issue: $ bazel build //go/src/grail.com/bigslice/typechecktest/{innerfunc,wrongarg} ... INFO: Found 2 targets... ERROR: /mnt/data/src/g6/go/src/grail.com/bigslice/typechecktest/wrongarg/BUILD:18:1: GoCompilePkg go/src/grail.com/bigslice/typechecktest/wrongarg/linux_amd64_stripped/wrongarg%/grail.com/bigslice/typechecktest/wrongarg.a failed (Exit 1) builder failed: error executing command bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -tags bazel -src go/src/grail.com/bigslice/typechecktest/wrongarg/wrongarg.go -arc ... (remaining 19 argument(s) skipped) Use --sandbox_debug to see verbose messages from the sandbox compilepkg: nogo: errors found by nogo during build-time code analysis: /tmp/bazel-root/08c79ec0540d8dbd0697ab520fcc7f4a/sandbox/linux-sandbox/235/execroot/grail/go/src/grail.com/bigslice/typechecktest/wrongarg/wrongarg.go:22:34: bigslice type error: func "testFunc" argument "_" [0] requires int, but got string /tmp/bazel-root/08c79ec0540d8dbd0697ab520fcc7f4a/sandbox/linux-sandbox/235/execroot/grail/go/src/grail.com/bigslice/typechecktest/wrongarg/wrongarg.go:23:24: bigslice type error: testFunc requires 2 arguments, but got 3 INFO: Elapsed time: 7.619s, Critical Path: 6.35s INFO: 110 processes: 110 linux-sandbox. FAILED: Build did NOT complete successfully The typechecker also discovered existing, broken instances in our codebase (it may be obsolete, but hey, I'll take it). Reviewers: jcharumilind Reviewed By: jcharumilind Subscribers: O38 mixture model classifier Differential Revision: https://phabricator.grailbio.com/D53043 fbshipit-source-id: f31ac23
1 parent ce3baf0 commit 6270bca

File tree

2 files changed

+128
-0
lines changed

2 files changed

+128
-0
lines changed

analysis/typecheck/typecheck.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
package typecheck
2+
3+
import (
4+
"fmt"
5+
"go/ast"
6+
"go/types"
7+
8+
"golang.org/x/tools/go/analysis"
9+
"golang.org/x/tools/go/analysis/passes/inspect"
10+
"golang.org/x/tools/go/ast/inspector"
11+
"golang.org/x/tools/go/types/typeutil"
12+
)
13+
14+
var Analyzer = &analysis.Analyzer{
15+
Name: "bigslice_typecheck",
16+
Doc: `check bigslice func call arguments
17+
18+
Basic typechecker for bigslice programs that inspects session.Run and
19+
session.Must calls to ensure the arguments are compatible with the Func.
20+
Checks are limited by static analysis and are best-effort. For example, the call
21+
session.Must(ctx, chooseFunc(), args...)
22+
cannot be checked, because it uses chooseFunc() instead of a simple identifier.
23+
24+
Typechecking does not include any slice operations yet.`,
25+
Requires: []*analysis.Analyzer{inspect.Analyzer},
26+
Run: run,
27+
}
28+
29+
const (
30+
funcFullName = "github.com/grailbio/bigslice.Func"
31+
execMustFullName = "(*github.com/grailbio/bigslice/exec.Session).Must"
32+
execRunFullName = "(*github.com/grailbio/bigslice/exec.Session).Run"
33+
)
34+
35+
func run(pass *analysis.Pass) (interface{}, error) {
36+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
37+
38+
// funcTypes describes the types of declared bigslice.FuncValues.
39+
// TODO: As stated below, we're only recording top-level func for now.
40+
// If that changes, this map should be keyed by a global identifier, not *ast.Ident.
41+
// TODO: We may also want to return these types as Facts to allow checking across packages.
42+
funcTypes := map[string]*types.Signature{}
43+
44+
// Collect the types of bigslice.Funcs.
45+
// TODO: ValueSpec captures top-level vars, but maybe we should include non-top-level ones, too.
46+
inspect.Preorder([]ast.Node{&ast.ValueSpec{}}, func(node ast.Node) {
47+
valueSpec := node.(*ast.ValueSpec)
48+
for i := range valueSpec.Values {
49+
call, ok := valueSpec.Values[i].(*ast.CallExpr)
50+
if !ok {
51+
continue
52+
}
53+
fn := typeutil.StaticCallee(pass.TypesInfo, call)
54+
if fn == nil {
55+
continue
56+
}
57+
if fn.FullName() != funcFullName {
58+
continue
59+
}
60+
if len(call.Args) != 1 {
61+
panic(fmt.Errorf("unexpected arguments to bigslice.Func: %v", call.Args))
62+
}
63+
funcType := pass.TypesInfo.TypeOf(call.Args[0]).Underlying().(*types.Signature)
64+
funcTypes[valueSpec.Names[i].Name] = funcType
65+
}
66+
})
67+
68+
inspect.Preorder([]ast.Node{&ast.CallExpr{}}, func(node ast.Node) {
69+
call := node.(*ast.CallExpr)
70+
fn := typeutil.StaticCallee(pass.TypesInfo, call)
71+
if fn == nil {
72+
return
73+
}
74+
if name := fn.FullName(); name != execRunFullName && name != execMustFullName {
75+
return
76+
}
77+
78+
funcValueIdent, ok := call.Args[1].(*ast.Ident)
79+
if !ok {
80+
// This function invocation is more complicated than a simple identifier.
81+
// Give up on typechecking this call.
82+
return
83+
}
84+
funcType, ok := funcTypes[funcValueIdent.Name]
85+
if !ok {
86+
// TODO: Propagate bigslice.Func types as Facts so we can do a better job here.
87+
return
88+
}
89+
90+
wantArgTypes := funcType.Params()
91+
gotArgs := call.Args[2:]
92+
if want, got := wantArgTypes.Len(), len(gotArgs); want != got {
93+
pass.ReportRangef(funcValueIdent,
94+
"bigslice type error: %s requires %d arguments, but got %d",
95+
funcValueIdent.Name, want, got)
96+
return
97+
}
98+
99+
for i, gotArg := range gotArgs {
100+
wantType := wantArgTypes.At(i).Type()
101+
gotType := pass.TypesInfo.TypeOf(gotArg)
102+
if !types.AssignableTo(gotType, wantType) {
103+
pass.ReportRangef(gotArg,
104+
"bigslice type error: func %q argument %q [%d] requires %v, but got %v",
105+
funcValueIdent.Name, wantArgTypes.At(i).Name(), i, wantType, gotType)
106+
}
107+
}
108+
})
109+
110+
return nil, nil
111+
}

cmd/slicetypecheck/main.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// Standalone bigslice typechecker (alpha).
2+
//
3+
// This is new and in testing. Please report any issues:
4+
// https://github.com/grailbio/bigslice/issues.
5+
//
6+
// TODO: Consider merging this into the main `bigslice` command, when it's well-tested.
7+
// TODO: Consider supporting the golangci-lint plugin interface.
8+
package main
9+
10+
import (
11+
"github.com/grailbio/bigslice/analysis/typecheck"
12+
"golang.org/x/tools/go/analysis/singlechecker"
13+
)
14+
15+
func main() {
16+
singlechecker.Main(typecheck.Analyzer)
17+
}

0 commit comments

Comments
 (0)