clarify how each "cannot obfuscate" map works

We used to record all objects in cannotObfuscateNames,
and then we'd add the exported ones to KnownCannotObfuscate.

Instead, teach recordAsNotObfuscated to store each object in either
knownCannotObfuscateUnexported or KnownCannotObfuscate, but not both.
The former isn't cached so it uses in-memory pointers as keys,
and the latter uses the cross-process objectStrings like before.

Functionally, this is all the same, but with the difference that the map
indexed by types.Object will not contain objects already recorded in
KnownCannotObfuscate, reducing the amount of duplicate memory use.

While here, give recordIgnore a less ambiguous name,
and remove the second parameter as it was always tf.pkg.Path().
This also means we can compare *types.Package pointers directly.

Finally, add more TODOs for further improvement ideas.
It does mean that we end up with more TODOs than before,
even though I'm fixing one, but I reckon that's a good thing.
Recording these ideas can give first-time contributors ways to help,
and it ensures I don't forget about ideas just in my head.

main.go

@@ -939,6 +939,13 @@ type (
 	}
 )
 
+// TODO: read-write globals like these should probably be inside transformer
+
+// knownCannotObfuscateUnexported is like KnownCannotObfuscate but for
+// unexported names. We don't need to store this in the build cache,
+// because these names cannot be referenced by downstream packages.
+var knownCannotObfuscateUnexported = map[types.Object]bool{}
+
 // cachedOutput contains information that will be stored as per garbleExportFile.
 var cachedOutput = struct {
 	// KnownReflectAPIs is a static record of what std APIs use reflection on their
@@ -952,8 +959,6 @@ var cachedOutput = struct {
 	// package that we could not obfuscate as per cannotObfuscateNames.
 	// This record is necessary for knowing what names from imported packages
 	// weren't obfuscated, so we can obfuscate their local uses accordingly.
-	//
-	// TODO: merge cannotObfuscateNames into this directly
 	KnownCannotObfuscate map[objectString]struct{}
 
 	// KnownEmbeddedAliasFields records which embedded fields use a type alias.
@@ -1108,7 +1113,6 @@ func (tf *transformer) findReflectFunctions(files []*ast.File) {
 // Since we obfuscate one package at a time, we only detect those if the type
 // definition and the reflect usage are both in the same package.
 func (tf *transformer) prefillObjectMaps(files []*ast.File) {
-	tf.cannotObfuscateNames = make(map[types.Object]bool)
 	tf.linkerVariableStrings = make(map[types.Object]string)
 
 	ldflags := flagValue(cache.ForwardBuildFlags, "-ldflags")
@@ -1161,7 +1165,7 @@ func (tf *transformer) prefillObjectMaps(files []*ast.File) {
 		for _, argPos := range cachedOutput.KnownReflectAPIs[fullName] {
 			arg := call.Args[argPos]
 			argType := tf.info.TypeOf(arg)
-			tf.recordIgnore(argType, tf.pkg.Path())
+			tf.recursivelyRecordAsNotObfuscated(argType)
 		}
 
 		return true
@@ -1178,7 +1182,8 @@ func (tf *transformer) prefillObjectMaps(files []*ast.File) {
 				if obj == nil {
 					continue // not found; skip
 				}
-				tf.cannotObfuscateNames[obj] = true
+				// TODO(mvdan): it seems like removing this doesn't break any tests.
+				recordAsNotObfuscated(obj)
 			}
 		}
 		ast.Inspect(file, visit)
@@ -1192,19 +1197,6 @@ type transformer struct {
 	pkg  *types.Package
 	info *types.Info
 
-	// cannotObfuscateNames records all the objects whose names we cannot obfuscate.
-	// An object is any named entity, such as a declared variable or type.
-	//
-	// This map is initialized by prefillObjectMaps at the start,
-	// and extra entries from dependencies are added by transformGo,
-	// for the sake of caching type lookups.
-	// So far, it records:
-	//
-	//  * Types which are used for reflection.
-	//  * Declarations exported via "//export".
-	//  * Types or variables from external packages which were not obfuscated.
-	cannotObfuscateNames map[types.Object]bool
-
 	// linkerVariableStrings is also initialized by prefillObjectMaps.
 	// It records objects for variables used in -ldflags=-X flags,
 	// as well as the strings the user wants to inject them with.
@@ -1305,6 +1297,9 @@ func (tf *transformer) recordType(t types.Type) {
 	}
 }
 
+// TODO: consider caching recordedObjectString via a map,
+// if that shows an improvement in our benchmark
+
 func recordedObjectString(obj types.Object) objectString {
 	if obj, ok := obj.(*types.Var); ok && obj.IsField() {
 		// For exported fields, "pkgpath.Field" is not unique,
@@ -1335,30 +1330,38 @@ func recordedObjectString(obj types.Object) objectString {
 	return fmt.Sprintf("%s.%s", obj.Pkg().Path(), obj.Name())
 }
 
-func recordAsNotObfuscated(obj types.Object) bool {
+// recordAsNotObfuscated records all the objects whose names we cannot obfuscate.
+// An object is any named entity, such as a declared variable or type.
+//
+// So far, it records:
+//
+//  * Types which are used for reflection.
+//  * Declarations exported via "//export".
+//  * Types or variables from external packages which were not obfuscated.
+func recordAsNotObfuscated(obj types.Object) {
 	if obj.Pkg().Path() != curPkg.ImportPath {
 		panic("called recordedAsNotObfuscated with a foreign object")
 	}
 	if !obj.Exported() {
 		// Unexported names will never be used by other packages,
-		// so we don't need to bother recording them.
+		// so we don't need to bother recording them in cachedOutput.
-		return true
+		knownCannotObfuscateUnexported[obj] = true
+		return
 	}
 
-	if objStr := recordedObjectString(obj); objStr != "" {
+	objStr := recordedObjectString(obj)
-		cachedOutput.KnownCannotObfuscate[objStr] = struct{}{}
+	if objStr == "" {
+		// If the object can't be described via a qualified string,
+		// then other packages can't use it.
+		// TODO: should we still record it in knownCannotObfuscateUnexported?
+		return
 	}
-	return true // to simplify early returns in astutil.ApplyFunc
+	cachedOutput.KnownCannotObfuscate[objStr] = struct{}{}
 }
 
 func recordedAsNotObfuscated(obj types.Object) bool {
-	if obj.Pkg().Path() == curPkg.ImportPath {
+	if knownCannotObfuscateUnexported[obj] {
-		// The current package knows what names it's not obfuscating.
+		return true
-		return false
-	}
-	if !obj.Exported() {
-		// Not recorded, as per recordAsNotObfuscated.
-		return false
 	}
 	objStr := recordedObjectString(obj)
 	if objStr == "" {
@@ -1468,11 +1471,7 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File {
 			return true
 		}
 
-		// We don't want to obfuscate this object name.
+		// The package that declared this object did not obfuscate it.
-		if tf.cannotObfuscateNames[obj] {
-			return recordAsNotObfuscated(obj)
-		}
-		// The imported package that declared this object did not obfuscate it.
 		if recordedAsNotObfuscated(obj) {
 			return true
 		}
@@ -1584,26 +1583,26 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File {
 	return astutil.Apply(file, pre, post).(*ast.File)
 }
 
-// recordIgnore adds any named types (including fields) under typ to
+// recursivelyRecordAsNotObfuscated calls recordAsNotObfuscated on any named
-// cannotObfuscateNames.
+// types and fields under typ.
 //
-// Only the names declared in package pkgPath are recorded. This is to ensure
+// Only the names declared in the current package are recorded. This is to ensure
 // that reflection detection only happens within the package declaring a type.
 // Detecting it in downstream packages could result in inconsistencies.
-func (tf *transformer) recordIgnore(t types.Type, pkgPath string) {
+func (tf *transformer) recursivelyRecordAsNotObfuscated(t types.Type) {
 	switch t := t.(type) {
 	case *types.Named:
 		obj := t.Obj()
-		if obj.Pkg() == nil || obj.Pkg().Path() != pkgPath {
+		if obj.Pkg() == nil || obj.Pkg() != tf.pkg {
 			return // not from the specified package
 		}
-		if tf.cannotObfuscateNames[obj] {
+		if recordedAsNotObfuscated(obj) {
 			return // prevent endless recursion
 		}
-		tf.cannotObfuscateNames[obj] = true
+		recordAsNotObfuscated(obj)
 
 		// Record the underlying type, too.
-		tf.recordIgnore(t.Underlying(), pkgPath)
+		tf.recursivelyRecordAsNotObfuscated(t.Underlying())
 
 	case *types.Struct:
 		for i := 0; i < t.NumFields(); i++ {
@@ -1612,19 +1611,19 @@ func (tf *transformer) recordIgnore(t types.Type, pkgPath string) {
 			// This check is similar to the one in *types.Named.
 			// It's necessary for unnamed struct types,
 			// as they aren't named but still have named fields.
-			if field.Pkg() == nil || field.Pkg().Path() != pkgPath {
+			if field.Pkg() == nil || field.Pkg() != tf.pkg {
 				return // not from the specified package
 			}
 
 			// Record the field itself, too.
-			tf.cannotObfuscateNames[field] = true
+			recordAsNotObfuscated(field)
 
-			tf.recordIgnore(field.Type(), pkgPath)
+			tf.recursivelyRecordAsNotObfuscated(field.Type())
 		}
 
 	case interface{ Elem() types.Type }:
 		// Get past pointers, slices, etc.
-		tf.recordIgnore(t.Elem(), pkgPath)
+		tf.recursivelyRecordAsNotObfuscated(t.Elem())
 	}
 }