Skip to content

Commit 2705bc6

Browse files
mandel-macaqueGitHub Actions Autoformatter
andauthored
[RGen] Update the class validator to deal with properties and fields. (#23656)
Add part of the class validator that will analyze a class and will check for several things in the propertites and methods: 1. Validates all fields and properties using the PropertyOrField validator. 2. Validates that WeakDelegates do not use the same strong delegate name. 3. Selectors are unique. Added a end to end test for the class analyzer. --------- Co-authored-by: GitHub Actions Autoformatter <github-actions-autoformatter@xamarin.com>
1 parent 762c0aa commit 2705bc6

File tree

10 files changed

+1138
-26
lines changed

10 files changed

+1138
-26
lines changed

src/rgen/Microsoft.Macios.Bindings.Analyzer/BindingTypeSemanticAnalyzer.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ static ImmutableArray<Diagnostic> Validate<T> (BindingValidator validator, RootC
7575
{
7676
var bucket = ImmutableArray.CreateBuilder<Diagnostic> ();
7777
if (declarationNode is T baseTypeDeclarationSyntax) {
78-
var binding = Binding.FromDeclaration (baseTypeDeclarationSyntax, context);
78+
var binding = Binding.FromDeclaration (baseTypeDeclarationSyntax, context, false);
7979
if (binding is null) {
8080
// add a diagnostic if the binding could not be created
8181
bucket.Add (Diagnostic.Create (

src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs

Lines changed: 54 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.resx

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,4 +443,30 @@
443443
<value>Wrong weak delegate</value>
444444
</data>
445445

446+
<!-- RBI0033 -->
447+
448+
<data name="RBI0033Description" xml:space="preserve">
449+
<value>A weak delegate has a duplicate strong delegate name.</value>
450+
</data>
451+
<data name="RBI0033MessageFormat" xml:space="preserve">
452+
<value>The weak delegate '{0}' strong delegate '{1}' is already used by '{2}'</value>
453+
<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>
454+
</data>
455+
<data name="RBI0033Title" xml:space="preserve">
456+
<value>Wrong strong delegate name</value>
457+
</data>
458+
459+
<!-- RBI0034 -->
460+
461+
<data name="RBI0034Description" xml:space="preserve">
462+
<value>A selector is used in more than one symbol.</value>
463+
</data>
464+
<data name="RBI0034MessageFormat" xml:space="preserve">
465+
<value>The selector '{0}' used by '{1}' is already used by '{2}'</value>
466+
<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>
467+
</data>
468+
<data name="RBI0034Title" xml:space="preserve">
469+
<value>Duplicate selector</value>
470+
</data>
471+
446472
</root>

src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ public static class RgenDiagnostics {
446446
new LocalizableResourceString (nameof (Resources.RBI0029MessageFormat), Resources.ResourceManager,
447447
typeof (Resources)),
448448
"Usage",
449-
DiagnosticSeverity.Warning,
449+
DiagnosticSeverity.Error,
450450
isEnabledByDefault: true,
451451
description: new LocalizableResourceString (nameof (Resources.RBI0029Description), Resources.ResourceManager,
452452
typeof (Resources))
@@ -461,7 +461,7 @@ public static class RgenDiagnostics {
461461
new LocalizableResourceString (nameof (Resources.RBI0030MessageFormat), Resources.ResourceManager,
462462
typeof (Resources)),
463463
"Usage",
464-
DiagnosticSeverity.Warning,
464+
DiagnosticSeverity.Error,
465465
isEnabledByDefault: true,
466466
description: new LocalizableResourceString (nameof (Resources.RBI0030Description), Resources.ResourceManager,
467467
typeof (Resources))
@@ -476,7 +476,7 @@ public static class RgenDiagnostics {
476476
new LocalizableResourceString (nameof (Resources.RBI0031MessageFormat), Resources.ResourceManager,
477477
typeof (Resources)),
478478
"Usage",
479-
DiagnosticSeverity.Warning,
479+
DiagnosticSeverity.Error,
480480
isEnabledByDefault: true,
481481
description: new LocalizableResourceString (nameof (Resources.RBI0031Description), Resources.ResourceManager,
482482
typeof (Resources))
@@ -496,4 +496,34 @@ public static class RgenDiagnostics {
496496
description: new LocalizableResourceString (nameof (Resources.RBI0032Description), Resources.ResourceManager,
497497
typeof (Resources))
498498
);
499+
500+
/// <summary>
501+
/// Diagnostic descriptor for when a weak delegate strong delegate is duplicated.
502+
/// </summary>
503+
internal static readonly DiagnosticDescriptor RBI0033 = new (
504+
"RBI0033",
505+
new LocalizableResourceString (nameof (Resources.RBI0033Title), Resources.ResourceManager, typeof (Resources)),
506+
new LocalizableResourceString (nameof (Resources.RBI0033MessageFormat), Resources.ResourceManager,
507+
typeof (Resources)),
508+
"Usage",
509+
DiagnosticSeverity.Error,
510+
isEnabledByDefault: true,
511+
description: new LocalizableResourceString (nameof (Resources.RBI0033Description), Resources.ResourceManager,
512+
typeof (Resources))
513+
);
514+
515+
/// <summary>
516+
/// Diagnostic descriptor for when a selector is used in more than one symbol.
517+
/// </summary>
518+
internal static readonly DiagnosticDescriptor RBI0034 = new (
519+
"RBI0034",
520+
new LocalizableResourceString (nameof (Resources.RBI0034Title), Resources.ResourceManager, typeof (Resources)),
521+
new LocalizableResourceString (nameof (Resources.RBI0034MessageFormat), Resources.ResourceManager,
522+
typeof (Resources)),
523+
"Usage",
524+
DiagnosticSeverity.Warning,
525+
isEnabledByDefault: true,
526+
description: new LocalizableResourceString (nameof (Resources.RBI0034Description), Resources.ResourceManager,
527+
typeof (Resources))
528+
);
499529
}

src/rgen/Microsoft.Macios.Bindings.Analyzer/Validator.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ interface IValidator {
2323
/// <param name="context">The root context for validation.</param>
2424
/// <returns>A dictionary where the key is the name of the invalid field and the value is a list of diagnostics.</returns>
2525
Dictionary<string, List<Diagnostic>> ValidateAll (object data, RootContext context);
26+
27+
/// <summary>
28+
/// Gets all the diagnostic descriptors that this validator and its nested validators can produce.
29+
/// </summary>
30+
ImmutableArray<DiagnosticDescriptor> Descriptors { get; }
2631
}
2732

2833
/// <summary>
@@ -61,6 +66,11 @@ public ImmutableArray<DiagnosticDescriptor> Descriptors {
6166
}
6267
}
6368

69+
// add the nested validators
70+
foreach (var (_, validator) in nestedValidators) {
71+
allDescriptors.UnionWith (validator.Descriptors);
72+
}
73+
6474
return [.. allDescriptors];
6575
}
6676
}
Lines changed: 167 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,186 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4+
using System.Collections.Generic;
5+
using System.Collections.Immutable;
6+
using System.Linq;
7+
using Microsoft.CodeAnalysis;
8+
using Microsoft.CodeAnalysis.CSharp;
49
using Microsoft.Macios.Generator;
10+
using Microsoft.Macios.Generator.Context;
11+
using Microsoft.Macios.Generator.DataModel;
12+
using static Microsoft.Macios.Generator.RgenDiagnostics;
513

614
namespace Microsoft.Macios.Bindings.Analyzer.Validators;
715

816
/// <summary>
917
/// Validator for class bindings.
1018
/// </summary>
1119
sealed class ClassValidator : BindingValidator {
20+
21+
readonly ArrayValidator<Property> propertiesValidator = new (new PropertyOrFieldValidator ());
22+
23+
/// <summary>
24+
/// Validates that strong delegate names are unique across all properties.
25+
/// </summary>
26+
/// <param name="properties">The properties to validate.</param>
27+
/// <param name="context">The root context for validation.</param>
28+
/// <param name="diagnostics">When this method returns, contains diagnostics for any duplicate strong delegate names; otherwise, an empty array.</param>
29+
/// <param name="location">The code location to be used for the diagnostics.</param>
30+
/// <returns><c>true</c> if all strong delegate names are unique; otherwise, <c>false</c>.</returns>
31+
bool StrongDelegatesAreUnique (ImmutableArray<Property> properties, RootContext context,
32+
out ImmutableArray<Diagnostic> diagnostics, Location? location = null)
33+
{
34+
diagnostics = ImmutableArray<Diagnostic>.Empty;
35+
// use a dictionary to track all the strong names and the properties that use them
36+
var strongNames = new Dictionary<string, List<Property>> ();
37+
foreach (var p in properties) {
38+
var strongDelegate = p.ToStrongDelegate ();
39+
if (strongNames.TryGetValue (strongDelegate.Name, out var list)) {
40+
list.Add (p);
41+
} else {
42+
// add list with the current property since we want to use is as a ref
43+
strongNames.Add (strongDelegate.Name, [p]);
44+
}
45+
}
46+
// get all the strong names that have more than one property using them
47+
var duplicates = strongNames.Where (x => x.Value.Count > 1).ToImmutableArray ();
48+
if (duplicates.Length == 0) {
49+
// no duplicates, we are good
50+
return true;
51+
}
52+
// build the diagnostics
53+
var builder = ImmutableArray.CreateBuilder<Diagnostic> ();
54+
foreach (var duplicate in duplicates) {
55+
// add a diagnostic for each duplicate strong delegate using the first one as a reference and the second
56+
// one as the location of the error. We use the first one as a reference because we have to choose one and
57+
// is the one on top of the file
58+
var firstProperty = duplicate.Value.First ();
59+
for (var index = 1; index < duplicate.Value.Count; index++) {
60+
var dupProperty = duplicate.Value [index]; // used for the msg and the location
61+
builder.Add (Diagnostic.Create (
62+
descriptor: RBI0033,
63+
location: dupProperty.Location,
64+
messageArgs: [
65+
dupProperty.Name,
66+
duplicate.Key,
67+
firstProperty.Name
68+
]));
69+
}
70+
}
71+
diagnostics = builder.ToImmutable ();
72+
return diagnostics.Length == 0;
73+
}
74+
75+
/// <summary>
76+
/// Validates that selectors are unique across all properties and methods in a binding.
77+
/// </summary>
78+
/// <param name="binding">The binding to validate.</param>
79+
/// <param name="context">The root context for validation.</param>
80+
/// <param name="diagnostics">When this method returns, contains diagnostics for any duplicate selectors; otherwise, an empty array.</param>
81+
/// <param name="location">The code location to be used for the diagnostics.</param>
82+
/// <returns><c>true</c> if all selectors are unique; otherwise, <c>false</c>.</returns>
83+
bool SelectorsAreUnique (Binding binding, RootContext context,
84+
out ImmutableArray<Diagnostic> diagnostics, Location? location = null)
85+
{
86+
diagnostics = ImmutableArray<Diagnostic>.Empty;
87+
var builder = ImmutableArray.CreateBuilder<Diagnostic> ();
88+
89+
// the logic is as follows:
90+
// 1. Collect all selectors that we have decided to register. Those are the ones in properties and methods that
91+
// do not have the SkipRegister attribute.
92+
// 2. Collect the selectors based on them being static or instance selectors. We can have the same selector
93+
// for static and instance methods, but not for two static or two instance methods.
94+
95+
var instanceSelectors = new Dictionary<string, List<(string SymbolName, Location? Location)>> ();
96+
var staticSelectors = new Dictionary<string, List<(string SymbolName, Location? Location)>> ();
97+
// collect property selectors
98+
foreach (var property in binding.Properties) {
99+
if (string.IsNullOrEmpty (property.Selector))
100+
continue;
101+
if (property.SkipRegistration)
102+
// user has decided to skip registration for this property, so we don't need to validate it
103+
continue;
104+
// decide which dictionary to use based on the property being static or instance
105+
var selectors = property.IsStatic ? staticSelectors : instanceSelectors;
106+
if (selectors.TryGetValue (property.Selector, out var list)) {
107+
list.Add ((property.Name, property.Location));
108+
} else {
109+
// add a new list with the current property
110+
selectors.Add (property.Selector, [(property.Name, property.Location)]);
111+
}
112+
}
113+
114+
// collect method selectors
115+
foreach (var method in binding.Methods) {
116+
if (string.IsNullOrEmpty (method.Selector))
117+
continue;
118+
if (method.SkipRegistration)
119+
// user has decided to skip registration for this method, so we don't need to validate it
120+
continue;
121+
var selectors = method.IsStatic ? staticSelectors : instanceSelectors;
122+
if (selectors.TryGetValue (method.Selector, out var list)) {
123+
list.Add ((method.Name, method.Location));
124+
} else {
125+
// add a new list with the current property
126+
selectors.Add (method.Selector, [(method.Name, method.Location)]);
127+
}
128+
}
129+
// get all the selectors that have more than one property or method
130+
var instanceDuplicates = instanceSelectors.Where (x => x.Value.Count > 1).ToImmutableArray ();
131+
var staticDuplicates = staticSelectors.Where (x => x.Value.Count > 1).ToImmutableArray ();
132+
133+
if (instanceDuplicates.Length == 0 && staticDuplicates.Length == 0) {
134+
// no duplicates, we are good
135+
return true;
136+
}
137+
// loop over each of the duplicates and create diagnostics for them, we do this separately for instance and
138+
// static selectors to make it easier to read the code and to avoid mixing selectors and getting confused about
139+
// which one is which.
140+
BuildDiagnostics (instanceDuplicates, builder);
141+
BuildDiagnostics (staticDuplicates, builder);
142+
143+
diagnostics = builder.ToImmutable ();
144+
return diagnostics.Length == 0;
145+
146+
void BuildDiagnostics (ImmutableArray<KeyValuePair<string, List<(string SymbolName, Location? Location)>>> keyValuePairs,
147+
ImmutableArray<Diagnostic>.Builder builder1)
148+
{
149+
foreach (var duplicate in keyValuePairs) {
150+
var firstSymbol = duplicate.Value.First ();
151+
for (var index = 1; index < duplicate.Value.Count; index++) {
152+
var dupSymbol = duplicate.Value [index]; // used for the msg and the location
153+
builder1.Add (Diagnostic.Create (
154+
descriptor: RBI0034,
155+
location: dupSymbol.Location,
156+
messageArgs: [
157+
duplicate.Key,
158+
dupSymbol.SymbolName,
159+
firstSymbol.SymbolName
160+
]));
161+
}
162+
}
163+
}
164+
}
165+
12166
/// <summary>
13167
/// Initializes a new instance of the <see cref="ClassValidator"/> class.
14168
/// </summary>
15169
public ClassValidator ()
16170
{
17171
// class bindings must be partial
18-
AddGlobalStrategy (RgenDiagnostics.RBI0001, IsPartial);
172+
AddGlobalStrategy (RBI0001, IsPartial);
173+
174+
// use a nested validator to validate the properties and fields individually
175+
AddNestedValidator (b => b.Properties, propertiesValidator);
176+
177+
// validate that the selectors are not duplicated, this includes properties and methods
178+
AddGlobalStrategy ([RBI0034], SelectorsAreUnique);
179+
180+
// validate that strong delegates are not duplicated, this is only for weak properties
181+
AddStrategy (
182+
b => b.Properties.Where (p => p.IsWeakDelegate).ToImmutableArray (),
183+
[RBI0033],
184+
StrongDelegatesAreUnique, "WeakDelegates");
19185
}
20186
}

0 commit comments

Comments
 (0)