Skip to content

Commit 492e58e

Browse files
AndreyAkinshinAndreyAkinshin
authored
Improve ParamsValidator (#2283)
1 parent bfb1927 commit 492e58e

File tree

5 files changed

+131
-124
lines changed

5 files changed

+131
-124
lines changed

‎src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs‎

Lines changed: 19 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -158,31 +158,26 @@ private static MethodInfo[] GetBenchmarks(this TypeInfo typeInfo)
158158
.Where(method => method.GetCustomAttributes(true).OfType<BenchmarkAttribute>().Any())
159159
.ToArray();
160160

161-
internal static (string Name, TAttribute Attribute, bool IsPrivate, bool IsStatic, Type ParameterType)[] GetTypeMembersWithGivenAttribute<TAttribute>(this Type type, BindingFlags reflectionFlags)
162-
where TAttribute : Attribute
161+
internal static (string Name, TAttribute Attribute, bool IsStatic, Type ParameterType)[]
162+
GetTypeMembersWithGivenAttribute<TAttribute>(this Type type, BindingFlags reflectionFlags) where TAttribute : Attribute
163163
{
164-
var allFields = type.GetFields(reflectionFlags)
165-
.Select(f => (
166-
Name: f.Name,
167-
Attribute: f.ResolveAttribute<TAttribute>(),
168-
IsPrivate: f.IsPrivate,
169-
IsStatic: f.IsStatic,
170-
ParameterType: f.FieldType));
171-
172-
var allProperties = type.GetProperties(reflectionFlags)
173-
.Select(p => (
174-
Name: p.Name,
175-
Attribute: p.ResolveAttribute<TAttribute>(),
176-
IsPrivate: p.GetSetMethod() == null,
177-
IsStatic: p.GetSetMethod() != null && p.GetSetMethod().IsStatic,
178-
PropertyType: p.PropertyType));
179-
180-
var joined = allFields.Concat(allProperties).Where(member => member.Attribute != null).ToArray();
181-
182-
foreach (var member in joined.Where(m => m.IsPrivate))
183-
throw new InvalidOperationException($"Member \"{member.Name}\" must be public if it has the [{typeof(TAttribute).Name}] attribute applied to it");
184-
185-
return joined;
164+
var allFields = type
165+
.GetFields(reflectionFlags)
166+
.Select(f => (
167+
Name: f.Name,
168+
Attribute: f.ResolveAttribute<TAttribute>(),
169+
IsStatic: f.IsStatic,
170+
ParameterType: f.FieldType));
171+
172+
var allProperties = type
173+
.GetProperties(reflectionFlags)
174+
.Select(p => (
175+
Name: p.Name,
176+
Attribute: p.ResolveAttribute<TAttribute>(),
177+
IsStatic: p.GetSetMethod() != null && p.GetSetMethod().IsStatic,
178+
PropertyType: p.PropertyType));
179+
180+
return allFields.Concat(allProperties).Where(member => member.Attribute != null).ToArray();
186181
}
187182

188183
internal static bool IsStackOnlyWithImplicitCast(this Type argumentType, object? argumentInstance)

‎src/BenchmarkDotNet/Validators/ParamsValidator.cs‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ public IEnumerable<ValidationError> Validate(ValidationParameters input) => inpu
2020

2121
private IEnumerable<ValidationError> Validate(Type type)
2222
{
23-
foreach (var memberInfo in type.GetMembers(BindingFlags.Public | BindingFlags.Static | BindingFlags.Instance | BindingFlags.FlattenHierarchy))
23+
const BindingFlags reflectionFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.Instance |
24+
BindingFlags.FlattenHierarchy;
25+
foreach (var memberInfo in type.GetMembers(reflectionFlags))
2426
{
2527
var attributes = new Attribute[]
2628
{
@@ -40,17 +42,33 @@ private IEnumerable<ValidationError> Validate(Type type)
4042
yield return new ValidationError(TreatsWarningsAsErrors,
4143
$"Unable to use {name} with {attributeString} at the same time. Please, use a single attribute.");
4244

43-
if (memberInfo is FieldInfo fieldInfo && (fieldInfo.IsLiteral || fieldInfo.IsInitOnly))
45+
if (memberInfo is FieldInfo fieldInfo)
4446
{
45-
string modifier = fieldInfo.IsInitOnly ? "readonly" : "constant";
46-
yield return new ValidationError(TreatsWarningsAsErrors,
47-
$"Unable to use {name} with {attributeString} because it's a {modifier} field. Please, remove the {modifier} modifier.");
47+
if (fieldInfo.IsLiteral || fieldInfo.IsInitOnly)
48+
{
49+
string modifier = fieldInfo.IsInitOnly ? "readonly" : "constant";
50+
yield return new ValidationError(TreatsWarningsAsErrors,
51+
$"Unable to use {name} with {attributeString} because it's a {modifier} field. Please, remove the {modifier} modifier.");
52+
}
53+
54+
if (!fieldInfo.IsPublic)
55+
yield return new ValidationError(TreatsWarningsAsErrors,
56+
$"Unable to use {name} with {attributeString} because it's not public. Please, make it public.");
4857
}
4958

50-
if (memberInfo is PropertyInfo propertyInfo && propertyInfo.IsInitOnly())
59+
if (memberInfo is PropertyInfo propertyInfo)
5160
{
52-
yield return new ValidationError(TreatsWarningsAsErrors,
53-
$"Unable to use {name} with {attributeString} because it's init-only. Please, provide a public setter.");
61+
if (propertyInfo.IsInitOnly())
62+
yield return new ValidationError(TreatsWarningsAsErrors,
63+
$"Unable to use {name} with {attributeString} because it's init-only. Please, provide a public setter.");
64+
65+
if (propertyInfo.SetMethod == null)
66+
yield return new ValidationError(TreatsWarningsAsErrors,
67+
$"Unable to use {name} with {attributeString} because it has no setter. Please, provide a public setter.");
68+
69+
if (propertyInfo.SetMethod != null && !propertyInfo.SetMethod.IsPublic)
70+
yield return new ValidationError(TreatsWarningsAsErrors,
71+
$"Unable to use {name} with {attributeString} because its setter is not public. Please, make the setter public.");
5472
}
5573
}
5674
}

‎tests/BenchmarkDotNet.IntegrationTests/ParamsTests.cs‎

Lines changed: 0 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -30,22 +30,6 @@ public class ParamsTestProperty
3030
public void Benchmark() => Console.WriteLine($"// ### New Parameter {ParamProperty} ###");
3131
}
3232

33-
[Fact]
34-
public void ParamsDoesNotSupportPropertyWithoutPublicSetter()
35-
{
36-
// System.InvalidOperationException : Property "ParamProperty" must be public and writable if it has the [Params(..)] attribute applied to it
37-
Assert.Throws<InvalidOperationException>(() => CanExecute<ParamsTestPrivatePropertyError>());
38-
}
39-
40-
public class ParamsTestPrivatePropertyError
41-
{
42-
[Params(1, 2)]
43-
public int ParamProperty { get; private set; }
44-
45-
[Benchmark]
46-
public void Benchmark() => Console.WriteLine($"// ### New Parameter {ParamProperty} ###");
47-
}
48-
4933
[Fact]
5034
public void ParamsSupportPublicFields()
5135
{
@@ -67,22 +51,6 @@ public class ParamsTestField
6751
public void Benchmark() => Console.WriteLine($"// ### New Parameter {ParamField} ###");
6852
}
6953

70-
[Fact]
71-
public void ParamsDoesNotSupportPrivateFields()
72-
{
73-
// System.InvalidOperationException : Field "ParamField" must be public if it has the [Params(..)] attribute applied to it
74-
Assert.Throws<InvalidOperationException>(() => CanExecute<ParamsTestPrivateFieldError>());
75-
}
76-
77-
public class ParamsTestPrivateFieldError
78-
{
79-
[Params(1, 2)]
80-
private int ParamField = 0;
81-
82-
[Benchmark]
83-
public void Benchmark() => Console.WriteLine($"// ### New Parameter {ParamField} ###");
84-
}
85-
8654
public enum NestedOne
8755
{
8856
SampleValue = 1234
@@ -249,31 +217,5 @@ public void Test()
249217
throw new ArgumentException($"{nameof(StaticParamProperty)} has wrong value: {StaticParamProperty}!");
250218
}
251219
}
252-
253-
[Fact]
254-
public void ParamsPropertiesMustHavePublicSetter()
255-
=> Assert.Throws<InvalidOperationException>(() => CanExecute<WithStaticParamsPropertyWithNoPublicSetter>());
256-
257-
public class WithStaticParamsPropertyWithNoPublicSetter
258-
{
259-
[Params(3)]
260-
public static int StaticParamProperty { get; private set; }
261-
262-
[Benchmark]
263-
public int Benchmark() => StaticParamProperty;
264-
}
265-
266-
[Fact]
267-
public void ParamsFieldsMustBePublic()
268-
=> Assert.Throws<InvalidOperationException>(() => CanExecute<WithStaticPrivateParamsField>());
269-
270-
public class WithStaticPrivateParamsField
271-
{
272-
[Params(4)]
273-
private static int StaticParamField = 0;
274-
275-
[Benchmark]
276-
public int Benchmark() => StaticParamField;
277-
}
278220
}
279221
}

‎tests/BenchmarkDotNet.Tests/Validators/ExecutionValidatorTests.cs‎

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -290,40 +290,6 @@ public class NonPublicFieldWithParams
290290
public void NonThrowing() { }
291291
}
292292

293-
[Fact]
294-
public void NonPublicPropertiesWithParamsAreDiscovered()
295-
{
296-
Assert.Throws<InvalidOperationException>(
297-
() => ExecutionValidator.FailOnError.Validate(BenchmarkConverter.TypeToBenchmarks(typeof(NonPublicPropertyWithParams))));
298-
}
299-
300-
public class NonPublicPropertyWithParams
301-
{
302-
[Params(1)]
303-
[UsedImplicitly]
304-
internal int Property { get; set; }
305-
306-
[Benchmark]
307-
public void NonThrowing() { }
308-
}
309-
310-
[Fact]
311-
public void PropertyWithoutPublicSetterParamsAreDiscovered()
312-
{
313-
Assert.Throws<InvalidOperationException>(
314-
() => ExecutionValidator.FailOnError.Validate(BenchmarkConverter.TypeToBenchmarks(typeof(PropertyWithoutPublicSetterParams))));
315-
}
316-
317-
public class PropertyWithoutPublicSetterParams
318-
{
319-
[Params(1)]
320-
[UsedImplicitly]
321-
internal int Property { get; }
322-
323-
[Benchmark]
324-
public void NonThrowing() { }
325-
}
326-
327293
[Fact]
328294
public void FieldsWithoutParamsValuesAreDiscovered()
329295
{

‎tests/BenchmarkDotNet.Tests/Validators/ParamsValidatorTests.cs‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,13 @@
66
using BenchmarkDotNet.Validators;
77
using Xunit;
88
using Xunit.Abstractions;
9+
#pragma warning disable CS0414
910

1011
namespace BenchmarkDotNet.Tests.Validators
1112
{
1213
[SuppressMessage("ReSharper", "ClassNeverInstantiated.Global")]
1314
[SuppressMessage("ReSharper", "MemberCanBePrivate.Global")]
15+
[SuppressMessage("ReSharper", "UnusedAutoPropertyAccessor.Local")]
1416
public class ParamsValidatorTests
1517
{
1618
private readonly ITestOutputHelper output;
@@ -56,6 +58,18 @@ private void Check<T>(params string[] messageParts)
5658
[Fact] public void PropMultiple2Test() => Check<PropMultiple2>(nameof(PropMultiple2.Input), "single attribute", P, Ps);
5759
[Fact] public void PropMultiple3Test() => Check<PropMultiple3>(nameof(PropMultiple3.Input), "single attribute", Pa, Ps);
5860
[Fact] public void PropMultiple4Test() => Check<PropMultiple4>(nameof(PropMultiple4.Input), "single attribute", P, Pa, Ps);
61+
[Fact] public void PrivateSetter1Test() => Check<PrivateSetter1>(nameof(PrivateSetter1.Input), "setter is not public", P);
62+
[Fact] public void PrivateSetter2Test() => Check<PrivateSetter2>(nameof(PrivateSetter2.Input), "setter is not public", Pa);
63+
[Fact] public void PrivateSetter3Test() => Check<PrivateSetter3>(nameof(PrivateSetter3.Input), "setter is not public", Ps);
64+
[Fact] public void NoSetter1Test() => Check<NoSetter1>(nameof(NoSetter1.Input), "no setter", P);
65+
[Fact] public void NoSetter2Test() => Check<NoSetter2>(nameof(NoSetter2.Input), "no setter", Pa);
66+
[Fact] public void NoSetter3Test() => Check<NoSetter3>(nameof(NoSetter3.Input), "no setter", Ps);
67+
[Fact] public void InternalField1Test() => Check<InternalField1>(nameof(InternalField1.Input), "it's not public", P);
68+
[Fact] public void InternalField2Test() => Check<InternalField2>(nameof(InternalField2.Input), "it's not public", Pa);
69+
[Fact] public void InternalField3Test() => Check<InternalField3>(nameof(InternalField3.Input), "it's not public", Ps);
70+
[Fact] public void InternalProp1Test() => Check<InternalProp1>(nameof(InternalProp1.Input), "setter is not public", P);
71+
[Fact] public void InternalProp2Test() => Check<InternalProp2>(nameof(InternalProp2.Input), "setter is not public", Pa);
72+
[Fact] public void InternalProp3Test() => Check<InternalProp3>(nameof(InternalProp3.Input), "setter is not public", Ps);
5973

6074
public class Base
6175
{
@@ -119,6 +133,78 @@ public class NonStaticReadonly3 : Base
119133
public readonly bool Input = false;
120134
}
121135

136+
public class PrivateSetter1 : Base
137+
{
138+
[Params(false, true)]
139+
public bool Input { get; private set; }
140+
}
141+
142+
public class PrivateSetter2 : Base
143+
{
144+
[ParamsAllValues]
145+
public bool Input { get; private set; }
146+
}
147+
148+
public class PrivateSetter3 : Base
149+
{
150+
[ParamsSource(nameof(Source))]
151+
public bool Input { get; private set; }
152+
}
153+
154+
public class NoSetter1 : Base
155+
{
156+
[Params(false, true)]
157+
public bool Input { get; } = false;
158+
}
159+
160+
public class NoSetter2 : Base
161+
{
162+
[ParamsAllValues]
163+
public bool Input { get; } = false;
164+
}
165+
166+
public class NoSetter3 : Base
167+
{
168+
[ParamsSource(nameof(Source))]
169+
public bool Input { get; } = false;
170+
}
171+
172+
public class InternalField1 : Base
173+
{
174+
[Params(false, true)]
175+
internal bool Input = false;
176+
}
177+
178+
public class InternalField2 : Base
179+
{
180+
[ParamsAllValues]
181+
internal bool Input = false;
182+
}
183+
184+
public class InternalField3 : Base
185+
{
186+
[ParamsSource(nameof(Source))]
187+
internal bool Input = false;
188+
}
189+
190+
public class InternalProp1 : Base
191+
{
192+
[Params(false, true)]
193+
internal bool Input { get; set; }
194+
}
195+
196+
public class InternalProp2 : Base
197+
{
198+
[ParamsAllValues]
199+
internal bool Input { get; set; }
200+
}
201+
202+
public class InternalProp3 : Base
203+
{
204+
[ParamsSource(nameof(Source))]
205+
internal bool Input { get; set; }
206+
}
207+
122208
public class FieldMultiple1 : Base
123209
{
124210
[Params(false, true)]

0 commit comments

Comments
 (0)