Fix potential subinclude lockup (#3305)

* test case

* Ban it

* make it fail

* make work again

* Attempt fix

* comment

src/core/package.go

@@ -240,6 +240,9 @@ func FindOwningPackage(state *BuildState, file string) BuildLabel {
 // suggestTargets suggests the targets in the given package that might be misspellings of
 // the requested one.
 func suggestTargets(pkg *Package, label, dependent BuildLabel) string {
+	if pkg == nil {
+		return ""
+	}
 	// The initial haystack only contains target names
 	haystack := []string{}
 	for _, t := range pkg.AllTargets() {

src/core/state.go

@@ -1017,7 +1017,9 @@ func (state *BuildState) ActivateTarget(pkg *Package, label, dependent BuildLabe
 			// Bazel allows some things that look like build targets but aren't - notably the syntax
 			// to load(). It suits us to treat that as though it is one, but we now have to
 			// implicitly make it available.
-			exportFile(state, pkg, label)
+			if pkg != nil {
+				exportFile(state, pkg, label)
+			}
 		} else {
 			msg := fmt.Sprintf("Parsed build file %s but it doesn't contain target %s", pkg.Filename, label.Name)
 			if dependent != OriginalTarget {
@@ -1029,6 +1031,9 @@ func (state *BuildState) ActivateTarget(pkg *Package, label, dependent BuildLabe
 	if state.ParsePackageOnly && !mode.IsForSubinclude() {
 		return nil // Some kinds of query don't need a full recursive parse.
 	} else if label.IsAllTargets() {
+		if pkg == nil {
+			return fmt.Errorf("Cannot use :all in this context")
+		}
 		if dependent == OriginalTarget {
 			for _, target := range pkg.AllTargets() {
 				// Don't activate targets that were added in a post-build function; that causes a race condition

src/parse/asp/builtins.go

@@ -392,7 +392,7 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget {
 	// If the subinclude is local to this package, it must already exist in the graph. If it already exists in the graph
 	// but isn't activated, we should activate it otherwise WaitForSubincludedTarget might block. This can happen when
 	// another package also subincludes this target, and queues it first.
-	if isLocal && s.pkg != nil {
+	if isLocal {
 		t := s.state.Graph.Target(l)
 		if t == nil {
 			s.Error("Target :%s is not defined in this package; it has to be defined before the subinclude() call", l.Name)

test/double_local_subinclude/BUILD

@@ -0,0 +1,8 @@
+subinclude("//test/build_defs")
+
+please_repo_e2e_test(
+    name = "double_local_subinclude_test",
+    # This isn't 100% deterministic but 20 clean runs should be reasonably sufficient
+    plz_command = "for i in `seq 1 20`; do plz build -p -v 2 //...; done",
+    repo = "test_repo",
+)

test/double_local_subinclude/test_repo/a/1.build_defs

@@ -0,0 +1 @@
+THE_QUESTION = "what is the answer to life, the universe and everything?"

test/double_local_subinclude/test_repo/a/2.build_defs

@@ -0,0 +1,3 @@
+subinclude("//a:defs_1")
+
+THE_ANSWER = 42

test/double_local_subinclude/test_repo/a/BUILD_FILE

@@ -0,0 +1,16 @@
+filegroup(
+    name = "defs_1",
+    srcs = ["1.build_defs"],
+    visibility = ["PUBLIC"],
+)
+
+filegroup(
+    name = "defs_2",
+    srcs = ["2.build_defs"],
+    visibility = ["PUBLIC"],
+)
+
+subinclude(":defs_2")
+
+assert THE_QUESTION.endswith("?"), "not a question"
+assert THE_ANSWER == 42, "wrong answer to life, the universe and everything"

test/double_local_subinclude/test_repo/b/BUILD_FILE

@@ -0,0 +1,3 @@
+subinclude("//a:defs_2")
+
+assert THE_ANSWER == 42, "wrong answer to life, the universe and everything"

test/double_local_subinclude/test_repo/c/BUILD_FILE

@@ -0,0 +1,3 @@
+subinclude("//a:defs_1")
+
+assert THE_QUESTION.endswith("?"), "not a question"