Merge branch 'main' into zzq/function-contracts/auto-infer

.github/workflows/ci.yml

@@ -29,6 +29,7 @@ jobs:
         name: Lint
 
   test:
+    name: Test
     runs-on: ubuntu-latest
     strategy:
       matrix:

.github/workflows/golden-test.yml

@@ -0,0 +1,66 @@
+name: Golden Test
+
+# NilAway output may change due to introduction of new feature or bug fixes. Since NilAway is still
+# at early stage of development, constantly updating / maintaining the golden test output will be
+# a burden. Therefore, we run this as a separate CI job and post the differences as a PR comment
+# for manual reviews.
+on:
+  pull_request:
+
+jobs:
+  golden-test:
+    name: Golden Test
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        name: Check out repository
+
+      - name: Fetch base branch (${{ github.event.pull_request.base.ref }}) locally
+        run: git fetch origin ${{ github.event.pull_request.base.ref }}:${{ github.event.pull_request.base.ref }}
+
+      - name: Set up Go
+        uses: actions/setup-go@v4
+        with:
+          go-version: 1.21.x
+          cache: false
+
+      - name: Golden Test
+        id: golden_test
+        # Run golden test by comparing HEAD and the base branch (the target branch of the PR).
+        run: |
+          make golden-test ARGS="-base-branch ${{ github.event.pull_request.base.ref }} -result-file ${{ runner.temp }}/golden-test-result.md"
+
+      - uses: actions/github-script@v7
+        with:
+          script: |
+            const fsp = require('fs').promises;
+            
+            const issue_number = context.issue.number;
+            const owner = context.repo.owner;
+            const repo = context.repo.repo;
+            const body = await fsp.readFile(`${{ runner.temp }}/golden-test-result.md`, 'utf8');
+            
+            // First find the comments made by the bot.
+            const comments = await github.rest.issues.listComments({
+              owner: owner,
+              repo: repo,
+              issue_number: issue_number
+            });
+            const botComment = comments.data.find(comment => comment.user.login === 'github-actions[bot]' && comment.body.startsWith('## Golden Test'));
+            
+            // Update or create the PR comment.
+            if (botComment) {
+              await github.rest.issues.updateComment({
+                owner: owner,
+                repo: repo,
+                comment_id: botComment.id,
+                body: body
+              });
+            } else {
+              await github.rest.issues.createComment({
+                owner: owner,
+                repo: repo,
+                issue_number: issue_number,
+                body: body
+              });
+            }

Makefile

@@ -7,6 +7,10 @@ GOLANGCI_LINT_VERSION := $(shell golangci-lint --version 2>/dev/null)
 .PHONY: all
 all: build lint test
 
+.PHONY: clean
+clean:
+	@rm -rf $(GOBIN)
+
 .PHONY: build
 build:
 	go install go.uber.org/nilaway/cmd/nilaway
@@ -20,6 +24,11 @@ cover:
 	go test -v -race -coverprofile=cover.out -coverpkg=./... -v ./...
 	go tool cover -html=cover.out -o cover.html
 
+.PHONY: golden-test
+golden-test:
+	@cd tools && go install go.uber.org/nilaway/tools/cmd/golden-test
+	@$(GOBIN)/golden-test $(ARGS)
+
 .PHONY: lint
 lint: golangci-lint nilaway-lint tidy-lint
 

README.md

@@ -25,13 +25,29 @@ optimizations to further reduce its footprint.
 nil panics we have observed in production, allowing NilAway to maintain a good balance between usefulness and build-time 
 overhead.
 
-## Installation
-
-NilAway is implemented using the standard [go/analysis](https://pkg.go.dev/golang.org/x/tools/go/analysis) framework, 
-making it easy to integrate with existing analyzer drivers (e.g., [golangci-lint](https://github.com/golangci/golangci-lint),
-[nogo](https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst), or 
-[running as a standalone checker](https://pkg.go.dev/golang.org/x/tools/go/analysis/singlechecker)). Here, we list the 
-instructions for running NilAway as a standalone checker. More integration supports will be added soon.
+:star2: For more detailed technical discussion, please check our [Wiki][wiki], [Engineering Blog][blog], and paper (WIP).
+
+## Running NilAway
+
+NilAway is implemented using the standard [go/analysis][go-analysis], making it easy to integrate with existing analyzer
+drivers (i.e., [golangci-lint][golangci-lint], [nogo][nogo], or [running as a standalone checker][singlechecker]).
+
+> [!IMPORTANT]  
+> Due to the sophistication of the analyses that NilAway does, NilAway caches its findings about a particular 
+> package via the [Fact Mechanism][fact-mechanism] from the [go/analysis][go-analysis] framework. Therefore, it is 
+> _highly_ recommended to leverage a driver that supports modular analysis (i.e., bazel/nogo or golangci-lint, but _not_
+> the standalone checker since it stores all facts in memory) for better performance on large projects. For example,
+> see [instructions][nogo-instructions] below for running NilAway on your project with bazel/nogo.
+
+> [!IMPORTANT]  
+> By default, NilAway analyzes _all_ Go code, including the standard libraries and dependencies. This helps NilAway 
+> better understand the code form dependencies and reduce its false negatives. However, this would also incur a 
+> significant performance cost (only once for drivers with modular support) and increase the number of non-actionable 
+> errors in dependencies, for large Go projects with a lot of dependencies.
+> 
+> We highly recommend using the [include-pkgs][include-pkgs-flag] flag to narrow down the analysis to your project's 
+> code exclusively. This directs NilAway to skip analyzing dependencies (e.g., third-party libraries), allowing you to 
+> focus solely on potential nil panics reported by NilAway in your first-party code!
 
 ### Standalone Checker
 
@@ -42,9 +58,67 @@ go install go.uber.org/nilaway/cmd/nilaway@latest
 
 Then, run the linter by:
 ```shell
-nilaway ./...
+nilaway -include-pkgs="<YOUR_PKG_PREFIX>,<YOUR_PKG_PREFIX_2>" ./...
 ```
 
+### Bazel/nogo
+
+Running with bazel/nogo requires slightly more efforts. First follow the instructions from [rules_go][rules-go], 
+[gazelle][gazelle], and [nogo][nogo] to set up your Go project such that it can be built with bazel/nogo with no or 
+default set of linters configured. Then,
+
+(1) Add `import _ "go.uber.org/nilaway"` to your `tools.go` file (or other file that you use for configuring tool 
+dependencies, see [How can I track tool dependencies for a module?][track-tool-dependencies] from Go Modules 
+documentation) to avoid `go mod tidy` from removing NilAway as a tool dependency.
+
+(2) Run the following commands to add NilAway as a tool dependency to your project:
+```bash
+# Get NilAway as a dependency, as well as getting its transitive dependencies in go.mod file.
+$ go get go.uber.org/nilaway@latest
+# This should not remove NilAway as a dependency in your go.mod file.
+$ go mod tidy
+# Run gazelle to sync dependencies from go.mod to WORKSPACE file.
+$ bazel run //:gazelle -- update-repos -from_file=go.mod
+```
+
+(3) Add NilAway to nogo configurations (usually in top-level `BUILD.bazel` file):
+
+```diff
+nogo(
+    name = "my_nogo",
+    visibility = ["//visibility:public"],  # must have public visibility
+    deps = [
++++     "@org_uber_go_nilaway//:go_default_library",
+    ],
+    config = "config.json",
+)
+```
+
+(4) Run bazel build to see NilAway working (any nogo error will stop the bazel build, you can use the `--keep_going` 
+flag to request bazel to build as much as possible):
+
+```bash
+$ bazel build --keep_going //...
+```
+
+(5) See [nogo documentation][nogo-configure-analyzers] on how to pass a configuration JSON to the nogo driver, and see 
+our [wiki page][nogo-configure-nilaway] on how to pass configurations to NilAway.
+
+### golangci-lint
+
+NilAway, as its current form, still reports a fair number of false positives. This makes NilAway fail to be merged with 
+[golangci-lint][golangci-lint] and be offered as a linter (see [PR#4045][pr-4045]). The alternatives are to:
+
+(1) [build NilAway as a plugin to golangci-lint][nilaway-as-a-plugin]: the Go plugin system _requires_ that NilAway 
+  shares the exact same versions of dependencies as the ones from golangci-lint. This is almost impossible for us to
+  maintain.
+
+(2) fork golangci-lint and add NilAway: it would also require us to keep in sync with upstream and cause unnecessary
+  confusions to the users.
+
+:raising_hand: We would love to integrate NilAway with golangci-lint! If you have any other ideas here, please raise an issue (or 
+better, a PR)!
+
 ## Code Examples
 
 Let's look at a few examples to see how NilAway can help prevent nil panics.
@@ -64,7 +138,7 @@ error showing this nilness flow:
 
 ```
 go.uber.org/example.go:12:9: error: Potential nil panic detected. Observed nil flow from source to dereference point:
-    -> go.uber.org/example.go:12:9: unassigned variable `p` accessed field `f`
+    - go.uber.org/example.go:12:9: unassigned variable `p` accessed field `f`
 ```
 
 If we guard this dereference with a nilness check (`if p != nil`), the error goes away.
@@ -87,13 +161,13 @@ the nilness flow across function boundaries:
 
 ```
 go.uber.org/example.go:23:13: error: Potential nil panic detected. Observed nil flow from source to dereference point:
-    -> go.uber.org/example.go:20:14: literal `nil` returned from `foo()` in position 0
-    -> go.uber.org/example.go:23:13: result 0 of `foo()` dereferenced
+    - go.uber.org/example.go:20:14: literal `nil` returned from `foo()` in position 0
+    - go.uber.org/example.go:23:13: result 0 of `foo()` dereferenced
 ```
 
 Note that in the above example, `foo` does not necessarily have to reside in the same package as `bar`. NilAway is able
 to track nil flows across packages as well. Moreover, NilAway handles Go-specific language constructs such as receivers,
-interfaces, type assertions, type switches, and more. For more detailed discussion, please check our paper.
+interfaces, type assertions, type switches, and more.
 
 ## Configurations
 
@@ -118,9 +192,25 @@ our [Uber Contributor License Agreement](https://cla-assistant.io/uber-go/nilawa
 
 This project is copyright 2023 Uber Technologies, Inc., and licensed under Apache 2.0.
 
+[go-analysis]: https://pkg.go.dev/golang.org/x/tools/go/analysis
+[golangci-lint]: https://github.com/golangci/golangci-lint
+[singlechecker]: https://pkg.go.dev/golang.org/x/tools/go/analysis/singlechecker
+[nogo]: https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst
 [doc-img]: https://pkg.go.dev/badge/go.uber.org/nilaway.svg
 [doc]: https://pkg.go.dev/go.uber.org/nilaway
 [ci-img]: https://github.com/uber-go/nilaway/actions/workflows/ci.yml/badge.svg
 [ci]: https://github.com/uber-go/nilaway/actions/workflows/ci.yml
 [cov-img]: https://codecov.io/gh/uber-go/nilaway/branch/main/graph/badge.svg
-[cov]: https://codecov.io/gh/uber-go/nilaway
+[cov]: https://codecov.io/gh/uber-go/nilaway
+[wiki]: https://github.com/uber-go/nilaway/wiki
+[blog]: https://www.uber.com/blog/nilaway-practical-nil-panic-detection-for-go/
+[fact-mechanism]: https://pkg.go.dev/golang.org/x/tools/go/analysis#hdr-Modular_analysis_with_Facts
+[include-pkgs-flag]: https://github.com/uber-go/nilaway/wiki/Configuration#include-pkgs
+[pr-4045]: https://github.com/golangci/golangci-lint/issues/4045
+[nilaway-as-a-plugin]: https://golangci-lint.run/contributing/new-linters/#how-to-add-a-private-linter-to-golangci-lint
+[rules-go]: https://github.com/bazelbuild/rules_go
+[gazelle]: https://github.com/bazelbuild/bazel-gazelle
+[track-tool-dependencies]: https://go.dev/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module
+[nogo-configure-analyzers]: https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst#id14
+[nogo-configure-nilaway]: https://github.com/uber-go/nilaway/wiki/Configuration#nogo
+[nogo-instructions]: https://github.com/uber-go/nilaway?tab=readme-ov-file#bazelnogo

annotation/affiliation.go

@@ -59,16 +59,16 @@ func (a AffiliationPair) implementingMethodAsExpr() ast.Expr {
 func FullTriggerForInterfaceParamFlow(affiliation AffiliationPair, paramNum int) FullTrigger {
 	return FullTrigger{
 		Producer: &ProduceTrigger{
-			Annotation: InterfaceParamReachesImplementation{
-				TriggerIfNilable: TriggerIfNilable{
+			Annotation: &InterfaceParamReachesImplementation{
+				TriggerIfNilable: &TriggerIfNilable{
 					Ann: ParamKeyFromArgNum(affiliation.InterfaceMethod, paramNum)},
 				AffiliationPair: affiliation,
 			},
 			Expr: affiliation.interfaceMethodAsExpr(),
 		},
 		Consumer: &ConsumeTrigger{
-			Annotation: MethodParamFromInterface{
-				TriggerIfNonNil: TriggerIfNonNil{
+			Annotation: &MethodParamFromInterface{
+				TriggerIfNonNil: &TriggerIfNonNil{
 					Ann: ParamKeyFromArgNum(affiliation.ImplementingMethod, paramNum)},
 				AffiliationPair: affiliation,
 			},
@@ -88,16 +88,16 @@ func FullTriggerForInterfaceParamFlow(affiliation AffiliationPair, paramNum int)
 func FullTriggerForInterfaceResultFlow(affiliation AffiliationPair, retNum int) FullTrigger {
 	return FullTrigger{
 		Producer: &ProduceTrigger{
-			Annotation: MethodResultReachesInterface{
-				TriggerIfNilable: TriggerIfNilable{
+			Annotation: &MethodResultReachesInterface{
+				TriggerIfNilable: &TriggerIfNilable{
 					Ann: RetKeyFromRetNum(affiliation.ImplementingMethod, retNum)},
 				AffiliationPair: affiliation,
 			},
 			Expr: affiliation.implementingMethodAsExpr(),
 		},
 		Consumer: &ConsumeTrigger{
-			Annotation: InterfaceResultFromImplementation{
-				TriggerIfNonNil: TriggerIfNonNil{
+			Annotation: &InterfaceResultFromImplementation{
+				TriggerIfNonNil: &TriggerIfNonNil{
 					Ann: RetKeyFromRetNum(affiliation.InterfaceMethod, retNum)},
 				AffiliationPair: affiliation,
 			},