internal/telemetry/cmd/stacks: reopen issues

Re-open an issue if it was closed as fixed, but we encounter a new stack
in a later version.

For golang/go#71045.

Change-Id: If6a4fe4091588e42b6f6c47e8705313352dc295e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/644115
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: jba

gopls/internal/golang/assembly.go

@@ -21,6 +21,7 @@ import (
 	"strings"
 
 	"golang.org/x/tools/gopls/internal/cache"
+	"golang.org/x/tools/gopls/internal/util/morestrings"
 )
 
 // AssemblyHTML returns an HTML document containing an assembly listing of the selected function.
@@ -103,7 +104,7 @@ func AssemblyHTML(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Pack
 		// Skip filenames of the form "<foo>".
 		if parts := insnRx.FindStringSubmatch(line); parts != nil {
 			link := "     " // if unknown
-			if file, linenum, ok := cutLast(parts[2], ":"); ok && !strings.HasPrefix(file, "<") {
+			if file, linenum, ok := morestrings.CutLast(parts[2], ":"); ok && !strings.HasPrefix(file, "<") {
 				if linenum, err := strconv.Atoi(linenum); err == nil {
 					text := fmt.Sprintf("L%04d", linenum)
 					link = sourceLink(text, web.SrcURL(file, linenum, 1))
@@ -117,11 +118,3 @@ func AssemblyHTML(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Pack
 	}
 	return buf.Bytes(), nil
 }
-
-// cutLast is the "last" analogue of [strings.Cut].
-func cutLast(s, sep string) (before, after string, ok bool) {
-	if i := strings.LastIndex(s, sep); i >= 0 {
-		return s[:i], s[i+len(sep):], true
-	}
-	return s, "", false
-}

gopls/internal/telemetry/cmd/stacks/stacks.go

@@ -77,6 +77,7 @@ import (
 	"net/url"
 	"os"
 	"os/exec"
+	"path"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -86,10 +87,12 @@ import (
 	"time"
 	"unicode"
 
+	"golang.org/x/mod/semver"
 	"golang.org/x/sys/unix"
 	"golang.org/x/telemetry"
 	"golang.org/x/tools/gopls/internal/util/browser"
 	"golang.org/x/tools/gopls/internal/util/moremaps"
+	"golang.org/x/tools/gopls/internal/util/morestrings"
 )
 
 // flags
@@ -243,8 +246,15 @@ func main() {
 		if issue, ok := claimedBy[id]; ok {
 			// existing issue, already updated above, just store
 			// the summary.
+			state := issue.State
+			if issue.State == "closed" && issue.StateReason == "completed" {
+				state = "completed"
+			}
 			summary := fmt.Sprintf("#%d: %s [%s]",
-				issue.Number, issue.Title, issue.State)
+				issue.Number, issue.Title, state)
+			if state == "completed" && issue.Milestone != nil {
+				summary += " milestone " + strings.TrimPrefix(issue.Milestone.Title, "gopls/")
+			}
 			existingIssues[summary] += total
 		} else {
 			// new issue, need to create GitHub issue and store
@@ -266,7 +276,7 @@ func main() {
 		for _, summary := range keys {
 			count := issues[summary]
 			// Show closed issues in "white".
-			if isTerminal(os.Stdout) && strings.Contains(summary, "[closed]") {
+			if isTerminal(os.Stdout) && (strings.Contains(summary, "[closed]") || strings.Contains(summary, "[completed]")) {
 				// ESC + "[" + n + "m" => change color to n
 				// (37 = white, 0 = default)
 				summary = "\x1B[37m" + summary + "\x1B[0m"
@@ -590,8 +600,14 @@ func updateIssues(cli *githubClient, issues []*Issue, stacks map[string]map[Info
 			body += "\nDups:"
 		}
 		body += " " + strings.Join(newStackIDs, " ")
-		if err := cli.updateIssueBody(issue.Number, body); err != nil {
-			log.Printf("added comment to issue #%d but failed to update body: %v",
+
+		update := updateIssue{number: issue.Number, Body: body}
+		if shouldReopen(issue, stacks) {
+			update.State = "open"
+			update.StateReason = "reopened"
+		}
+		if err := cli.updateIssue(update); err != nil {
+			log.Printf("added comment to issue #%d but failed to update: %v",
 				issue.Number, err)
 			continue
 		}
@@ -600,6 +616,50 @@ func updateIssues(cli *githubClient, issues []*Issue, stacks map[string]map[Info
 	}
 }
 
+// An issue should be re-opened if it was closed as fixed, and at least one of the
+// new stacks happened since the version containing the fix.
+func shouldReopen(issue *Issue, stacks map[string]map[Info]int64) bool {
+	if !issue.isFixed() {
+		return false
+	}
+	issueProgram, issueVersion, ok := parseMilestone(issue.Milestone)
+	if !ok {
+		return false
+	}
+	// TODO(jba?): handle other programs
+	if issueProgram != "gopls" {
+		return false
+	}
+	for _, stack := range issue.newStacks {
+		for info := range stacks[stack] {
+			if path.Base(info.Program) == issueProgram && semver.Compare(info.ProgramVersion, issueVersion) >= 0 {
+				log.Printf("reopening issue #%d: purportedly fixed in %s@%s, but found a new stack from version %s",
+					issue.Number, issueProgram, issueVersion, info.ProgramVersion)
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// An issue is fixed if it was closed because it was completed.
+func (i *Issue) isFixed() bool {
+	return i.State == "closed" && i.StateReason == "completed"
+}
+
+// parseMilestone parses a the title of a GitHub milestone that is in the format
+// PROGRAM/VERSION. For example, "gopls/v0.17.0".
+func parseMilestone(m *Milestone) (program, version string, ok bool) {
+	if m == nil {
+		return "", "", false
+	}
+	program, version, ok = morestrings.CutLast(m.Title, "/")
+	if !ok || program == "" || version == "" || version[0] != 'v' {
+		return "", "", false
+	}
+	return program, version, true
+}
+
 // stackID returns a 32-bit identifier for a stack
 // suitable for use in GitHub issue titles.
 func stackID(stack string) string {
@@ -819,16 +879,27 @@ type githubClient struct {
 	changes       []any  // slice of (addIssueComment | updateIssueBody)
 }
 
+func (cli *githubClient) takeChanges() []any {
+	r := cli.changes
+	cli.changes = nil
+	return r
+}
+
 // addIssueComment is a change for creating a comment on an issue.
 type addIssueComment struct {
 	number  int
 	comment string
 }
 
-// updateIssueBody is a change for modifying an existing issue's body.
-type updateIssueBody struct {
-	number int
-	body   string
+// updateIssue is a change for modifying an existing issue.
+// It includes the issue number and the fields that can be updated on a GitHub issue.
+// A JSON-marshaled updateIssue can be used as the body of the update request sent to GitHub.
+// See https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#update-an-issue.
+type updateIssue struct {
+	number      int    // issue number; must be unexported
+	Body        string `json:"body,omitempty"`
+	State       string `json:"state,omitempty"`        // "open" or "closed"
+	StateReason string `json:"state_reason,omitempty"` // "completed", "not_planned", "reopened"
 }
 
 // -- GitHub search --
@@ -888,24 +959,19 @@ func (cli *githubClient) searchIssues(label string) ([]*Issue, error) {
 	return results, nil
 }
 
-// updateIssueBody updates the body of the numbered issue.
-func (cli *githubClient) updateIssueBody(number int, body string) error {
+// updateIssue updates the numbered issue.
+func (cli *githubClient) updateIssue(update updateIssue) error {
 	if cli.divertChanges {
-		cli.changes = append(cli.changes, updateIssueBody{number, body})
+		cli.changes = append(cli.changes, update)
 		return nil
 	}
 
-	// https://docs.github.com/en/rest/issues/comments#update-an-issue
-	var payload struct {
-		Body string `json:"body"`
-	}
-	payload.Body = body
-	data, err := json.Marshal(payload)
+	data, err := json.Marshal(update)
 	if err != nil {
 		return err
 	}
 
-	url := fmt.Sprintf("https://api.github.com/repos/golang/go/issues/%d", number)
+	url := fmt.Sprintf("https://api.github.com/repos/golang/go/issues/%d", update.number)
 	if err := cli.requestChange("PATCH", url, data, http.StatusOK); err != nil {
 		return fmt.Errorf("updating issue: %v", err)
 	}
@@ -963,13 +1029,15 @@ func (cli *githubClient) requestChange(method, url string, data []byte, wantStat
 // See https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#list-repository-issues.
 
 type Issue struct {
-	Number    int
-	HTMLURL   string `json:"html_url"`
-	Title     string
-	State     string
-	User      *User
-	CreatedAt time.Time `json:"created_at"`
-	Body      string    // in Markdown format
+	Number      int
+	HTMLURL     string `json:"html_url"`
+	Title       string
+	State       string
+	StateReason string `json:"state_reason"`
+	User        *User
+	CreatedAt   time.Time `json:"created_at"`
+	Body        string    // in Markdown format
+	Milestone   *Milestone
 
 	// Set by readIssues.
 	predicate func(string) bool // matching predicate over stack text
@@ -983,6 +1051,10 @@ type User struct {
 	HTMLURL string `json:"html_url"`
 }
 
+type Milestone struct {
+	Title string
+}
+
 // -- pclntab --
 
 type FileLine struct {