internal/frontend: change symbol history logic
The frontend now displays different synopsis for the same symbol, if
they change based on build context.
For golang/go#37102
Change-Id: Id0887efbcc263434e5d25b9149e480c005d09f2d
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/316549
Trust: Julie Qiu <julie@golang.org>
Run-TryBot: Julie Qiu <julie@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/internal/frontend/symbol.go b/internal/frontend/symbol.go
index d7d2248..b636ab0 100644
--- a/internal/frontend/symbol.go
+++ b/internal/frontend/symbol.go
@@ -22,9 +22,6 @@
// Synopsis is the one line description of the symbol that is displayed.
Synopsis string
- // Link is the link to the symbol name on pkg.go.dev.
- Link string
-
// Section is the section that a symbol appears in.
Section internal.SymbolSection
@@ -32,6 +29,9 @@
// function, type, field or method.
Kind internal.SymbolKind
+ // Link is the link to the symbol name on pkg.go.dev.
+ Link string
+
// Children contain the child symbols for this symbol. This will
// only be populated when the SymbolType is "Type". For example, the
// children of net/http.Handler are FileServer, NotFoundHandler,
@@ -44,6 +44,9 @@
// build contexts, Builds will be nil.
Builds []string
+ // builds keeps track of build contexts used to generate Builds.
+ builds map[internal.BuildContext]bool
+
// New indicates that the symbol is new as of the version where it is
// present. For example, if type Client was introduced in v1.0.0 and
// Client.Timeout was introduced in v1.1.0, New will be false for Client
@@ -51,53 +54,112 @@
New bool
}
+func (s *Symbol) addBuilds(builds []internal.BuildContext) {
+ if s.builds == nil {
+ s.builds = map[internal.BuildContext]bool{}
+ }
+ for _, b := range builds {
+ s.builds[b] = true
+ }
+}
+
// symbolsForVersions returns an array of symbols for use in the VersionSummary
// of the specified version.
-func symbolsForVersion(pkgURLPath string, symbolsAtVersion map[string]*internal.UnitSymbol) [][]*Symbol {
- nameToSymbol := map[string]*Symbol{}
- for _, us := range symbolsAtVersion {
- builds := symbolBuilds(us)
- s, ok := nameToSymbol[us.Name]
- if !ok {
- s = &Symbol{
- Name: us.Name,
- Synopsis: us.Synopsis,
- Link: symbolLink(pkgURLPath, us.Name, us.BuildContexts()),
- Section: us.Section,
- Kind: us.Kind,
- New: true,
- Builds: builds,
+func symbolsForVersion(pkgURLPath string, symbolsAtVersion map[string]map[internal.SymbolMeta]*internal.UnitSymbol) [][]*Symbol {
+ nameToMetaToSymbol := map[string]map[internal.SymbolMeta]*Symbol{}
+ children := map[internal.SymbolMeta]*internal.UnitSymbol{}
+ for _, smToUs := range symbolsAtVersion {
+ for sm, us := range smToUs {
+ if sm.ParentName != sm.Name {
+ // For the children, keep track of them for later.
+ children[sm] = us
+ continue
}
- } else if !s.New && us.Kind == internal.SymbolKindType {
- // It's possible that a symbol was already created if this is a parent
- // symbol and we called addSymbol on the child symbol first. In that
- // case, a parent symbol would have been created where s.New is set to
- // false and s.Synopsis is set to the one created in createParent.
- // Update those fields instead of overwritting the struct, since the
- // struct's Children field would have already been populated.
- s.New = true
- s.Synopsis = us.Synopsis
+
+ metaToSym, ok := nameToMetaToSymbol[us.Name]
+ if !ok {
+ metaToSym = map[internal.SymbolMeta]*Symbol{}
+ nameToMetaToSymbol[us.Name] = metaToSym
+ }
+ s, ok := metaToSym[sm]
+ if !ok {
+ s = &Symbol{
+ Name: sm.Name,
+ Synopsis: sm.Synopsis,
+ Section: sm.Section,
+ Kind: sm.Kind,
+ Link: symbolLink(pkgURLPath, sm.Name, us.BuildContexts()),
+ New: true,
+ }
+ nameToMetaToSymbol[us.Name][sm] = s
+ }
+ s.addBuilds(us.BuildContexts())
}
- if us.ParentName == us.Name {
- // s is not a child symbol of a type, so add it to the map and
- // continue.
- nameToSymbol[us.Name] = s
+ }
+
+ for cm, cus := range children {
+ // Option 1: no parent exists
+ // - make one, add to map
+ // - append to parent
+ // Option 2: parent exists and supports child bc
+ // - append to parent
+ // Option 3 parent exists and does not support child bc
+ // - append to parent
+ cs := &Symbol{
+ Name: cm.Name,
+ Synopsis: cm.Synopsis,
+ Section: cm.Section,
+ Kind: cm.Kind,
+ Link: symbolLink(pkgURLPath, cm.Name, cus.BuildContexts()),
+ New: true,
+ }
+
+ parents, ok := nameToMetaToSymbol[cm.ParentName]
+ var found bool
+ if ok {
+ for _, ps := range parents {
+ for build := range ps.builds {
+ if cus.SupportsBuild(build) {
+ ps.Children = append(ps.Children, cs)
+ found = true
+ break
+ }
+ }
+ }
+ }
+ if found {
continue
}
- // s is a child symbol of a parent type, so append it to the Children field
- // of the parent type.
- parent, ok := nameToSymbol[us.ParentName]
- if !ok {
- parent = createParent(us, pkgURLPath)
- nameToSymbol[us.ParentName] = parent
+ // We did not find a parent, so create one.
+ ps := createParent(cus, pkgURLPath)
+ ps.Children = append(ps.Children, cs)
+ pm := internal.SymbolMeta{
+ Name: ps.Name,
+ ParentName: ps.Name,
+ Synopsis: ps.Synopsis,
+ Section: ps.Section,
+ Kind: ps.Kind,
}
- if len(parent.Builds) == len(s.Builds) {
- s.Builds = nil
+ ps.addBuilds(cus.BuildContexts())
+ nameToMetaToSymbol[pm.Name] = map[internal.SymbolMeta]*Symbol{
+ pm: ps,
}
- parent.Children = append(parent.Children, s)
}
- return sortSymbols(nameToSymbol)
+
+ var symbols []*Symbol
+ for _, mts := range nameToMetaToSymbol {
+ for _, s := range mts {
+ if len(s.builds) != len(internal.BuildContexts) {
+ for b := range s.builds {
+ s.Builds = append(s.Builds, fmt.Sprintf("%s/%s", b.GOOS, b.GOARCH))
+ }
+ sort.Strings(s.Builds)
+ }
+ symbols = append(symbols, s)
+ }
+ }
+ return sortSymbols(symbols)
}
func symbolLink(pkgURLPath, name string, builds []internal.BuildContext) string {
@@ -111,33 +173,20 @@
return fmt.Sprintf("%s?GOOS=%s#%s", pkgURLPath, builds[0].GOOS, name)
}
-func symbolBuilds(us *internal.UnitSymbol) []string {
- if us.InAll() {
- return nil
- }
- var builds []string
- for _, b := range us.BuildContexts() {
- builds = append(builds, b.String())
- }
- sort.Strings(builds)
- return builds
-}
-
// createParent creates a parent symbol for the provided unit symbol. This is
// used when us is a child of a symbol that may have been introduced at a
// different version. The symbol created will have New set to false, since this
// function is only used when a parent symbol is not found for the unit symbol,
// which means it was not introduced at the same version.
func createParent(us *internal.UnitSymbol, pkgURLPath string) *Symbol {
- builds := symbolBuilds(us)
s := &Symbol{
Name: us.ParentName,
Synopsis: fmt.Sprintf("type %s", us.ParentName),
- Link: symbolLink(pkgURLPath, us.ParentName, us.BuildContexts()),
Section: internal.SymbolSectionTypes,
Kind: internal.SymbolKindType,
- Builds: builds,
+ Link: symbolLink(pkgURLPath, us.ParentName, us.BuildContexts()),
}
+ s.addBuilds(us.BuildContexts())
return s
}
@@ -148,11 +197,10 @@
// order of (1) Fields (2) Constants (3) Variables (4) Functions and (5)
// Methods. For interfaces, child symbols are sorted in order of
// (1) Methods (2) Constants (3) Variables and (4) Functions.
-func sortSymbols(nameToSymbol map[string]*Symbol) [][]*Symbol {
+func sortSymbols(symbols []*Symbol) [][]*Symbol {
sm := map[internal.SymbolSection][]*Symbol{}
- for _, parent := range nameToSymbol {
+ for _, parent := range symbols {
sm[parent.Section] = append(sm[parent.Section], parent)
-
cm := map[internal.SymbolKind][]*Symbol{}
parent.Synopsis = strings.TrimSuffix(parent.Synopsis, "{ ... }")
for _, c := range parent.Children {
@@ -193,9 +241,6 @@
sort.Slice(syms, func(i, j int) bool {
return syms[i].Synopsis < syms[j].Synopsis
})
- for _, s := range syms {
- sort.Strings(s.Builds)
- }
}
// ParseVersionsDetails returns a map of versionToNameToUnitSymbol based on
diff --git a/internal/frontend/versions.go b/internal/frontend/versions.go
index 6978ac0..70edb1c 100644
--- a/internal/frontend/versions.go
+++ b/internal/frontend/versions.go
@@ -92,9 +92,9 @@
return nil, err
}
- outVersionToNameToUnitSymbol := map[string]map[string]*internal.UnitSymbol{}
+ sh := internal.NewSymbolHistory()
if experiment.IsActive(ctx, internal.ExperimentSymbolHistoryVersionsPage) {
- outVersionToNameToUnitSymbol, err = db.LegacyGetSymbolHistory(ctx, fullPath, modulePath)
+ sh, err = db.GetSymbolHistory(ctx, fullPath, modulePath)
if err != nil {
return nil, err
}
@@ -110,7 +110,7 @@
}
return constructUnitURL(versionPath, mi.ModulePath, linkVersion(mi.Version, mi.ModulePath))
}
- return buildVersionDetails(modulePath, versions, outVersionToNameToUnitSymbol, linkify), nil
+ return buildVersionDetails(modulePath, versions, sh, linkify), nil
}
// pathInVersion constructs the full import path of the package corresponding
@@ -139,7 +139,7 @@
// given versions MUST be sorted first by module path and then by semver.
func buildVersionDetails(currentModulePath string,
modInfos []*internal.ModuleInfo,
- versionToNameToSymbol map[string]map[string]*internal.UnitSymbol,
+ sh *internal.SymbolHistory,
linkify func(v *internal.ModuleInfo) string) *VersionsDetails {
// lists organizes versions by VersionListKey. Note that major version isn't
// sufficient as a key: there are packages contained in the same major
@@ -188,8 +188,8 @@
key.DeprecationComment = shortRationale(mi.DeprecationComment)
vs.Retracted = mi.Retracted
vs.RetractionRationale = shortRationale(mi.RetractionRationale)
- if nts, ok := versionToNameToSymbol[mi.Version]; ok {
- vs.Symbols = symbolsForVersion(linkify(mi), nts)
+ if sv := sh.SymbolsAtVersion(mi.Version); sv != nil {
+ vs.Symbols = symbolsForVersion(linkify(mi), sv)
}
if _, ok := lists[key]; !ok {
seenLists = append(seenLists, key)
diff --git a/internal/proxy/testdata/symbols@v1.1.0.txtar b/internal/proxy/testdata/symbols@v1.1.0.txtar
index 24ed59e..d80d5d7 100644
--- a/internal/proxy/testdata/symbols@v1.1.0.txtar
+++ b/internal/proxy/testdata/symbols@v1.1.0.txtar
@@ -106,3 +106,21 @@
func HelloJS() string {
return "Hello"
}
+
+-- multigoos/multigoos_windows.go --
+// +build windows
+
+package multigoos
+
+func CloseOnExec(foo string) error {
+ return nil
+}
+
+-- multigoos/multigoos_unix.go --
+// +build aix darwin dragonfly freebsd linux netbsd openbsd solaris
+
+package multigoos
+
+func CloseOnExec(num int) (int, error) {
+ return num, nil
+}
diff --git a/internal/proxy/testdata/symbols@v1.2.0.txtar b/internal/proxy/testdata/symbols@v1.2.0.txtar
index d26de3e..d755d2c 100644
--- a/internal/proxy/testdata/symbols@v1.2.0.txtar
+++ b/internal/proxy/testdata/symbols@v1.2.0.txtar
@@ -103,3 +103,29 @@
func HelloJS() string {
return "Hello"
}
+
+-- multigoos/multigoos_windows.go --
+// +build windows
+
+package multigoos
+
+func CloseOnExec(foo string) error {
+ return nil
+}
+
+-- multigoos/multigoos_unix.go --
+// +build aix darwin dragonfly freebsd linux netbsd openbsd solaris
+
+package multigoos
+
+func CloseOnExec(num int) (int, error) {
+ return num, nil
+}
+
+-- multigoos/multigoos_js.go --
+// +build js,wasm
+
+package multigoos
+
+func CloseOnExec(n int) {
+}
diff --git a/internal/testing/integration/data_frontend_versions_test.go b/internal/testing/integration/data_frontend_versions_test.go
index 768e2f5..ca0bb75 100644
--- a/internal/testing/integration/data_frontend_versions_test.go
+++ b/internal/testing/integration/data_frontend_versions_test.go
@@ -6,6 +6,68 @@
import "golang.org/x/pkgsite/internal/frontend"
+var versionsPageMultiGoos = []*frontend.VersionList{
+ {
+ VersionListKey: frontend.VersionListKey{
+ ModulePath: "example.com/symbols",
+ Major: "v1",
+ },
+ Versions: []*frontend.VersionSummary{
+ {
+ CommitTime: "Jan 30, 2019",
+ Link: "/example.com/symbols@v1.2.0/multigoos",
+ Retracted: false,
+ RetractionRationale: "",
+ Version: "v1.2.0",
+ IsMinor: true,
+ Symbols: [][]*frontend.Symbol{
+ {
+ {
+ Name: "CloseOnExec",
+ Synopsis: "func CloseOnExec(n int)",
+ Link: "/example.com/symbols@v1.2.0/multigoos?GOOS=js#CloseOnExec",
+ New: true,
+ Section: "Functions",
+ Kind: "Function",
+ Builds: []string{"js/wasm"},
+ },
+ },
+ },
+ },
+ {
+ CommitTime: "Jan 30, 2019",
+ Link: "/example.com/symbols@v1.1.0/multigoos",
+ Retracted: false,
+ RetractionRationale: "",
+ Version: "v1.1.0",
+ IsMinor: true,
+ Symbols: [][]*frontend.Symbol{
+ {
+ {
+ Name: "CloseOnExec",
+ Synopsis: "func CloseOnExec(foo string) error",
+ Link: "/example.com/symbols@v1.1.0/multigoos?GOOS=windows#CloseOnExec",
+ New: true,
+ Section: "Functions",
+ Kind: "Function",
+ Builds: []string{"windows/amd64"},
+ },
+ {
+ Name: "CloseOnExec",
+ Synopsis: "func CloseOnExec(num int) (int, error)",
+ Link: "/example.com/symbols@v1.1.0/multigoos?GOOS=darwin#CloseOnExec",
+ New: true,
+ Section: "Functions",
+ Kind: "Function",
+ Builds: []string{"darwin/amd64", "linux/amd64"},
+ },
+ },
+ },
+ },
+ },
+ },
+}
+
var versionsPageHello = []*frontend.VersionList{
{
VersionListKey: frontend.VersionListKey{
diff --git a/internal/testing/integration/frontend_versions_test.go b/internal/testing/integration/frontend_versions_test.go
index 48f06d1..63ffefc 100644
--- a/internal/testing/integration/frontend_versions_test.go
+++ b/internal/testing/integration/frontend_versions_test.go
@@ -11,6 +11,7 @@
"testing"
"github.com/google/go-cmp/cmp"
+ "github.com/google/go-cmp/cmp/cmpopts"
"golang.org/x/pkgsite/internal"
"golang.org/x/pkgsite/internal/experiment"
"golang.org/x/pkgsite/internal/frontend"
@@ -51,6 +52,7 @@
}{
{"versions page symbols - one version all symbols", modulePath, versionsPageSymbols},
{"versions page hello - multi GOOS", modulePath + "/hello", versionsPageHello},
+ {"versions page duplicate symbols multi GOOS", modulePath + "/multigoos", versionsPageMultiGoos},
} {
t.Run(test.name, func(t *testing.T) {
urlPath := fmt.Sprintf("/%s?tab=versions&m=json", test.pkgPath)
@@ -59,7 +61,7 @@
if err := json.Unmarshal([]byte(body), &got); err != nil {
t.Fatalf("json.Unmarshal: %v", err)
}
- if diff := cmp.Diff(test.want, got.ThisModule); diff != "" {
+ if diff := cmp.Diff(test.want, got.ThisModule, cmpopts.IgnoreUnexported(frontend.Symbol{})); diff != "" {
t.Errorf("mismatch (-want, got):\n%s", diff)
}
})
