Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ static ImmutableArray<Diagnostic> Validate<T> (BindingValidator validator, RootC
{
var bucket = ImmutableArray.CreateBuilder<Diagnostic> ();
if (declarationNode is T baseTypeDeclarationSyntax) {
var binding = Binding.FromDeclaration (baseTypeDeclarationSyntax, context);
var binding = Binding.FromDeclaration (baseTypeDeclarationSyntax, context, false);
if (binding is null) {
// add a diagnostic if the binding could not be created
bucket.Add (Diagnostic.Create (
Expand Down
54 changes: 54 additions & 0 deletions src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -443,4 +443,30 @@
<value>Wrong weak delegate</value>
</data>

<!-- RBI0033 -->

<data name="RBI0033Description" xml:space="preserve">
<value>A weak delegate has a duplicate strong delegate name.</value>
</data>
<data name="RBI0033MessageFormat" xml:space="preserve">
<value>The weak delegate '{0}' strong delegate '{1}' is already used by '{2}'</value>
<comment>{0} is the name of the property {1} is the name of the strong delegate and {2} is the name of the other property.</comment>
</data>
<data name="RBI0033Title" xml:space="preserve">
<value>Wrong strong delegate name</value>
</data>

<!-- RBI0034 -->

<data name="RBI0034Description" xml:space="preserve">
<value>A selector is used in more than one symbol.</value>
</data>
<data name="RBI0034MessageFormat" xml:space="preserve">
<value>The selector '{0}' used by '{1}' is already used by '{2}'</value>
<comment>{0} is the selector {1} is the name of the duplicate symbol and {2} is the name of the first user of the selector.</comment>
</data>
<data name="RBI0034Title" xml:space="preserve">
<value>Duplicate selector</value>
</data>

</root>
36 changes: 33 additions & 3 deletions src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ public static class RgenDiagnostics {
new LocalizableResourceString (nameof (Resources.RBI0029MessageFormat), Resources.ResourceManager,
typeof (Resources)),
"Usage",
DiagnosticSeverity.Warning,
DiagnosticSeverity.Error,
isEnabledByDefault: true,
description: new LocalizableResourceString (nameof (Resources.RBI0029Description), Resources.ResourceManager,
typeof (Resources))
Expand All @@ -461,7 +461,7 @@ public static class RgenDiagnostics {
new LocalizableResourceString (nameof (Resources.RBI0030MessageFormat), Resources.ResourceManager,
typeof (Resources)),
"Usage",
DiagnosticSeverity.Warning,
DiagnosticSeverity.Error,
isEnabledByDefault: true,
description: new LocalizableResourceString (nameof (Resources.RBI0030Description), Resources.ResourceManager,
typeof (Resources))
Expand All @@ -476,7 +476,7 @@ public static class RgenDiagnostics {
new LocalizableResourceString (nameof (Resources.RBI0031MessageFormat), Resources.ResourceManager,
typeof (Resources)),
"Usage",
DiagnosticSeverity.Warning,
DiagnosticSeverity.Error,
isEnabledByDefault: true,
description: new LocalizableResourceString (nameof (Resources.RBI0031Description), Resources.ResourceManager,
typeof (Resources))
Expand All @@ -496,4 +496,34 @@ public static class RgenDiagnostics {
description: new LocalizableResourceString (nameof (Resources.RBI0032Description), Resources.ResourceManager,
typeof (Resources))
);

/// <summary>
/// Diagnostic descriptor for when a weak delegate strong delegate is duplicated.
/// </summary>
internal static readonly DiagnosticDescriptor RBI0033 = new (
"RBI0033",
new LocalizableResourceString (nameof (Resources.RBI0033Title), Resources.ResourceManager, typeof (Resources)),
new LocalizableResourceString (nameof (Resources.RBI0033MessageFormat), Resources.ResourceManager,
typeof (Resources)),
"Usage",
DiagnosticSeverity.Error,
isEnabledByDefault: true,
description: new LocalizableResourceString (nameof (Resources.RBI0033Description), Resources.ResourceManager,
typeof (Resources))
);

/// <summary>
/// Diagnostic descriptor for when a selector is used in more than one symbol.
/// </summary>
internal static readonly DiagnosticDescriptor RBI0034 = new (
"RBI0034",
new LocalizableResourceString (nameof (Resources.RBI0034Title), Resources.ResourceManager, typeof (Resources)),
new LocalizableResourceString (nameof (Resources.RBI0034MessageFormat), Resources.ResourceManager,
typeof (Resources)),
"Usage",
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
description: new LocalizableResourceString (nameof (Resources.RBI0034Description), Resources.ResourceManager,
typeof (Resources))
);
}
10 changes: 10 additions & 0 deletions src/rgen/Microsoft.Macios.Bindings.Analyzer/Validator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ interface IValidator {
/// <param name="context">The root context for validation.</param>
/// <returns>A dictionary where the key is the name of the invalid field and the value is a list of diagnostics.</returns>
Dictionary<string, List<Diagnostic>> ValidateAll (object data, RootContext context);

/// <summary>
/// Gets all the diagnostic descriptors that this validator and its nested validators can produce.
/// </summary>
ImmutableArray<DiagnosticDescriptor> Descriptors { get; }
}

/// <summary>
Expand Down Expand Up @@ -61,6 +66,11 @@ public ImmutableArray<DiagnosticDescriptor> Descriptors {
}
}

// add the nested validators
foreach (var (_, validator) in nestedValidators) {
allDescriptors.UnionWith (validator.Descriptors);
}

return [.. allDescriptors];
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,186 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.Macios.Generator;
using Microsoft.Macios.Generator.Context;
using Microsoft.Macios.Generator.DataModel;
using static Microsoft.Macios.Generator.RgenDiagnostics;

namespace Microsoft.Macios.Bindings.Analyzer.Validators;

/// <summary>
/// Validator for class bindings.
/// </summary>
sealed class ClassValidator : BindingValidator {

readonly ArrayValidator<Property> propertiesValidator = new (new PropertyOrFieldValidator ());

/// <summary>
/// Validates that strong delegate names are unique across all properties.
/// </summary>
/// <param name="properties">The properties to validate.</param>
/// <param name="context">The root context for validation.</param>
/// <param name="diagnostics">When this method returns, contains diagnostics for any duplicate strong delegate names; otherwise, an empty array.</param>
/// <param name="location">The code location to be used for the diagnostics.</param>
/// <returns><c>true</c> if all strong delegate names are unique; otherwise, <c>false</c>.</returns>
bool StrongDelegatesAreUnique (ImmutableArray<Property> properties, RootContext context,
out ImmutableArray<Diagnostic> diagnostics, Location? location = null)
{
diagnostics = ImmutableArray<Diagnostic>.Empty;
// use a dictionary to track all the strong names and the properties that use them
var strongNames = new Dictionary<string, List<Property>> ();
foreach (var p in properties) {
var strongDelegate = p.ToStrongDelegate ();
if (strongNames.TryGetValue (strongDelegate.Name, out var list)) {
list.Add (p);
} else {
// add list with the current property since we want to use is as a ref
strongNames.Add (strongDelegate.Name, [p]);
}
}
// get all the strong names that have more than one property using them
var duplicates = strongNames.Where (x => x.Value.Count > 1).ToImmutableArray ();
if (duplicates.Length == 0) {
// no duplicates, we are good
return true;
}
// build the diagnostics
var builder = ImmutableArray.CreateBuilder<Diagnostic> ();
foreach (var duplicate in duplicates) {
// add a diagnostic for each duplicate strong delegate using the first one as a reference and the second
// one as the location of the error. We use the first one as a reference because we have to choose one and
// is the one on top of the file
var firstProperty = duplicate.Value.First ();
for (var index = 1; index < duplicate.Value.Count; index++) {
var dupProperty = duplicate.Value [index]; // used for the msg and the location
builder.Add (Diagnostic.Create (
descriptor: RBI0033,
location: dupProperty.Location,
messageArgs: [
dupProperty.Name,
duplicate.Key,
firstProperty.Name
]));
}
}
diagnostics = builder.ToImmutable ();
return diagnostics.Length == 0;
}

/// <summary>
/// Validates that selectors are unique across all properties and methods in a binding.
/// </summary>
/// <param name="binding">The binding to validate.</param>
/// <param name="context">The root context for validation.</param>
/// <param name="diagnostics">When this method returns, contains diagnostics for any duplicate selectors; otherwise, an empty array.</param>
/// <param name="location">The code location to be used for the diagnostics.</param>
/// <returns><c>true</c> if all selectors are unique; otherwise, <c>false</c>.</returns>
bool SelectorsAreUnique (Binding binding, RootContext context,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selectors can actually be duplicated, it's only a problem if they're exported, which doesn't happen if the [Sealed] attribute is present.

Here's an example:

macios/src/foundation.cs

Lines 5378 to 5388 in 052a9e9

[Internal]
[Sealed]
[Export ("replaceObjectAtIndex:withObject:")]
void _ReplaceObject (nint index, IntPtr withObject);
/// <param name="index">To be added.</param>
/// <param name="withObject">To be added.</param>
/// <summary>To be added.</summary>
/// <remarks>To be added.</remarks>
[Export ("replaceObjectAtIndex:withObject:")]
void ReplaceObject (nint index, NSObject withObject);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave it as a warning that can be disabled with a #prama so that we know it is a not common situation.

out ImmutableArray<Diagnostic> diagnostics, Location? location = null)
{
diagnostics = ImmutableArray<Diagnostic>.Empty;
var builder = ImmutableArray.CreateBuilder<Diagnostic> ();

// the logic is as follows:
// 1. Collect all selectors that we have decided to register. Those are the ones in properties and methods that
// do not have the SkipRegister attribute.
// 2. Collect the selectors based on them being static or instance selectors. We can have the same selector
// for static and instance methods, but not for two static or two instance methods.

var instanceSelectors = new Dictionary<string, List<(string SymbolName, Location? Location)>> ();
var staticSelectors = new Dictionary<string, List<(string SymbolName, Location? Location)>> ();
// collect property selectors
foreach (var property in binding.Properties) {
if (string.IsNullOrEmpty (property.Selector))
continue;
if (property.SkipRegistration)
// user has decided to skip registration for this property, so we don't need to validate it
continue;
// decide which dictionary to use based on the property being static or instance
var selectors = property.IsStatic ? staticSelectors : instanceSelectors;
if (selectors.TryGetValue (property.Selector, out var list)) {
list.Add ((property.Name, property.Location));
} else {
// add a new list with the current property
selectors.Add (property.Selector, [(property.Name, property.Location)]);
}
}

// collect method selectors
foreach (var method in binding.Methods) {
if (string.IsNullOrEmpty (method.Selector))
continue;
if (method.SkipRegistration)
// user has decided to skip registration for this method, so we don't need to validate it
continue;
var selectors = method.IsStatic ? staticSelectors : instanceSelectors;
if (selectors.TryGetValue (method.Selector, out var list)) {
list.Add ((method.Name, method.Location));
} else {
// add a new list with the current property
selectors.Add (method.Selector, [(method.Name, method.Location)]);
}
}
// get all the selectors that have more than one property or method
var instanceDuplicates = instanceSelectors.Where (x => x.Value.Count > 1).ToImmutableArray ();
var staticDuplicates = staticSelectors.Where (x => x.Value.Count > 1).ToImmutableArray ();

if (instanceDuplicates.Length == 0 && staticDuplicates.Length == 0) {
// no duplicates, we are good
return true;
}
// loop over each of the duplicates and create diagnostics for them, we do this separately for instance and
// static selectors to make it easier to read the code and to avoid mixing selectors and getting confused about
// which one is which.
BuildDiagnostics (instanceDuplicates, builder);
BuildDiagnostics (staticDuplicates, builder);

diagnostics = builder.ToImmutable ();
return diagnostics.Length == 0;

void BuildDiagnostics (ImmutableArray<KeyValuePair<string, List<(string SymbolName, Location? Location)>>> keyValuePairs,
ImmutableArray<Diagnostic>.Builder builder1)
{
foreach (var duplicate in keyValuePairs) {
var firstSymbol = duplicate.Value.First ();
for (var index = 1; index < duplicate.Value.Count; index++) {
var dupSymbol = duplicate.Value [index]; // used for the msg and the location
builder1.Add (Diagnostic.Create (
descriptor: RBI0034,
location: dupSymbol.Location,
messageArgs: [
duplicate.Key,
dupSymbol.SymbolName,
firstSymbol.SymbolName
]));
}
}
}
}

/// <summary>
/// Initializes a new instance of the <see cref="ClassValidator"/> class.
/// </summary>
public ClassValidator ()
{
// class bindings must be partial
AddGlobalStrategy (RgenDiagnostics.RBI0001, IsPartial);
AddGlobalStrategy (RBI0001, IsPartial);

// use a nested validator to validate the properties and fields individually
AddNestedValidator (b => b.Properties, propertiesValidator);

// validate that the selectors are not duplicated, this includes properties and methods
AddGlobalStrategy ([RBI0034], SelectorsAreUnique);

// validate that strong delegates are not duplicated, this is only for weak properties
AddStrategy (
b => b.Properties.Where (p => p.IsWeakDelegate).ToImmutableArray (),
[RBI0033],
StrongDelegatesAreUnique, "WeakDelegates");
}
}
Loading