Skip to content
This repository was archived by the owner on Sep 16, 2021. It is now read-only.

Commit c91cf13

Browse files
TypeScript Teamalexeagle
authored andcommitted
Fix generic comparisons on protobuf messages
Generated protobuf messages contain internal data structures that general purpose comparison functions (e.g., reflect.DeepEqual, pretty.Compare, etc) do not properly compare. It is already the case today that these functions may report a difference when two messages are actually semantically equivalent. Fix all usages by either calling proto.Equal directly if the top-level types are themselves proto.Message, or by calling cmp.Equal with the cmp.Comparer(proto.Equal) option specified. This option teaches cmp to use proto.Equal anytime it encounters proto.Message types. PiperOrigin-RevId: 282412693
1 parent 62bbdc8 commit c91cf13

29 files changed

+958
-322
lines changed

internal/ts_repositories.bzl

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,12 @@ def ts_setup_dev_workspace():
4646

4747
ts_setup_workspace()
4848

49+
go_repository(
50+
name = "com_github_google_go_cmp",
51+
commit = "2d0692c2e9617365a95b295612ac0d4415ba4627",
52+
importpath = "github.com/google/go-cmp",
53+
)
54+
4955
go_repository(
5056
name = "com_github_kylelemons_godebug",
5157
commit = "d65d576e9348f5982d7f6d83682b694e731a45c6",

ts_auto_deps/analyze/BUILD.bazel

Lines changed: 0 additions & 37 deletions
This file was deleted.

ts_auto_deps/analyze/analyze.go

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Package analyze uses bazel query to determine and locate missing imports
1+
// Package analyze uses blaze query to determine and locate missing imports
22
// in TypeScript source files.
33
package analyze
44

@@ -10,13 +10,13 @@ import (
1010
"regexp"
1111
"strings"
1212

13-
"github.com/bazelbuild/buildtools/edit"
13+
"google3/net/proto2/go/proto"
14+
"google3/third_party/bazel_buildifier/edit/edit"
1415
"github.com/bazelbuild/rules_typescript/ts_auto_deps/platform"
1516
"github.com/bazelbuild/rules_typescript/ts_auto_deps/workspace"
16-
"github.com/golang/protobuf/proto"
1717

18-
appb "github.com/bazelbuild/buildtools/build_proto"
19-
arpb "github.com/bazelbuild/rules_typescript/ts_auto_deps/proto"
18+
appb "google3/third_party/bazel/src/main/protobuf/build_go_proto"
19+
arpb "google3/third_party/bazel_rules/rules_typescript/ts_auto_deps/proto/analyze_result_go_proto"
2020
)
2121

2222
var (
@@ -97,7 +97,7 @@ func New(loader TargetLoader) *Analyzer {
9797

9898
// Analyze generates a dependency report for each target label in labels.
9999
//
100-
// dir is the directory that ts_auto_deps should execute in. Must be a sub-directory
100+
// dir is the directory that taze should execute in. Must be a sub-directory
101101
// of the workspace root.
102102
func (a *Analyzer) Analyze(ctx context.Context, dir string, labels []string) ([]*arpb.DependencyReport, error) {
103103
if len(labels) == 0 {
@@ -124,14 +124,14 @@ func (a *Analyzer) Analyze(ctx context.Context, dir string, labels []string) ([]
124124
return a.generateReports(labels, resolved)
125125
}
126126

127-
// resolvedTarget represents a Bazel target and all resolved information.
127+
// resolvedTarget represents a Blaze target and all resolved information.
128128
type resolvedTarget struct {
129129
label string
130130
// A map of all existing dependencies on a target at the time of analysis.
131131
// The keys are labels and the values are thes loaded target.
132132
dependencies map[string]*appb.Rule
133133
// A map of source file paths to their imports.
134-
imports map[string][]*ts_auto_depsImport
134+
imports map[string][]*tazeImport
135135
// rule is the original rule the target was constructed from.
136136
rule *appb.Rule
137137
// missingSources are source files which could not be opened on disk.
@@ -147,7 +147,7 @@ type resolvedTarget struct {
147147
// setSources sets the sources on t. It returns an error if one of the srcs of
148148
// t's rule isn't in loadedSrcs. It also sorts the sources into literal and
149149
// generated sources, setting literalSourcePaths and generatedSourcePaths.
150-
// Returns an error if all the sources are generated - ts_auto_deps can't read the
150+
// Returns an error if all the sources are generated - taze can't read the
151151
// import statements to determine deps.
152152
func (t *resolvedTarget) setSources(loadedSrcs map[string]*appb.Target) error {
153153
for _, label := range listAttribute(t.rule, "srcs") {
@@ -220,7 +220,7 @@ func newResolvedTarget(r *appb.Rule) *resolvedTarget {
220220
return &resolvedTarget{
221221
label: r.GetName(),
222222
dependencies: make(map[string]*appb.Rule),
223-
imports: make(map[string][]*ts_auto_depsImport),
223+
imports: make(map[string][]*tazeImport),
224224
rule: r,
225225
sources: make(map[string]*appb.Target),
226226
}
@@ -264,7 +264,7 @@ func (a *Analyzer) resolveImportsForTargets(ctx context.Context, currentPkg, roo
264264
return nil, err
265265
}
266266
}
267-
// only extract the imports out of the literal sources, since ts_auto_deps can't
267+
// only extract the imports out of the literal sources, since taze can't
268268
// see the contents of generated files
269269
allLiteralSrcPaths, err := getAllLiteralSrcPaths(targets)
270270
if err != nil {
@@ -302,7 +302,7 @@ func (a *Analyzer) resolveImportsForTargets(ctx context.Context, currentPkg, roo
302302
func (a *Analyzer) resolveImports(ctx context.Context, currentPkg, root string, targets map[string]*resolvedTarget) error {
303303
for _, target := range targets {
304304
var paths []string
305-
needingResolution := make(map[string][]*ts_auto_depsImport)
305+
needingResolution := make(map[string][]*tazeImport)
306306
for _, imports := range target.imports {
307307
handlingImports:
308308
for _, imp := range imports {
@@ -363,7 +363,7 @@ var ambientModuleDeclRE = regexp.MustCompile("(?m)^\\s*declare\\s+module\\s+['\"
363363
//
364364
// If the import already has a knownTarget, findRuleProvidingImport will
365365
// return the knownTarget.
366-
func (a *Analyzer) findExistingDepProvidingImport(ctx context.Context, root string, rt *resolvedTarget, i *ts_auto_depsImport) (string, error) {
366+
func (a *Analyzer) findExistingDepProvidingImport(ctx context.Context, root string, rt *resolvedTarget, i *tazeImport) (string, error) {
367367
if i.knownTarget != "" {
368368
return i.knownTarget, nil
369369
}
@@ -545,17 +545,17 @@ func (a *Analyzer) generateReport(target *resolvedTarget) (*arpb.DependencyRepor
545545
// TypeScript declarations might declare arbitrary global symbols, so it
546546
// is impossible to detect reliably if the import is being used (without
547547
// compiling, at least). Report that the rule has no explicit import as a
548-
// warning, so that ts_auto_deps can decide to import remove or not based on a
548+
// warning, so that taze can decide to import remove or not based on a
549549
// flag.
550550
warning := fmt.Sprintf("WARNING: %s: keeping possibly used %s '%s'", rule.GetLocation(), class, label)
551551
report.Feedback = append(report.Feedback, warning)
552552
case "css_library":
553-
// Similar to ts_declaration, ts_auto_deps can't reliably detect if css_library
554-
// imports are being used, since ts_auto_deps can't currently parse @requirecss
553+
// Similar to ts_declaration, taze can't reliably detect if css_library
554+
// imports are being used, since taze can't currently parse @requirecss
555555
// annotations. Unlike ts_declaration, there's no flag to remove them, so
556556
// there's no need to report a warning.
557557
default:
558-
// The contents of generated files aren't visible, so ts_auto_deps can't discover
558+
// The contents of generated files aren't visible, so taze can't discover
559559
// the import statements/deps that they contain. To be safe, don't remove
560560
// any unused deps, since they might be used by the generated file(s).
561561
if len(target.generatedSourcePaths) == 0 {

ts_auto_deps/analyze/analyze_test.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,14 @@ import (
1010
"strings"
1111
"testing"
1212

13+
"google3/net/proto2/go/proto"
1314
"github.com/bazelbuild/rules_typescript/ts_auto_deps/platform"
14-
"github.com/golang/protobuf/proto"
15-
"github.com/kylelemons/godebug/pretty"
15+
"google3/third_party/golang/cmp/cmp"
16+
"google3/third_party/golang/cmp/cmpopts/cmpopts"
17+
"google3/third_party/golang/godebug/pretty/pretty"
1618

17-
appb "github.com/bazelbuild/buildtools/build_proto"
18-
arpb "github.com/bazelbuild/rules_typescript/ts_auto_deps/proto"
19+
appb "google3/third_party/bazel/src/main/protobuf/build_go_proto"
20+
arpb "google3/third_party/bazel_rules/rules_typescript/ts_auto_deps/proto/analyze_result_go_proto"
1921
)
2022

2123
var (
@@ -204,7 +206,7 @@ func TestUnnecessaryDependencies(t *testing.T) {
204206
func TestNecessaryDependencies(t *testing.T) {
205207
tests := [][]string{
206208
[]string{"import x from 'b/target';"},
207-
[]string{"// ts_auto_deps: x from //b:b_lib"},
209+
[]string{"// taze: x from //b:b_lib"},
208210
[]string{"export x from 'b/target';"},
209211
}
210212
for _, test := range tests {
@@ -616,7 +618,7 @@ func TestSetSources(t *testing.T) {
616618
t.Errorf("got err %q, expected %q", err, test.err)
617619
}
618620

619-
if diff := pretty.Compare(rt.sources, test.expected); err == nil && diff != "" {
621+
if diff := cmp.Diff(rt.sources, test.expected, cmpopts.EquateEmpty(), cmp.Comparer(proto.Equal)); err == nil && diff != "" {
620622
t.Errorf("failed to set correct sources: (-got, +want)\n%s", diff)
621623
}
622624
})
@@ -646,7 +648,7 @@ func createFile(path string, content ...string) error {
646648
return ioutil.WriteFile(path, []byte(strings.Join(content, "\n")), 0666)
647649
}
648650

649-
// This method creates a WORKSPACE file in the root of the Bazel test
651+
// This method creates a WORKSPACE file in the root of the Blaze test
650652
// directory. This allows the tests to resolve the root path of the
651653
// workspace by looking for the WORKSPACE file on disk.
652654
func createWorkspaceFile() error {

ts_auto_deps/analyze/imports.go

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,13 @@ import (
1111
"github.com/bazelbuild/rules_typescript/ts_auto_deps/workspace"
1212
)
1313

14-
// ts_auto_depsImport represents a single import in a TypeScript source.
15-
type ts_auto_depsImport struct {
14+
// tazeImport represents a single import in a TypeScript source.
15+
type tazeImport struct {
1616
// importPath can be an ES6 path ('./foo/bar'), but also a namespace ('goog:...').
1717
// This is the import path as it appears in the TypeScript source.
1818
importPath string
19-
// knownTarget is the (fully qualified) bazel target providing importPath.
20-
// It's either found by locateMissingTargets or taken from a ts_auto_deps comment.
19+
// knownTarget is the (fully qualified) blaze target providing importPath.
20+
// It's either found by locateMissingTargets or taken from a taze comment.
2121
knownTarget string
2222
location sourceLocation
2323
}
@@ -30,7 +30,7 @@ type ts_auto_depsImport struct {
3030
// these imports depends on the dependencies of the target the source
3131
// location is a member of. For example, an import of 'foo/bar' would have
3232
// no resolvedPath.
33-
func (i *ts_auto_depsImport) resolvedPath() string {
33+
func (i *tazeImport) resolvedPath() string {
3434
if strings.HasPrefix(i.importPath, "./") || strings.HasPrefix(i.importPath, "../") {
3535
// If the import is relative to the source location, use the source
3636
// location to form a "canonical" path from the root.
@@ -55,9 +55,9 @@ type sourceLocation struct {
5555
//
5656
// paths should be relative to root. The root will be joined to each path
5757
// to construct a physical path to each file.
58-
func extractAllImports(root string, paths []string) (map[string][]*ts_auto_depsImport, []error) {
58+
func extractAllImports(root string, paths []string) (map[string][]*tazeImport, []error) {
5959
debugf("extracting imports from TypeScript files relative to %q: %q", root, paths)
60-
allImports := make(map[string][]*ts_auto_depsImport)
60+
allImports := make(map[string][]*tazeImport)
6161
var (
6262
errors []error
6363
mutex sync.Mutex
@@ -84,7 +84,7 @@ func extractAllImports(root string, paths []string) (map[string][]*ts_auto_depsI
8484

8585
// extractImports extracts the TypeScript imports from a single file. path
8686
// should be a path from the root to the file.
87-
func extractImports(root, path string) ([]*ts_auto_depsImport, error) {
87+
func extractImports(root, path string) ([]*tazeImport, error) {
8888
d, err := ioutil.ReadFile(filepath.Join(root, path))
8989
if err != nil {
9090
return nil, err
@@ -93,7 +93,7 @@ func extractImports(root, path string) ([]*ts_auto_depsImport, error) {
9393
}
9494

9595
const (
96-
ts_auto_depsFrom = `^[ \t]*//[ \t]+ts_auto_deps:[^\n]*?from[ \t]+(?P<Target>//\S+)$`
96+
tazeFrom = `^[ \t]*//[ \t]+taze:[^\n]*?from[ \t]+(?P<Target>//\S+)$`
9797
importPreface = `^[ \t]*(?:import|export)\b\s*`
9898
wildcardTerm = `\*(?:\s*as\s+\S+)?` // "as..." is optional to match exports.
9999
identifiersClause = `(?:\{[^}]*\}|\S+|` + wildcardTerm + `)`
@@ -103,26 +103,26 @@ const (
103103
)
104104

105105
var importRE = regexp.MustCompile("(?ms)" +
106-
"(?:" + ts_auto_depsFrom + ")|" +
106+
"(?:" + tazeFrom + ")|" +
107107
"(?:" + importPreface + symbolsTerm + url + namespaceComment + ")")
108108

109-
// parseImports scans contents for imports (ES6 modules, ts_auto_deps comments), and
110-
// returns a list of ts_auto_depsImports. knownTarget is already filled in for imports
111-
// that have ts_auto_deps comments.
112-
func parseImports(sourcePath string, contents []byte) []*ts_auto_depsImport {
113-
var imports []*ts_auto_depsImport
109+
// parseImports scans contents for imports (ES6 modules, taze comments), and
110+
// returns a list of tazeImports. knownTarget is already filled in for imports
111+
// that have taze comments.
112+
func parseImports(sourcePath string, contents []byte) []*tazeImport {
113+
var imports []*tazeImport
114114
lastOffset := 0
115115
line := 1
116116
column := 1
117117
for _, matchIndices := range importRE.FindAllSubmatchIndex(contents, -1) {
118-
imp := &ts_auto_depsImport{}
118+
imp := &tazeImport{}
119119
imports = append(imports, imp)
120120
// matchIndices[0, 1]: full RE match
121121
imp.location.sourcePath = sourcePath
122122
for lastOffset < matchIndices[1] {
123123
// Iterate to the *end* of the import statement.
124-
// The ts_auto_deps comment must be placed at the end of the "import" statement.
125-
// This offset has to be exactly the end of the import for ts_auto_deps later on
124+
// The taze comment must be placed at the end of the "import" statement.
125+
// This offset has to be exactly the end of the import for taze later on
126126
// to insert the '// from' comment in the correct line.
127127
column++
128128
if contents[lastOffset] == '\n' {
@@ -135,7 +135,7 @@ func parseImports(sourcePath string, contents []byte) []*ts_auto_depsImport {
135135
imp.location.length = matchIndices[1] - matchIndices[0]
136136
imp.location.line = line
137137
if matchIndices[2] >= 0 {
138-
// matchIndices[2, 3]: Target for a // ts_auto_deps: ... from ... comment.
138+
// matchIndices[2, 3]: Target for a // taze: ... from ... comment.
139139
imp.knownTarget = string(contents[matchIndices[2]:matchIndices[3]])
140140
} else {
141141
// matchIndices[4, 5]: URL in import x from 'url';

ts_auto_deps/analyze/imports_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import (
44
"fmt"
55
"testing"
66

7-
"github.com/kylelemons/godebug/pretty"
7+
"google3/third_party/golang/godebug/pretty/pretty"
88
)
99

1010
func TestParseImports(t *testing.T) {
@@ -41,8 +41,8 @@ func TestParseImports(t *testing.T) {
4141
{"export*from'no whitespace';", "no whitespace", ""},
4242
{"export{}from'no whitespace';", "no whitespace", ""},
4343
// Comments
44-
{"x;\n// ts_auto_deps: ng from //some/global:rule\ny;", "", "//some/global:rule"},
45-
{"// ts_auto_deps: ng from //foo/bar from //some/global:rule", "", "//some/global:rule"},
44+
{"x;\n// taze: ng from //some/global:rule\ny;", "", "//some/global:rule"},
45+
{"// taze: ng from //foo/bar from //some/global:rule", "", "//some/global:rule"},
4646
}
4747

4848
for i, tst := range tests {

0 commit comments

Comments
 (0)