Repository: google/go-flow-levee
Pull request #153 (2020-10-15 00:00:55)

Defend against infinite recursion in source identification.
I was running `master` against Kubernetes and blew the stack.  `*types.Signature` is also self-referential with `tt.Underlying()`.  It looks like `*types.Pointer` is, too.

I need to run that failure mode again to get a real test-case for it, but figure I'd open it quickly for review of the main changes.  Of course, trying something like `type Producer func() Source` ends up getting handled as a `*types.Named` rather than a `*types.Signature`, so we're hitting that corner of "`analysistest` is kind of hard to work with."

* Promote examination of `*types.Named` as the "main" identification case.
    *  If we're sticking to that pattern, we should consider renaming `IsSource` to something like `IsNamedSource` or something, to counter `IsSourceType`.  I'll leave that to a future PR discussion though.
* Enumerate all the kinds that we are ignoring, and add a panic if we're missing something.
    * I know we generally shouldn't panic, but I consider this an indication of catastrophic failure in need of immediate remediation.
    * If you hate it, I could be talked into rewriting this to cover all `types` Kinds, and having the `default:` case print an error and returning false to indicate a gap in our structure here.


mlevesquedion: Good catch. This is quite concerning. I introduced this change 10 days ago, and I didn't run the analyzer then, nor have I run it since. Clearly, running the analyzer on kubernetes (or some other large code base, but for now I think kubernetes is the most relevant candidate) should be done before every merge.

**A few ideas for prevention:**

- Add a checkbox "I ran the analyzer on kubernetes" to the PR template.

By itself, this is not a sufficient safeguard, but it would be a helpful reminder.

- Run levee on kubernetes as part of our CI.

This would provide a much higher level of confidence. Unfortunately, it is not without limitations: 1. I recently realized that we can merge even if the CI is running, or if it has failed. This can happen by accident, and it has happened recently. 2. Running the analyzer on kubernetes is much more computationally intensive than running our tests. We may need to upgrade our CI, lest we want to wait half an hour every time we push something.

- Run levee on kubernetes as part of a pre-merge event. Reject the merge on failure. (We would need to define "failure".  To me, "failure" here would mean some kind of crash.)

This would provide the highest level of confidence as far as I can tell. I personally don't know how to do this, so I would need to investigate if I pick up this task.

For now, I suggest we add the checkbox and look into running levee on kubernetes as a pre-merge gatekeeper. WDYT?
mlevesquedion: > *types.Signature is also self-referential with tt.Underlying(). It looks like *types.Pointer is, too.

It is my understanding that `utils.Dereference` should take care of `*types.Pointer`, but it's good to have extra safety.
mlevesquedion: I think the current CI is good for most of our purposes, but probably not for this. IMO the current CI is appropriate for quick checks like the ones we have right now.

I think nightly would be a good start, but pre-merge would be preferable. At most, we're merging a few PR's a day (often less), so I don't think the added latency would be too painful. I do think faster feedback would be good to have, though. Pre-merge would also prevent weird interactions like we've had before (#143).

I don't have a strong preference, I think for now we should create an Issue and place it near the top of our priority list.
PurelyApplied: Getting tests into the existing harness wasn't so bad once I actually looked at what was blowing in up K8s.  I should dig into where exactly the line gets drawn in SSA, but we end up with a `*types.Signature` only when we are writing a function into a struct, it seems.  Something like `x := func() {}` doesn't cut muster.
mlevesquedion: > Getting tests into the existing harness wasn't so bad once I actually looked at what was blowing in up K8s. I should dig into where exactly the line gets drawn in SSA, but we end up with a *types.Signature only when we are writing a function into a struct, it seems. Something like x := func() {} doesn't cut muster.

SG, that makes sense. Please ignore the related comments in my review that I was doing concurrently.

In that case, maybe add a comment to the tests to explain what code these fields are exercising.

Patch:
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
index 0787bd9..87af549 100644
--- a/.github/PULL_REQUEST_TEMPLATE.md
+++ b/.github/PULL_REQUEST_TEMPLATE.md
@@ -3,4 +3,5 @@ Fixes #<issue_number_goes_here>
 > It's a good idea to open an issue first for discussion.
 
 - [ ] Tests pass
+- [ ] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out.
 - [ ] Appropriate changes to README are included in PR
\ No newline at end of file
diff --git a/internal/pkg/source/source.go b/internal/pkg/source/source.go
index ecbde83..77f2933 100644
--- a/internal/pkg/source/source.go
+++ b/internal/pkg/source/source.go
@@ -403,6 +403,8 @@ func sourcesFromBlocks(fn *ssa.Function, conf classifier) []*Source {
 func isSourceType(c classifier, t types.Type) bool {
 	deref := utils.Dereference(t)
 	switch tt := deref.(type) {
+	case *types.Named:
+		return c.IsSource(tt) || isSourceType(c, tt.Underlying())
 	case *types.Array:
 		return isSourceType(c, tt.Elem())
 	case *types.Slice:
@@ -413,10 +415,16 @@ func isSourceType(c classifier, t types.Type) bool {
 		key := isSourceType(c, tt.Key())
 		elem := isSourceType(c, tt.Elem())
 		return key || elem
-	case *types.Basic, *types.Interface, *types.Tuple, *types.Struct:
+	case *types.Basic, *types.Struct, *types.Tuple, *types.Interface, *types.Signature:
+		// These types do not currently represent possible source types
+		return false
+	case *types.Pointer:
+		// This should be unreachable due to the dereference above
 		return false
 	default:
-		return c.IsSource(tt) || isSourceType(c, tt.Underlying())
+		// The above should be exhaustive.  Reaching this default case is an error.
+		fmt.Printf("unexpected type received: %T %v; please report this issue\n", tt, tt)
+		return false
 	}
 }
 


Test patch:
diff --git a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
index 2a975f4..d109d88 100644
--- a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
+++ b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
@@ -20,6 +20,35 @@ type Alias = Source
 // type definition where the underlying type is a Source
 type Definition Source
 
+type FuncField struct {
+	flipCoin func() bool
+	producer func() Source
+	ptr      func() *Source
+}
+
+// This test exercises isSourceType for *types.Signature and related interactions.
+func TestFunctionFields() {
+	bar := FuncField{
+		flipCoin: func() bool {
+			return true // It's a trick coin
+		},
+		producer: func() Source {
+			return Source{} // want "source identified"
+		},
+		ptr: func() *Source {
+			return &Source{} // want "source identified"
+		},
+	}
+
+	// When the assigned-to type is a Source, then we expect two Allocs, and therefore two Source identifications.
+	// This Alloc doesn't occur if the assigned-to type is a pointer or interface
+	var i interface{} = bar.producer() // want "source identified"
+	s := bar.producer()                // want "source identified" "source identified"
+	ptr := bar.ptr()                   // want "source identified"
+
+	noop(bar, s, i, ptr)
+}
+
 // This function allows us to consume multiple arguments in a single line so this file can compile
 func noop(args ...interface{}) {}