go/packages: do not nullify Fset when NeedSyntax is set

Currently, Fset is initialized when either NeedTypes or NeedSyntax are set in newLoader().

However, later in refine() it is nullified using a different condition that doesn't take NeedSyntax into account.

Use the inverse condition to that of in newLoader() when deciding on whether to nullify Fset or not.

Resolves golang/go#48226.

Change-Id: Ic7c05dfe3337d5cf14aa185350a8e766e224c898
GitHub-Last-Rev: a906871
GitHub-Pull-Request: #506
Reviewed-on: https://go-review.googlesource.com/c/tools/+/601239
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Matloob <matloob@golang.org>

go/packages/packages.go

@@ -46,7 +46,6 @@ import (
 //
 // Unfortunately there are a number of open bugs related to
 // interactions among the LoadMode bits:
-// - https://github.com/golang/go/issues/48226
 // - https://github.com/golang/go/issues/56633
 // - https://github.com/golang/go/issues/56677
 // - https://github.com/golang/go/issues/58726
@@ -76,7 +75,7 @@ const (
 	// NeedTypes adds Types, Fset, and IllTyped.
 	NeedTypes
 
-	// NeedSyntax adds Syntax.
+	// NeedSyntax adds Syntax and Fset.
 	NeedSyntax
 
 	// NeedTypesInfo adds TypesInfo.
@@ -961,12 +960,14 @@ func (ld *loader) refine(response *DriverResponse) ([]*Package, error) {
 		}
 		if ld.requestedMode&NeedTypes == 0 {
 			ld.pkgs[i].Types = nil
-			ld.pkgs[i].Fset = nil
 			ld.pkgs[i].IllTyped = false
 		}
 		if ld.requestedMode&NeedSyntax == 0 {
 			ld.pkgs[i].Syntax = nil
 		}
+		if ld.requestedMode&NeedTypes == 0 && ld.requestedMode&NeedSyntax == 0 {
+			ld.pkgs[i].Fset = nil
+		}
 		if ld.requestedMode&NeedTypesInfo == 0 {
 			ld.pkgs[i].TypesInfo = nil
 		}

go/packages/packages_test.go

@@ -2645,6 +2645,42 @@ func testTypecheckCgo(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
+// TestIssue48226 ensures that when NeedSyntax is provided we do not nullify the
+// Fset, which is needed to resolve the syntax tree element positions to files.
+func TestIssue48226(t *testing.T) { testAllOrModulesParallel(t, testIssue48226) }
+func testIssue48226(t *testing.T, exporter packagestest.Exporter) {
+	exported := packagestest.Export(t, exporter, []packagestest.Module{
+		{
+			Name: "golang.org/fake/syntax",
+			Files: map[string]interface{}{
+				"syntax.go": `package test`,
+			},
+		},
+	})
+	defer exported.Cleanup()
+
+	exported.Config.Mode = packages.NeedFiles | packages.NeedSyntax
+
+	initial, err := packages.Load(exported.Config, "golang.org/fake/syntax")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(initial) != 1 {
+		t.Fatalf("exepected 1 package, got %d", len(initial))
+	}
+	pkg := initial[0]
+
+	if len(pkg.Errors) != 0 {
+		t.Fatalf("package has errors: %v", pkg.Errors)
+	}
+
+	fname := pkg.Fset.File(pkg.Syntax[0].Pos()).Name()
+	if filepath.Base(fname) != "syntax.go" {
+		t.Errorf("expected the package declaration position "+
+			"to resolve to \"syntax.go\", got %q instead", fname)
+	}
+}
+
 func TestModule(t *testing.T) {
 	testAllOrModulesParallel(t, testModule)
 }