[Observation] Exclude generic class types from pre-caching their keypaths (swiftlang#78790)

* [Observation] Exclude generic class types from pre-caching their keypaths

* Account for multiple nesting levels of generics for observation cached keypaths

Author: phausler

lib/Macros/Sources/ObservationMacros/Extensions.swift

@@ -14,6 +14,21 @@ import SwiftSyntaxMacros
 import SwiftDiagnostics
 import SwiftSyntaxBuilder
 
+extension Syntax {
+  var isNonGeneric: Bool {
+    if let classDecl = self.as(ClassDeclSyntax.self) {
+      if classDecl.genericParameterClause == nil { return true }
+    } else if let structDecl = self.as(StructDeclSyntax.self) {
+      if structDecl.genericParameterClause == nil { return true }
+    } else if let enumDecl = self.as(EnumDeclSyntax.self) {
+      if enumDecl.genericParameterClause == nil { return true }
+    } else if let actorDecl = self.as(ActorDeclSyntax.self) {
+      if actorDecl.genericParameterClause == nil { return true }
+    }
+    return false
+  }
+}
+
 extension VariableDeclSyntax {
   var identifierPattern: IdentifierPatternSyntax? {
     bindings.first?.pattern.as(IdentifierPatternSyntax.self)

lib/Macros/Sources/ObservationMacros/ObservableMacro.swift

@@ -101,6 +101,10 @@ public struct ObservableMacro {
       """
   }
 
+  static func canCacheKeyPaths(_ lexicalContext: [Syntax]) -> Bool {
+    lexicalContext.allSatisfy { $0.isNonGeneric }
+  }
+
   static var ignoredAttribute: AttributeSyntax {
     AttributeSyntax(
       leadingTrivia: .space,
@@ -353,47 +357,88 @@ public struct ObservationTrackedMacro: AccessorMacro {
         _\(identifier) = initialValue
       }
       """
+    if ObservableMacro.canCacheKeyPaths(context.lexicalContext) {
+      let getAccessor: AccessorDeclSyntax =
+        """
+        get {
+          access(keyPath: \(container.trimmed.name)._cachedKeypath_\(identifier))
+          return _\(identifier)
+        }
+        """
 
-    let getAccessor: AccessorDeclSyntax =
-      """
-      get {
-        access(keyPath: \(container.trimmed.name)._cachedKeypath_\(identifier))
-        return _\(identifier)
-      }
-      """
+      let setAccessor: AccessorDeclSyntax =
+        """
+        set {
+          guard shouldNotifyObservers(_\(identifier), newValue) else {
+            return
+          }
+          withMutation(keyPath: \(container.trimmed.name)._cachedKeypath_\(identifier)) {
+            _\(identifier) = newValue
+          }
+        }
+        """
+        
+      // Note: this accessor cannot test the equality since it would incur
+      // additional CoW's on structural types. Most mutations in-place do
+      // not leave the value equal so this is "fine"-ish.
+      // Warning to future maintence: adding equality checks here can make
+      // container mutation O(N) instead of O(1).
+      // e.g. observable.array.append(element) should just emit a change
+      // to the new array, and NOT cause a copy of each element of the
+      // array to an entirely new array.
+      let modifyAccessor: AccessorDeclSyntax =
+        """
+        _modify {
+          let keyPath = \(container.trimmed.name)._cachedKeypath_\(identifier)
+          access(keyPath: keyPath)
+          \(raw: ObservableMacro.registrarVariableName).willSet(self, keyPath: keyPath)
+          defer { \(raw: ObservableMacro.registrarVariableName).didSet(self, keyPath: keyPath) }
+          yield &_\(identifier)
+        }
+        """
 
-    let setAccessor: AccessorDeclSyntax =
-      """
-      set {
-        guard shouldNotifyObservers(_\(identifier), newValue) else {
-          return
+      return [initAccessor, getAccessor, setAccessor, modifyAccessor]
+    } else {
+      let getAccessor: AccessorDeclSyntax =
+        """
+        get {
+          access(keyPath: \\.\(identifier))
+          return _\(identifier)
+        }
+        """
+
+      let setAccessor: AccessorDeclSyntax =
+        """
+        set {
+          guard shouldNotifyObservers(_\(identifier), newValue) else {
+            return
+          }
+          withMutation(keyPath: \\.\(identifier)) {
+            _\(identifier) = newValue
+          }
         }
-        withMutation(keyPath: \(container.trimmed.name)._cachedKeypath_\(identifier)) {
-          _\(identifier) = newValue
+        """
+        
+      // Note: this accessor cannot test the equality since it would incur
+      // additional CoW's on structural types. Most mutations in-place do
+      // not leave the value equal so this is "fine"-ish.
+      // Warning to future maintence: adding equality checks here can make
+      // container mutation O(N) instead of O(1).
+      // e.g. observable.array.append(element) should just emit a change
+      // to the new array, and NOT cause a copy of each element of the
+      // array to an entirely new array.
+      let modifyAccessor: AccessorDeclSyntax =
+        """
+        _modify {
+          access(keyPath: \\.\(identifier))
+          \(raw: ObservableMacro.registrarVariableName).willSet(self, keyPath: \\.\(identifier))
+          defer { \(raw: ObservableMacro.registrarVariableName).didSet(self, keyPath: \\.\(identifier)) }
+          yield &_\(identifier)
         }
-      }
-      """
-      
-    // Note: this accessor cannot test the equality since it would incur
-    // additional CoW's on structural types. Most mutations in-place do
-    // not leave the value equal so this is "fine"-ish.
-    // Warning to future maintence: adding equality checks here can make
-    // container mutation O(N) instead of O(1).
-    // e.g. observable.array.append(element) should just emit a change
-    // to the new array, and NOT cause a copy of each element of the
-    // array to an entirely new array.
-    let modifyAccessor: AccessorDeclSyntax =
-      """
-      _modify {
-        let keyPath = \(container.trimmed.name)._cachedKeypath_\(identifier)
-        access(keyPath: keyPath)
-        \(raw: ObservableMacro.registrarVariableName).willSet(self, keyPath: keyPath)
-        defer { \(raw: ObservableMacro.registrarVariableName).didSet(self, keyPath: keyPath) }
-        yield &_\(identifier)
-      }
-      """
+        """
 
-    return [initAccessor, getAccessor, setAccessor, modifyAccessor]
+      return [initAccessor, getAccessor, setAccessor, modifyAccessor]
+    }
   }
 }
 
@@ -422,11 +467,15 @@ extension ObservationTrackedMacro: PeerMacro {
     }
 
     let storage = DeclSyntax(property.privatePrefixed("_", addingAttribute: ObservableMacro.ignoredAttribute))
-    let cachedKeypath: DeclSyntax =
-      """
-      private static let _cachedKeypath_\(identifier) = \\\(container.name).\(identifier)
-      """
-    return [storage, cachedKeypath]
+    if ObservableMacro.canCacheKeyPaths(context.lexicalContext) {
+      let cachedKeypath: DeclSyntax =
+        """
+        private static let _cachedKeypath_\(identifier) = \\\(container.name).\(identifier)
+        """
+      return [storage, cachedKeypath]
+    } else {
+      return [storage]
+    }
   }
 }
 

test/stdlib/Observation/Observable.swift

@@ -185,6 +185,39 @@ class RecursiveInner {
   var value = "prefix"
 }
 
+@Observable
+class GenericClass<T> {
+  var value = 3
+}
+
+struct StructParent {
+  @Observable
+  class NestedGenericClass<T> {
+    var value = 3
+  }
+}
+
+struct GenericStructParent<T> {
+  @Observable
+  class NestedClass {
+    var value = 3
+  }
+}
+
+class ClassParent {
+  @Observable
+  class NestedGenericClass<T> {
+    var value = 3
+  }
+}
+
+class GenericClassParent<T> {
+  @Observable
+  class NestedClass {
+    var value = 3
+  }
+}
+
 @Observable
 class RecursiveOuter {
   var inner = RecursiveInner()