Initial, best-effort static typechecker for bigslice.Func calls #69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
josh-newman
force-pushed
the
typelint
branch
from
0da5498 to
5b6a44b
Compare
josh-newman
force-pushed
the
typelint
branch
from
5b6a44b to
9e64ca6
Compare
jcharum
pushed a commit
that referenced
this pull request
Jun 1, 2022
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is designed to run as a golangci-lint linter, however I haven't set up a plugin binary yet so far now it just runs as a standalone binary. It's simple and designed to catch some basic errors with func invocations. With slightly more work it can efficiently support invocations across packages, too (right now it just skips). That alone might be worthwhile, since it's pretty quick.
It'd be more interesting to check types on the bigslice API's slice operations. However, I think in practice there's often more dynamism that'd prevent simple static type inference, and there may be diminishing returns. We can look more carefully and consider it in the future.
This PR is based on a day of curiosity; I was waiting for a job to run and happened to discover go/analysis. We can certainly discuss the overall approach, testing, etc. if they seem questionable.