From ce6908d8bb7d21607a39075eeab7a8086ab0d0e2 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:26:39 +1000 Subject: [PATCH 1/3] fix(audience): init does not mutate caller's AudienceConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Init auto-fills PersistentDataPath from DefaultPersistentDataPathProvider. The old code wrote that back onto the caller's config object — a surprising side-effect for callers who reuse the config. Clone via MemberwiseClone before the fill-in. Reference-typed properties (OnError, HttpHandler) are intentionally shared — cloning delegates and handlers would break them. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/AudienceConfig.cs | 5 +++++ src/Packages/Audience/Runtime/ImmutableAudience.cs | 2 ++ 2 files changed, 7 insertions(+) diff --git a/src/Packages/Audience/Runtime/AudienceConfig.cs b/src/Packages/Audience/Runtime/AudienceConfig.cs index c4b2ae9af..023e76216 100644 --- a/src/Packages/Audience/Runtime/AudienceConfig.cs +++ b/src/Packages/Audience/Runtime/AudienceConfig.cs @@ -40,5 +40,10 @@ public class AudienceConfig // Test seam for HttpTransport; not part of the public API. internal System.Net.Http.HttpMessageHandler? HttpHandler { get; set; } + + // Shallow copy so Init can populate PersistentDataPath without mutating + // the caller's object. Reference-typed properties (OnError, HttpHandler) + // are intentionally shared — cloning delegates/handlers is wrong. + internal AudienceConfig Clone() => (AudienceConfig)MemberwiseClone(); } } diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index e90ef7b46..18c6439bb 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -50,6 +50,8 @@ public static void Init(AudienceConfig config) if (string.IsNullOrEmpty(config.PublishableKey)) throw new ArgumentException("PublishableKey is required", nameof(config)); + // Clone so Init's PersistentDataPath fill-in does not mutate the caller's object. + config = config.Clone(); if (string.IsNullOrEmpty(config.PersistentDataPath)) config.PersistentDataPath = DefaultPersistentDataPathProvider?.Invoke(); if (string.IsNullOrEmpty(config.PersistentDataPath)) From ba7b793ff3da74419c18fa91fb82542059182036 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 15:20:06 +1000 Subject: [PATCH 2/3] docs(audience): plain-language comment on Init config clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrites the Clone() rationale and the Init call-site comment in plain language — no jargon about "shallow copy" or "caller's object". No code changes. All 180 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/AudienceConfig.cs | 7 ++++--- src/Packages/Audience/Runtime/ImmutableAudience.cs | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Packages/Audience/Runtime/AudienceConfig.cs b/src/Packages/Audience/Runtime/AudienceConfig.cs index 023e76216..deda29847 100644 --- a/src/Packages/Audience/Runtime/AudienceConfig.cs +++ b/src/Packages/Audience/Runtime/AudienceConfig.cs @@ -41,9 +41,10 @@ public class AudienceConfig // Test seam for HttpTransport; not part of the public API. internal System.Net.Http.HttpMessageHandler? HttpHandler { get; set; } - // Shallow copy so Init can populate PersistentDataPath without mutating - // the caller's object. Reference-typed properties (OnError, HttpHandler) - // are intentionally shared — cloning delegates/handlers is wrong. + // Makes a shallow copy so Init can fill in PersistentDataPath without + // modifying the caller's object. Delegates and handlers (OnError, + // HttpHandler) are deliberately kept as the same instance — cloning + // them would break their behaviour. internal AudienceConfig Clone() => (AudienceConfig)MemberwiseClone(); } } diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index 18c6439bb..0fca64deb 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -50,7 +50,8 @@ public static void Init(AudienceConfig config) if (string.IsNullOrEmpty(config.PublishableKey)) throw new ArgumentException("PublishableKey is required", nameof(config)); - // Clone so Init's PersistentDataPath fill-in does not mutate the caller's object. + // Copy the caller's config so filling in PersistentDataPath below + // doesn't change the object they passed in. config = config.Clone(); if (string.IsNullOrEmpty(config.PersistentDataPath)) config.PersistentDataPath = DefaultPersistentDataPathProvider?.Invoke(); From 47a4bfd4097766e7ffc4c44627fbfdc9c4d5b3b6 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 15:28:48 +1000 Subject: [PATCH 3/3] fix(audience): clone config before validating PublishableKey The previous order (validate then Clone) reset the compiler's null-flow analysis when config was reassigned to the clone, so the later `new HttpTransport(..., config.PublishableKey, ...)` call emitted CS8604 "Possible null reference argument for parameter 'publishableKey'". Moving the Clone above the PublishableKey check keeps validation on the same config object used downstream. MemberwiseClone is O(1), so the extra allocation on the error path is trivial. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/ImmutableAudience.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index 0fca64deb..c35c08e5b 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -47,12 +47,14 @@ public static class ImmutableAudience public static void Init(AudienceConfig config) { if (config == null) throw new ArgumentNullException(nameof(config)); - if (string.IsNullOrEmpty(config.PublishableKey)) - throw new ArgumentException("PublishableKey is required", nameof(config)); // Copy the caller's config so filling in PersistentDataPath below - // doesn't change the object they passed in. + // doesn't change the object they passed in. Validating after the + // clone keeps the compiler's null-flow analysis for PublishableKey + // alive through the HttpTransport constructor below. config = config.Clone(); + if (string.IsNullOrEmpty(config.PublishableKey)) + throw new ArgumentException("PublishableKey is required", nameof(config)); if (string.IsNullOrEmpty(config.PersistentDataPath)) config.PersistentDataPath = DefaultPersistentDataPathProvider?.Invoke(); if (string.IsNullOrEmpty(config.PersistentDataPath))