Fix import option emitted as a regular dependency

experimental/ir/ir_imports.go

@@ -126,6 +126,15 @@ type imported struct {
 	// still re-exported.
 	public bool
 
+	// Whether this file's symbols can only be used in options, because it is
+	// not reachable through any import that is not an option import, either
+	// directly or because that import re-exports it.
+	//
+	// This is not the same as option, which records how the import was
+	// declared. An option import is not option-only if a regular import also
+	// re-exports it.
+	optionOnly bool
+
 	visible, used bool
 }
 
@@ -152,16 +161,11 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) {
 	// import cannot be classified by looking only at whichever direct import
 	// happens to pull it in first. Instead, classify every transitive import
 	// up front, quantifying over all direct imports.
-	public, visible := i.classifyTransitive()
+	public, visible, dependencies := i.classifyTransitive()
 
-	for k, file := range seq.All(i.Directs()) {
+	for file := range seq.Values(i.Directs()) {
 		for imp := range seq.Values(file.TransitiveImports()) {
 			if !mapsx.AddZero(dedup, imp.InternedPath()) {
-				// If imp is public, but file is already present, we need to
-				// treat this import as non-option, because this overrides it.
-				if imp.Public {
-					i.files[k].option = false
-				}
 				continue
 			}
 
@@ -186,6 +190,10 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) {
 		}
 	}
 
+	for n := range i.files {
+		i.files[n].optionOnly = !dependencies.ContainsID(i.files[n].file.InternedPath())
+	}
+
 	// Now, build the path and causes maps.
 	i.byPath = make(intern.Map[uint32])
 	i.causes = make(intern.Map[uint32])
@@ -194,11 +202,18 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) {
 		i.byPath[imp.file.InternedPath()] = uint32(n)
 	}
 
+	// Direct imports take precedence over transitive imports, and a direct
+	// import that re-exports a file takes precedence over one that does not.
 	for k, file := range seq.All(i.Directs()) {
-		// Direct imports take precedence over transitive imports.
 		i.causes[file.InternedPath()] = uint32(k)
-		for imp := range seq.Values(file.TransitiveImports()) {
-			mapsx.Add(i.causes, imp.InternedPath(), uint32(k))
+	}
+	for _, public := range []bool{true, false} {
+		for k, file := range seq.All(i.Directs()) {
+			for imp := range seq.Values(file.TransitiveImports()) {
+				if imp.Public == public {
+					mapsx.Add(i.causes, imp.InternedPath(), uint32(k))
+				}
+			}
 		}
 	}
 }
@@ -216,10 +231,18 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) {
 // Neither set includes the direct imports themselves, which are always
 // visible, nor files reached only through non-public imports, which are
 // neither.
-func (i *imports) classifyTransitive() (public, visible intern.Set) {
+//
+// dependencies is the set of direct imports that are not option imports,
+// together with the files they re-export. Unlike public and visible, it
+// includes direct imports.
+func (i *imports) classifyTransitive() (public, visible, dependencies intern.Set) {
 	public = make(intern.Set)
 	visible = make(intern.Set)
+	dependencies = make(intern.Set)
 	for file := range seq.Values(i.Directs()) {
+		if !file.Option {
+			dependencies.AddID(file.InternedPath())
+		}
 		for imp := range seq.Values(file.TransitiveImports()) {
 			if !imp.Public {
 				continue
@@ -229,9 +252,12 @@ func (i *imports) classifyTransitive() (public, visible intern.Set) {
 			if file.Public {
 				public.AddID(imp.InternedPath())
 			}
+			if !file.Option {
+				dependencies.AddID(imp.InternedPath())
+			}
 		}
 	}
-	return public, visible
+	return public, visible, dependencies
 }
 
 // Insert inserts a new import at the given position. It also builds up the path map for lookups.

experimental/ir/ir_symbol.go

@@ -166,7 +166,7 @@ func (s Symbol) Visible(in *File, allowOptions bool) bool {
 	}
 
 	imp := in.imports.files[idx]
-	if !imp.visible || !(allowOptions || !imp.option) {
+	if !imp.visible || (!allowOptions && imp.optionOnly) {
 		return false
 	}
 

experimental/ir/ir_test.go

@@ -555,13 +555,15 @@ func symtabProto(files []*ir.File) *compilerpb.SymbolSet {
 				options = sym.AsOneof().Options()
 			}
 
+			// OptionOnly is set for symbols that are visible in options, but not
+			// elsewhere.
 			symtab.Symbols = append(symtab.Symbols, &compilerpb.Symbol{
 				Fqn:        string(sym.FullName()),
 				Kind:       compilerpb.Symbol_Kind(sym.Kind()),
 				File:       sym.Context().Path(),
 				Index:      uint32(sym.RawData()),
 				Visible:    sym.Kind() != ir.SymbolKindPackage && sym.Visible(file, false),
-				OptionOnly: sym.Kind() != ir.SymbolKindPackage && !sym.Visible(file, true) && sym.Visible(file, true),
+				OptionOnly: sym.Kind() != ir.SymbolKindPackage && !sym.Visible(file, false) && sym.Visible(file, true),
 				Options:    new(optionWalker).message(options),
 				Features:   dumpFeatures(sym.FeatureSet(), sym.Kind().OptionTarget()),
 			})

experimental/ir/lower_resolve.go

@@ -372,7 +372,10 @@ func (r symbolRef) diagnoseLookup(sym Symbol, expectedName FullName) *report.Dia
 		)
 	case !sym.Visible(r.File, r.allowOption):
 		if !r.allowOption && sym.Visible(r.File, true) {
-			decl := sym.Import(r.File).Decl
+			// The symbol may come from a file the option import re-exports, so
+			// point at the direct import instead.
+			imports := &r.File.imports
+			decl := imports.files[imports.causes[sym.Context().InternedPath()]].decl
 			var option token.Token
 			for m := range seq.Values(decl.ModifierTokens()) {
 				if m.Keyword() == keyword.Option {

experimental/ir/lower_validate.go

@@ -57,7 +57,7 @@ func diagnoseUnusedImports(f *File, r *report.Report) {
 		}
 
 		r.Warnf("unused import %s", imp.Decl.ImportPath().AsLiteral().Text()).Apply(
-			report.Snippet(imp.Decl.ImportPath()),
+			report.Snippet(imp.Decl),
 			report.SuggestEdits(imp.Decl, "delete it", report.Edit{
 				Start: 0, End: imp.Decl.Span().Len(),
 			}),

experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml

@@ -23,18 +23,22 @@ tables:
       kind: KIND_MESSAGE
       file: "option.proto"
       index: 1
+      option_only: true
     - fqn: "buf.test.Foo.x"
       kind: KIND_FIELD
       file: "option.proto"
       index: 1
+      option_only: true
     - fqn: "buf.test.e"
       kind: KIND_EXTENSION
       file: "option.proto"
       index: 2
+      option_only: true
     - fqn: "buf.test.any_opt"
       kind: KIND_EXTENSION
       file: "option.proto"
       index: 3
+      option_only: true
   "main2.proto":
     imports:
     - { path: "google/protobuf/any.proto", transitive: true }
@@ -60,18 +64,22 @@ tables:
       kind: KIND_MESSAGE
       file: "option.proto"
       index: 1
+      visible: true
     - fqn: "buf.test.Foo.x"
       kind: KIND_FIELD
       file: "option.proto"
       index: 1
+      visible: true
     - fqn: "buf.test.e"
       kind: KIND_EXTENSION
       file: "option.proto"
       index: 2
+      visible: true
     - fqn: "buf.test.any_opt"
       kind: KIND_EXTENSION
       file: "option.proto"
       index: 3
+      visible: true
   "main3.proto":
     imports:
     - { path: "google/protobuf/any.proto", transitive: true }
@@ -94,15 +102,19 @@ tables:
       kind: KIND_MESSAGE
       file: "option.proto"
       index: 1
+      option_only: true
     - fqn: "buf.test.Foo.x"
       kind: KIND_FIELD
       file: "option.proto"
       index: 1
+      option_only: true
     - fqn: "buf.test.e"
       kind: KIND_EXTENSION
       file: "option.proto"
       index: 2
+      option_only: true
     - fqn: "buf.test.any_opt"
       kind: KIND_EXTENSION
       file: "option.proto"
       index: 3
+      option_only: true

experimental/ir/testdata/imports/option_public.proto.yaml

@@ -0,0 +1,80 @@
+descriptor: true
+symtab: true
+files:
+# An option import must stay an option import, even when its public import is
+# also reached through another import.
+- path: "main.proto"
+  text: |
+    edition = "2024";
+    package buf.test;
+
+    import "other.proto";
+    import option "b.proto";
+
+    option (c.copt) = 1;
+
+# An option import that is also re-exported by a regular import is fully
+# visible.
+- path: "main2.proto"
+  text: |
+    edition = "2024";
+    package buf.test;
+
+    import "x.proto";
+    import option "b.proto";
+
+    message M2 {
+      b.B f = 1;
+      c.C g = 2;
+    }
+
+# A file re-exported only by an option import is only visible in options.
+- path: "main3.proto"
+  text: |
+    edition = "2024";
+    package buf.test;
+
+    import option "b.proto";
+
+    option (c.copt) = 1;
+    message M3 { c.C f = 1; }
+
+- path: "other.proto"
+  import: true
+  text: |
+    edition = "2024";
+    package other;
+
+    import option "b.proto";
+
+- path: "x.proto"
+  import: true
+  text: |
+    edition = "2024";
+    package x;
+
+    import public "b.proto";
+
+- path: "b.proto"
+  import: true
+  text: |
+    edition = "2024";
+    package b;
+
+    import public "c.proto";
+
+    message B {}
+
+- path: "c.proto"
+  import: true
+  text: |
+    edition = "2024";
+    package c;
+
+    import "google/protobuf/descriptor.proto";
+
+    message C {}
+
+    extend google.protobuf.FileOptions {
+      int32 copt = 50000;
+    }

experimental/ir/testdata/imports/option_public.proto.yaml.fds.yaml

@@ -0,0 +1,75 @@
+file:
+- name: "c.proto"
+  package: "c"
+  dependency: ["google/protobuf/descriptor.proto"]
+  message_type: [{ name: "C" }]
+  extension:
+  - name: "copt"
+    number: 50000
+    label: LABEL_OPTIONAL
+    type: TYPE_INT32
+    extendee: ".google.protobuf.FileOptions"
+    json_name: "copt"
+  syntax: "editions"
+  edition: EDITION_2024
+- name: "b.proto"
+  package: "b"
+  dependency: ["c.proto"]
+  public_dependency: [0]
+  message_type: [{ name: "B" }]
+  syntax: "editions"
+  edition: EDITION_2024
+- name: "other.proto"
+  package: "other"
+  option_dependency: ["b.proto"]
+  syntax: "editions"
+  edition: EDITION_2024
+- name: "main.proto"
+  package: "buf.test"
+  dependency: ["other.proto"]
+  option_dependency: ["b.proto"]
+  options.$unknown: "50000: 1"
+  syntax: "editions"
+  edition: EDITION_2024
+- name: "x.proto"
+  package: "x"
+  dependency: ["b.proto"]
+  public_dependency: [0]
+  syntax: "editions"
+  edition: EDITION_2024
+- name: "main2.proto"
+  package: "buf.test"
+  dependency: ["x.proto"]
+  option_dependency: ["b.proto"]
+  message_type:
+  - name: "M2"
+    field:
+    - name: "f"
+      number: 1
+      label: LABEL_OPTIONAL
+      type: TYPE_MESSAGE
+      type_name: ".b.B"
+      json_name: "f"
+    - name: "g"
+      number: 2
+      label: LABEL_OPTIONAL
+      type: TYPE_MESSAGE
+      type_name: ".c.C"
+      json_name: "g"
+  syntax: "editions"
+  edition: EDITION_2024
+- name: "main3.proto"
+  package: "buf.test"
+  option_dependency: ["b.proto"]
+  message_type:
+  - name: "M3"
+    field:
+    - name: "f"
+      number: 1
+      label: LABEL_OPTIONAL
+      type: TYPE_MESSAGE
+      type_name: ".c.C"
+      json_name: "f"
+  options.$unknown: "50000: 1"
+  syntax: "editions"
+  edition: EDITION_2024