From e054e621f301cb3384a695aeab5b1bea9b374ab3 Mon Sep 17 00:00:00 2001 From: Manuel de la Pena Saenz Date: Wed, 20 Aug 2025 12:14:46 -0400 Subject: [PATCH 1/2] [RGen] Add the validation for weak delegates. Weak delegate properties have to start with 'Weak' or provide a strong delegate name for the rgen to generate a unique valid name. Added tests to ensure that both the PropertyValidator and PropertyOrFieldValidator return the correct error. --- .../Resources.Designer.cs | 27 ++++++++++++++++ .../Resources.resx | 13 ++++++++ .../RgenDiagnostics.cs | 15 +++++++++ .../Validators/PropertyValidator.cs | 32 +++++++++++++++++++ .../PropertyOrFieldValidatorTests.cs | 8 +++++ .../Validators/PropertyValidatorTestLogic.cs | 21 ++++++++++-- .../Validators/PropertyValidatorTests.cs | 8 +++++ 7 files changed, 121 insertions(+), 3 deletions(-) diff --git a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs index e3b142d84aa6..0e7971de7701 100644 --- a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs +++ b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs @@ -920,5 +920,32 @@ internal static string RBI0031Title { return ResourceManager.GetString("RBI0031Title", resourceCulture); } } + + /// + /// Looks up a localized string similar to A weak delegate has to have a strong delegate name if it does not start with 'Weak'.. + /// + internal static string RBI0032Description { + get { + return ResourceManager.GetString("RBI0032Description", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The weak delegate '{0}' is missing a strong delegate name, provide one or use the 'Weak' prefix. + /// + internal static string RBI0032MessageFormat { + get { + return ResourceManager.GetString("RBI0032MessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Wrong weak delegate. + /// + internal static string RBI0032Title { + get { + return ResourceManager.GetString("RBI0032Title", resourceCulture); + } + } } } diff --git a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.resx b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.resx index c1bec498c5c5..0ffcda715c7e 100644 --- a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.resx +++ b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.resx @@ -430,4 +430,17 @@ Wrong property declaration + + + + A weak delegate has to have a strong delegate name if it does not start with 'Weak'. + + + The weak delegate '{0}' is missing a strong delegate name, provide one or use the 'Weak' prefix + {0} is the name of the property that does not start with 'Weak'. + + + Wrong weak delegate + + diff --git a/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs b/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs index 5c48bd735028..ebc3e07a2b4a 100644 --- a/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs +++ b/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs @@ -481,4 +481,19 @@ public static class RgenDiagnostics { description: new LocalizableResourceString (nameof (Resources.RBI0031Description), Resources.ResourceManager, typeof (Resources)) ); + + /// + /// Diagnostic descriptor for when a weak delegate does not start with "Weak". + /// + internal static readonly DiagnosticDescriptor RBI0032 = new ( + "RBI0032", + new LocalizableResourceString (nameof (Resources.RBI0032Title), Resources.ResourceManager, typeof (Resources)), + new LocalizableResourceString (nameof (Resources.RBI0032MessageFormat), Resources.ResourceManager, + typeof (Resources)), + "Usage", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: new LocalizableResourceString (nameof (Resources.RBI0032Description), Resources.ResourceManager, + typeof (Resources)) + ); } diff --git a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs index 9e1d1256205f..5c723edf07df 100644 --- a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs +++ b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using System; using System.Collections.Immutable; using Microsoft.CodeAnalysis; using Microsoft.Macios.Generator.Context; @@ -70,6 +71,32 @@ internal static bool AccessorIsValid ((string PropertyName, Accessor Accessor) d out diagnostics); } + /// + /// Validates that a weak delegate property's name starts with "Weak" or that it has a corresponding strong delegate name defined. + /// + /// The property to validate. + /// The root context for validation. + /// When this method returns, contains a diagnostic if the property name is invalid; otherwise, an empty array. + /// The code location to be used for the diagnostics. + /// true if the property is not a weak delegate or if its name is valid; otherwise, false. + internal static bool WeakPropertyNameStartsWithWeak (Property data, RootContext context, + out ImmutableArray diagnostic, Location? location) + { + diagnostic = ImmutableArray.Empty; + if (data.IsWeakDelegate + && !data.Name.StartsWith ("Weak", StringComparison.Ordinal) + && string.IsNullOrEmpty (data.ExportPropertyData.StrongDelegateName)) { + // if the property is weak, it must start with Weak + diagnostic = [Diagnostic.Create ( + descriptor: RBI0032, + location: data.Location, + messageArgs: data.Name)]; + return false; + } + // we do not care about non-weak properties + return true; + } + /// /// Initializes a new instance of the class. /// @@ -96,5 +123,10 @@ public PropertyValidator () : base (p => p.Location) descriptor: [RBI0018, RBI0019, RBI0029], validation: AccessorIsValid, propertyName: "setter"); + + // if a property is weak ensure that it starts the name with Weak or set the strondelegatename in the + // export property data + AddGlobalStrategy (RBI0032, WeakPropertyNameStartsWithWeak); } + } diff --git a/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyOrFieldValidatorTests.cs b/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyOrFieldValidatorTests.cs index d3329d375dcb..738eded7d1a6 100644 --- a/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyOrFieldValidatorTests.cs +++ b/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyOrFieldValidatorTests.cs @@ -152,4 +152,12 @@ public void ValidateValidPropertyTests () [Fact] public void ValidateValidFieldPropertyTests () => testLogic.ValidateValidFieldPropertyTestsImpl (); + + [Theory] + [InlineData ("MyProperty", false, null, 0)] // Not weak, should pass + [InlineData ("WeakMyProperty", true, null, 0)] // Weak, starts with "Weak", should pass + [InlineData ("MyProperty", true, "StrongDelegateName", 0)] // Weak, doesn't start with "Weak", but has StrongDelegateName, should pass + [InlineData ("MyProperty", true, null, 1)] // Weak, doesn't start with "Weak", no StrongDelegateName, should fail + public void WeakPropertyNameStartsWithWeakTests (string propertyName, bool isWeak, string? strongDelegateName, int expectedDiagnosticsCount) + => testLogic.WeakPropertyNameStartsWithWeakTestsImpl (propertyName, isWeak, strongDelegateName, expectedDiagnosticsCount); } diff --git a/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTestLogic.cs b/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTestLogic.cs index 0807e89fc39f..9fd88c4d2d02 100644 --- a/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTestLogic.cs +++ b/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTestLogic.cs @@ -51,7 +51,8 @@ Property CreateProperty (string name = "TestProperty", string? getterSelector = null, string? setterSelector = null, bool hasGetter = true, - bool hasSetter = true) + bool hasSetter = true, + string? strongDelegateName = null) { var modifiers = ImmutableArray.CreateBuilder (); modifiers.Add (SyntaxFactory.Token (SyntaxKind.PublicKeyword)); @@ -103,7 +104,9 @@ Property CreateProperty (string name = "TestProperty", accessors: accessors.ToImmutable () ) { ExportPropertyData = new ExportData (propertySelector, - argumentSemantic, propertyFlags) + argumentSemantic, propertyFlags) { + StrongDelegateName = strongDelegateName + } }; } @@ -318,5 +321,17 @@ public void ValidateValidFieldPropertyTestsImpl () var result = validator.ValidateAll (property, context); Assert.Empty (result); } -} + public void WeakPropertyNameStartsWithWeakTestsImpl (string propertyName, bool isWeak, string? strongDelegateName, int expectedDiagnosticsCount) + { + var flags = isWeak ? ObjCBindings.Property.WeakDelegate : ObjCBindings.Property.Default; + var property = CreateProperty (name: propertyName, propertyFlags: flags, strongDelegateName: strongDelegateName); + var result = validator.ValidateAll (property, context); + var totalDiagnostics = result.Values.Sum (x => x.Count); + Assert.Equal (expectedDiagnosticsCount, totalDiagnostics); + if (expectedDiagnosticsCount > 0) { + var diagnostic = result.Values.SelectMany (x => x).First (); + Assert.Equal ("RBI0032", diagnostic.Id); + } + } +} diff --git a/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTests.cs b/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTests.cs index 51d83a918ffe..748eb36998f5 100644 --- a/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTests.cs +++ b/tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyValidatorTests.cs @@ -88,4 +88,12 @@ public void PropertyAccessorPresenceTests (bool hasGetter, bool hasSetter, int e [InlineData (false, "invalid getter", "invalid setter:", 3)] // Not partial and multiple issues public void CombinedValidationTests (bool isPartial, string? getterSelector, string? setterSelector, int expectedDiagnosticsCount) => testLogic.CombinedPropertyValidationTestsImpl (isPartial, getterSelector, setterSelector, expectedDiagnosticsCount); + + [Theory] + [InlineData ("MyProperty", false, null, 0)] // Not weak, should pass + [InlineData ("WeakMyProperty", true, null, 0)] // Weak, starts with "Weak", should pass + [InlineData ("MyProperty", true, "StrongDelegateName", 0)] // Weak, doesn't start with "Weak", but has StrongDelegateName, should pass + [InlineData ("MyProperty", true, null, 1)] // Weak, doesn't start with "Weak", no StrongDelegateName, should fail + public void WeakPropertyNameStartsWithWeakTests (string propertyName, bool isWeak, string? strongDelegateName, int expectedDiagnosticsCount) + => testLogic.WeakPropertyNameStartsWithWeakTestsImpl (propertyName, isWeak, strongDelegateName, expectedDiagnosticsCount); } From b80b8c864ee521a0b1aa9f152c0f63ce651aa57b Mon Sep 17 00:00:00 2001 From: GitHub Actions Autoformatter Date: Wed, 20 Aug 2025 16:20:47 +0000 Subject: [PATCH 2/2] Auto-format source code --- .../RgenDiagnostics.cs | 2 +- .../Validators/PropertyValidator.cs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs b/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs index ebc3e07a2b4a..ded22afc4295 100644 --- a/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs +++ b/src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs @@ -481,7 +481,7 @@ public static class RgenDiagnostics { description: new LocalizableResourceString (nameof (Resources.RBI0031Description), Resources.ResourceManager, typeof (Resources)) ); - + /// /// Diagnostic descriptor for when a weak delegate does not start with "Weak". /// diff --git a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs index 5c723edf07df..7a6fef34cc5a 100644 --- a/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs +++ b/src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/PropertyValidator.cs @@ -79,13 +79,13 @@ internal static bool AccessorIsValid ((string PropertyName, Accessor Accessor) d /// When this method returns, contains a diagnostic if the property name is invalid; otherwise, an empty array. /// The code location to be used for the diagnostics. /// true if the property is not a weak delegate or if its name is valid; otherwise, false. - internal static bool WeakPropertyNameStartsWithWeak (Property data, RootContext context, + internal static bool WeakPropertyNameStartsWithWeak (Property data, RootContext context, out ImmutableArray diagnostic, Location? location) { diagnostic = ImmutableArray.Empty; - if (data.IsWeakDelegate - && !data.Name.StartsWith ("Weak", StringComparison.Ordinal) - && string.IsNullOrEmpty (data.ExportPropertyData.StrongDelegateName)) { + if (data.IsWeakDelegate + && !data.Name.StartsWith ("Weak", StringComparison.Ordinal) + && string.IsNullOrEmpty (data.ExportPropertyData.StrongDelegateName)) { // if the property is weak, it must start with Weak diagnostic = [Diagnostic.Create ( descriptor: RBI0032, @@ -123,7 +123,7 @@ public PropertyValidator () : base (p => p.Location) descriptor: [RBI0018, RBI0019, RBI0029], validation: AccessorIsValid, propertyName: "setter"); - + // if a property is weak ensure that it starts the name with Weak or set the strondelegatename in the // export property data AddGlobalStrategy (RBI0032, WeakPropertyNameStartsWithWeak);