From 021c30d89bb96b925b41a4a853fc367311a39e8b Mon Sep 17 00:00:00 2001 From: Paul Cody Johnston Date: Sun, 3 Dec 2023 14:49:50 -0700 Subject: [PATCH] Add gazelle:resolve_file_symbol_name directive (#109) * Add gazelle:resolve_file_symbol_name directive * integrate scala_rule with ShouldResolveFileSymbolName * fix tests] * docs --- README.md | 109 ++++++++++++++++-- language/scala/BUILD.bazel | 2 + language/scala/existing_scala_rule.go | 7 +- language/scala/language.go | 7 +- language/scala/language_test.go | 7 +- language/scala/scala_config.go | 67 +++++++++-- language/scala/scala_config_test.go | 52 +++++++++ language/scala/scala_rule.go | 3 + .../scala/testdata/java_provider/BUILD.out | 1 - .../scala/testdata/maven_provider/BUILD.out | 3 - .../testdata/override_provider/BUILD.out | 1 - .../testdata/protobuf_provider/BUILD.out | 3 - 12 files changed, 227 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index 30cc53ce..e6f2a95d 100644 --- a/README.md +++ b/README.md @@ -49,8 +49,10 @@ - [`gazelle:resolve`](#gazelleresolve) - [`gazelle:resolve_with`](#gazelleresolve_with) - [`gazelle:resolve_kind_rewrite_name`](#gazelleresolve_kind_rewrite_name) + - [`gazelle:resolve_file_symbol_name`](#gazelleresolve_file_symbol_name) - [`gazelle:annotate`](#gazelleannotate) - [`imports`](#imports) + - [`exports`](#exports) - [Import Resolution Procedure](#import-resolution-procedure) - [How Required Imports are Calculated](#how-required-imports-are-calculated) - [Rule](#rule) @@ -832,6 +834,70 @@ This tells the extension _"if you find a rule with kind `my_scala_app`, rewrite the label name to name + `"_lib"`, using the magic token `%{name}` as a placeholder."_ +### `gazelle:resolve_file_symbol_name` + +This directive can be used to resolve free names listed in a scala file against +the current file symbol scope. To inspect the `names` of a file, take a look at the file parse cache. For example: + +```json +{ + "label": "//common/utils/logging/scala", + "kind": "scala_library", + "files": [ + { + "filename": "src/LogField.scala", + "imports": [ + "com.typesafe.scalalogging.LazyLogging", + "net.logstash.logback.marker.MapEntriesAppendingMarker", + "net.logstash.logback.marker.ObjectAppendingMarker", + "scala.jdk.CollectionConverters._" + ], + "packages": [ + "common.utils.logging" + ], + "objects": [ + "common.utils.logging.LogField", + "common.utils.logging.LogFields" + ], + "traits": [ + "common.utils.logging.LogField" + ], + "names": [ + "LazyLogging", + "LogField", + "LogFields", + "MapEntriesAppendingMarker", + "ObjectAppendingMarker", + "String", + "apply", + "fieldName", + "fieldValue", + "fieldValue.toString", + "name", + ], + "extends": { + "object trumid.common.utils.logging.LogField": { + "classes": [ + "com.typesafe.scalalogging.LazyLogging" + ] + } + } + } + ], + "sha256": "3ee80930372ea846ebb48e55eb76d55fed89b6af5f05d08f98b38045eb0464d6", + "parseTimeMillis": "3" +}, +``` + +In this case, if a dependency was missing from the `deps` list, but would be +corrected by resolving `ObjectAppendingMarker` (but not +`MapEntriesAppendingMarker`, for example purposes), one could instruct the +resolver to try and resolve it selectively via: + +``` +# gazelle:resolve_file_symbol_name LogField.scala +ObjectAppendingMarker -MapEntriesAppendingMarker +``` + ### `gazelle:annotate` @@ -852,26 +918,38 @@ Generates: ```bazel scala_binary( name = "app", - # ❌ AbstractServiceBase symbol not found (EXTENDS of foo.allocation.Main) - # ✅ akka.NotUsed @maven//:com_typesafe_akka_akka_actor_2_12 (DIRECT of BusinessFlows.scala) - # ✅ java.time.format.DateTimeFormatter NO-LABEL (DIRECT of RequestHandler.scala) - # ✅ scala.concurrent.ExecutionContext @maven//:org_scala_lang_scala_library (DIRECT of RequestHandler.scala) + # import: ❌ AbstractServiceBase symbol not found (EXTENDS of foo.allocation.Main) + # import: ✅ akka.NotUsed @maven//:com_typesafe_akka_akka_actor_2_12 (DIRECT of BusinessFlows.scala) + # import: ✅ java.time.format.DateTimeFormatter NO-LABEL (DIRECT of RequestHandler.scala) + # import: ✅ scala.concurrent.ExecutionContext @maven//:org_scala_lang_scala_library (DIRECT of RequestHandler.scala) srcs = glob(["src/main/**/*.scala"]), main_class = "foo.allocation.Main", ) ``` +#### `exports` + +This adds a list of comments to the `srcs` attribute detailing the provided +exports and how they resolved. Example: + +``` +# gazelle:annotate exports +``` + # Import Resolution Procedure ## How Required Imports are Calculated ### Rule -If the rule has `main_class` attribute, that name is added to the imports (type `MAIN_CLASS`). +If the rule has `main_class` attribute, that name is added to the imports (type +`MAIN_CLASS`). -The remainder of rule imports are collected from file imports for all `.scala` source files in the rule. +The remainder of rule imports are collected from file imports for all `.scala` +source files in the rule. -Once this initial set of imports are gathered, the transitive set of required symbol are collected from: +Once this initial set of imports are gathered, the transitive set of required +symbol are collected from: - `extends` clauses (type `EXTENDS`) - imports matching a `gazelle:resolve_with` directive (type `IMPLICIT`). @@ -885,7 +963,8 @@ The imports for a file are collected as follows: The `.scala` file is parsed: - Import statements are collected, including nested imports. -- a set of *names* are collected by traversing the body of the AST. Some of these names are function calls, some of them are types, etc. +- a set of *names* are collected by traversing the body of the AST. Some of + these names are function calls, some of them are types, etc. Symbols named in import statements are added to imports (type `DIRECT`). @@ -896,7 +975,19 @@ A trie of the symbols in scope for the file is built from: - the file package(s) - wildcard imports -Then, all *names* in the file are tested against the file scope. Matching symbols are added to the imports (type `RESOLVED_NAME`). +"Names" represent a variety of things in a scala file. It might be a class +instatiation (e.g `new Foo()`), a static method call `doSomething()`, and other +similar names. + +In an earlier implementation of scala-gazelle, all *names* in the file were +tested against the file scope and matching symbols are added to the imports +list (of type `RESOLVED_NAME`). + +The drawback of that approach is that it was imprecise, potentially leading to +false positive import resolutions (and unnecessary and/or incorrect `deps` +list). + +Now, name resolution is an opt-in feature using the gazelle directive `gazelle:resolve_file_symbol_name` directive. ## How Required Imports are Resolved diff --git a/language/scala/BUILD.bazel b/language/scala/BUILD.bazel index 069ea441..789c93e0 100644 --- a/language/scala/BUILD.bazel +++ b/language/scala/BUILD.bazel @@ -51,6 +51,7 @@ go_library( "@bazel_gazelle//rule:go_default_library", "@build_stack_rules_proto//pkg/protoc", "@com_github_bazelbuild_buildtools//build:go_default_library", + "@com_github_bmatcuk_doublestar_v4//:doublestar", "@com_github_pcj_mobyprogress//:mobyprogress", ], ) @@ -83,6 +84,7 @@ go_test( "language_test.go", "loads_test.go", "scala_config_test.go", + "scala_package_test.go", "scala_rule_test.go", ], data = [":gazelle"] + glob(["testdata/**"]), diff --git a/language/scala/existing_scala_rule.go b/language/scala/existing_scala_rule.go index 251a1a3d..369d1c7a 100644 --- a/language/scala/existing_scala_rule.go +++ b/language/scala/existing_scala_rule.go @@ -29,9 +29,10 @@ func init() { mustRegister("@io_bazel_rules_scala//scala:scala.bzl", "scala_test") } -// existingScalaRuleProvider implements RuleResolver for scala-like rules that are -// already in the build file. It does not create any new rules. This rule -// implementation is used to parse files named in 'srcs' and update 'deps'. +// existingScalaRuleProvider implements RuleResolver for scala-like rules that +// are already in the build file. It does not create any new rules. This rule +// implementation is used to parse files named in 'srcs' and update 'deps' (and +// optionally, exports). type existingScalaRuleProvider struct { load, name string } diff --git a/language/scala/language.go b/language/scala/language.go index fcc82503..fe5cfcd0 100644 --- a/language/scala/language.go +++ b/language/scala/language.go @@ -87,12 +87,13 @@ func (sl *scalaLang) Name() string { return scalaLangName } // KnownDirectives implements part of the language.Language interface func (*scalaLang) KnownDirectives() []string { return []string{ - scalaRuleDirective, + resolveConflictsDirective, + resolveFileSymbolName, resolveGlobDirective, + resolveKindRewriteNameDirective, resolveWithDirective, - resolveConflictsDirective, scalaAnnotateDirective, - resolveKindRewriteNameDirective, + scalaRuleDirective, } } diff --git a/language/scala/language_test.go b/language/scala/language_test.go index 6c9a8038..5440a683 100644 --- a/language/scala/language_test.go +++ b/language/scala/language_test.go @@ -15,10 +15,11 @@ func ExampleLanguage_KnownDirectives() { fmt.Println(d) } // output: - // scala_rule + // resolve_conflicts + // resolve_file_symbol_name // resolve_glob + // resolve_kind_rewrite_name // resolve_with - // resolve_conflicts // scala_annotate - // resolve_kind_rewrite_name + // scala_rule } diff --git a/language/scala/scala_config.go b/language/scala/scala_config.go index 85fba769..efe05776 100644 --- a/language/scala/scala_config.go +++ b/language/scala/scala_config.go @@ -11,6 +11,7 @@ import ( "github.com/bazelbuild/bazel-gazelle/resolve" "github.com/bazelbuild/bazel-gazelle/rule" "github.com/bazelbuild/buildtools/build" + "github.com/bmatcuk/doublestar/v4" "github.com/stackb/scala-gazelle/pkg/collections" "github.com/stackb/scala-gazelle/pkg/resolver" @@ -31,20 +32,22 @@ const ( resolveGlobDirective = "resolve_glob" resolveConflictsDirective = "resolve_conflicts" resolveWithDirective = "resolve_with" + resolveFileSymbolName = "resolve_file_symbol_name" resolveKindRewriteNameDirective = "resolve_kind_rewrite_name" ) // scalaConfig represents the config extension for the a scala package. type scalaConfig struct { - config *config.Config - rel string - universe resolver.Universe - overrides []*overrideSpec - implicitImports []*implicitImportSpec - rules map[string]*scalarule.Config - labelNameRewrites map[string]resolver.LabelNameRewriteSpec - annotations map[annotation]interface{} - conflictResolvers []resolver.ConflictResolver + config *config.Config + rel string + universe resolver.Universe + overrides []*overrideSpec + implicitImports []*implicitImportSpec + resolveFileSymbolNames []*resolveFileSymbolNameSpec + rules map[string]*scalarule.Config + labelNameRewrites map[string]resolver.LabelNameRewriteSpec + annotations map[annotation]interface{} + conflictResolvers []resolver.ConflictResolver } // newScalaConfig initializes a new scalaConfig. @@ -102,6 +105,9 @@ func (c *scalaConfig) clone(config *config.Config, rel string) *scalaConfig { if c.conflictResolvers != nil { clone.conflictResolvers = c.conflictResolvers[:] } + if c.resolveFileSymbolNames != nil { + clone.resolveFileSymbolNames = c.resolveFileSymbolNames[:] + } return clone } @@ -149,6 +155,8 @@ func (c *scalaConfig) parseDirectives(directives []rule.Directive) (err error) { c.parseResolveGlobDirective(d) case resolveWithDirective: c.parseResolveWithDirective(d) + case resolveFileSymbolName: + c.parseResolveFileSymbolNames(d) case resolveKindRewriteNameDirective: c.parseResolveKindRewriteNameDirective(d) case resolveConflictsDirective: @@ -215,6 +223,23 @@ func (c *scalaConfig) parseResolveWithDirective(d rule.Directive) { }) } +func (c *scalaConfig) parseResolveFileSymbolNames(d rule.Directive) { + parts := strings.Fields(d.Value) + if len(parts) < 2 { + log.Printf("invalid gazelle:%s directive: expected [FILENAME_PATTERN [+|-]SYMBOLS...], got %v", resolveKindRewriteNameDirective, parts) + return + } + pattern := parts[0] + + for _, part := range parts[1:] { + intent := collections.ParseIntent(part) + c.resolveFileSymbolNames = append(c.resolveFileSymbolNames, &resolveFileSymbolNameSpec{ + pattern: pattern, + symbolName: *intent, + }) + } +} + func (c *scalaConfig) parseResolveKindRewriteNameDirective(d rule.Directive) { parts := strings.Fields(d.Value) if len(parts) != 3 { @@ -318,6 +343,23 @@ func (c *scalaConfig) shouldAnnotateExports() bool { return ok } +// ShouldResolveFileSymbolName tests whether the given symbol name pattern +// should be resolved within the scope of the given filename pattern. +// resolveFileSymbolNameSpecs represent a whitelist; if no patterns match, false +// is returned. +func (c *scalaConfig) ShouldResolveFileSymbolName(filename, name string) bool { + for _, spec := range c.resolveFileSymbolNames { + if ok, _ := doublestar.Match(spec.pattern, filename); !ok { + continue + } + if ok, _ := doublestar.Match(spec.symbolName.Value, name); !ok { + continue + } + return spec.symbolName.Want + } + return false +} + func (c *scalaConfig) Comment() build.Comment { return build.Comment{Token: "# " + c.String()} } @@ -487,6 +529,13 @@ type implicitImportSpec struct { deps []string } +type resolveFileSymbolNameSpec struct { + // pattern is the filename glob pattern to test + pattern string + // symbol is the symbol name to resolve + symbolName collections.Intent +} + func parseAnnotation(val string) annotation { switch val { case "imports": diff --git a/language/scala/scala_config_test.go b/language/scala/scala_config_test.go index b2cc457f..7d7ba4db 100644 --- a/language/scala/scala_config_test.go +++ b/language/scala/scala_config_test.go @@ -239,6 +239,58 @@ func TestScalaConfigParseImplicitImportDirective(t *testing.T) { } } +func TestScalaConfigParseResolveFileSymbolName(t *testing.T) { + for name, tc := range map[string]struct { + directives []rule.Directive + filename string + names []string + want []bool + wantErr error + }{ + "degenerate": { + want: []bool{}, + }, + "exact matches": { + directives: []rule.Directive{ + {Key: resolveFileSymbolName, Value: "filename.scala +foo -bar"}, + }, + filename: "filename.scala", + names: []string{"foo", "bar"}, + want: []bool{true, false}, + }, + "glob matches": { + directives: []rule.Directive{ + {Key: resolveFileSymbolName, Value: "*.scala +foo* -bar*"}, + }, + filename: "filename.scala", + names: []string{"foo", "foox", "bar", "barx"}, + want: []bool{true, true, false, false}, + }, + "no match": { + directives: []rule.Directive{ + {Key: resolveFileSymbolName, Value: "*.scala +foo* -bar*"}, + }, + filename: "filename.scala", + names: []string{"baz"}, + want: []bool{false}, + }, + } { + t.Run(name, func(t *testing.T) { + sc, err := newTestScalaConfig(t, mocks.NewUniverse(t), "", tc.directives...) + if testutil.ExpectError(t, tc.wantErr, err) { + return + } + got := []bool{} + for _, name := range tc.names { + got = append(got, sc.ShouldResolveFileSymbolName(tc.filename, name)) + } + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("(-want +got):\n%s", diff) + } + }) + } +} + func TestScalaConfigParseScalaAnnotate(t *testing.T) { for name, tc := range map[string]struct { directives []rule.Directive diff --git a/language/scala/scala_rule.go b/language/scala/scala_rule.go index d3a8f3bc..71a41938 100644 --- a/language/scala/scala_rule.go +++ b/language/scala/scala_rule.go @@ -352,6 +352,9 @@ func (r *scalaRule) fileImports(file *sppb.File, imports resolver.ImportMap) { // resolve symbols named in the file. For each one we find, add an import. for _, name := range file.Names { + if !r.ctx.scalaConfig.ShouldResolveFileSymbolName(file.Filename, name) { + continue + } if sym, ok := scope.GetSymbol(name); ok { putImport(resolver.NewResolvedNameImport(sym.Name, file, name, sym)) } else { diff --git a/language/scala/testdata/java_provider/BUILD.out b/language/scala/testdata/java_provider/BUILD.out index 0c1f56fc..12f87b31 100644 --- a/language/scala/testdata/java_provider/BUILD.out +++ b/language/scala/testdata/java_provider/BUILD.out @@ -5,7 +5,6 @@ load("@io_bazel_rules_scala//scala:scala.bzl", "scala_binary") scala_binary( name = "app", - # import: ❌ a.b.c name not found (IMPORT_KIND_UNKNOWN) # import: ✅ java.util.Map NO-LABEL (DIRECT of Main.scala) srcs = ["Main.scala"], ) diff --git a/language/scala/testdata/maven_provider/BUILD.out b/language/scala/testdata/maven_provider/BUILD.out index 5fa90696..d281b773 100644 --- a/language/scala/testdata/maven_provider/BUILD.out +++ b/language/scala/testdata/maven_provider/BUILD.out @@ -6,11 +6,8 @@ load("@io_bazel_rules_scala//scala:scala.bzl", "scala_library") scala_library( name = "app", - # import: ❌ Unit name not found (IMPORT_KIND_UNKNOWN) - # import: ❌ a.b.c name not found (IMPORT_KIND_UNKNOWN) # import: ✅ javax.inject @maven//:javax_inject_javax_inject (IMPLICIT via "javax.xml._") # import: ✅ javax.xml._ @maven//:xml_apis_xml_apis (DIRECT of Main.scala) - # import: ❌ main name not found (IMPORT_KIND_UNKNOWN) # import: ❌ org.junit.rules.TemporaryFolder symbol not found (DIRECT of Main.scala) srcs = ["Main.scala"], deps = [ diff --git a/language/scala/testdata/override_provider/BUILD.out b/language/scala/testdata/override_provider/BUILD.out index a9cf2803..366197d4 100644 --- a/language/scala/testdata/override_provider/BUILD.out +++ b/language/scala/testdata/override_provider/BUILD.out @@ -8,7 +8,6 @@ load("@io_bazel_rules_scala//scala:scala.bzl", "scala_library") scala_library( name = "app", - # import: ❌ a.b.c name not found (IMPORT_KIND_UNKNOWN) # import: ✅ com.typesafe.scalalogging.LazyLogging @maven//:com_typesafe_scala_logging_scala_logging_2_12 (DIRECT of Main.scala) # import: ✅ org.slf4j.Logger @maven//:org_slf4j_slf4j_api (IMPLICIT via "com.typesafe.scalalogging.LazyLogging") srcs = ["Main.scala"], diff --git a/language/scala/testdata/protobuf_provider/BUILD.out b/language/scala/testdata/protobuf_provider/BUILD.out index 12f9003f..a02d217d 100644 --- a/language/scala/testdata/protobuf_provider/BUILD.out +++ b/language/scala/testdata/protobuf_provider/BUILD.out @@ -14,9 +14,6 @@ load("@io_bazel_rules_scala//scala:scala.bzl", "scala_library") scala_library( name = "app", - # import: ❌ Unit name not found (IMPORT_KIND_UNKNOWN) - # import: ❌ a.b.c name not found (IMPORT_KIND_UNKNOWN) - # import: ❌ main name not found (IMPORT_KIND_UNKNOWN) # import: ❌ not.Exists symbol not found (DIRECT of Main.scala) # import: ✅ proto.Customer //proto:proto_proto_scala_library (DIRECT of Main.scala) srcs = ["Main.scala"],