Skip to content

Commit 754ab14

Browse files
committed
fix: harden type-set dependency targets after review
- Normalize the resolved target set via the shared StripElementTypes, so array/by-ref/pointer types in a target selection match their element type, mirroring the specific-type overloads (an array target in DoesNotDependOn no longer passes silently). - Report type-set violations deduplicated by type identity and qualify distinct violators sharing a simple name with their namespace, so they no longer collapse into one indistinguishable entry. - Validate widening targets fully before mutating the shared options and report the actual parameter name of the caller (additional/targets). - Share the violation-collection core and the framework/own-namespace exemption predicate between the namespace-based and type-set-based helpers, so the two families cannot drift apart. - Factor the null-item predicates of the type-set collection constraints into static helpers, mirroring the namespace-based siblings.
1 parent 33f04e9 commit 754ab14

6 files changed

Lines changed: 144 additions & 69 deletions

File tree

Source/aweXpect.Reflection/Helpers/TypeHelpers.cs

Lines changed: 63 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -788,10 +788,11 @@ internal static bool MatchesType(Type dependency, Type target)
788788
/// element type.
789789
/// </summary>
790790
/// <remarks>
791-
/// Shared between dependency unwrapping (<see cref="Unwrap" />) and target matching
792-
/// (<see cref="MatchesType" />), so that the two sides of the documented symmetry cannot drift apart.
791+
/// Shared between dependency unwrapping (<see cref="Unwrap" />), target matching
792+
/// (<see cref="MatchesType" />) and target-set resolution (<see cref="TypeSetDependencyOptions.Resolve" />),
793+
/// so that the sides of the documented symmetry cannot drift apart.
793794
/// </remarks>
794-
private static Type StripElementTypes(Type type)
795+
internal static Type StripElementTypes(Type type)
795796
{
796797
while (type.HasElementType && type.GetElementType() is { } elementType)
797798
{
@@ -877,17 +878,12 @@ private static bool IsFrameworkDependency(this Type type, string[] excludedPrefi
877878
internal static IReadOnlyList<string> GetDependencyNamespaceViolations(
878879
this Type type, NamespaceDependencyOptions allowed)
879880
{
880-
string? ownNamespace = type.Namespace;
881-
string[] excludedPrefixes = Customize.aweXpect.Reflection().ExcludedAssemblyPrefixes.Get();
882881
List<string> violations = [];
883882
HashSet<string> seen = new(StringComparer.Ordinal);
884-
foreach (Type dependency in type.ResolveDependencies())
883+
foreach (Type dependency in type.GetDependencyViolations(
884+
(dependency, ownNamespace, excludedPrefixes)
885+
=> IsDependencyViolation(dependency, ownNamespace, allowed, excludedPrefixes)))
885886
{
886-
if (!IsDependencyViolation(dependency, ownNamespace, allowed, excludedPrefixes))
887-
{
888-
continue;
889-
}
890-
891887
string display = dependency.Namespace ?? GlobalNamespaceDisplay;
892888
if (seen.Add(display))
893889
{
@@ -908,24 +904,45 @@ internal static IReadOnlyList<string> GetDependencyNamespaceViolations(
908904
/// a verdict and not the violation list.
909905
/// </remarks>
910906
internal static bool HasDependencyNamespaceViolations(this Type type, NamespaceDependencyOptions allowed)
911-
{
912-
string? ownNamespace = type.Namespace;
913-
string[] excludedPrefixes = Customize.aweXpect.Reflection().ExcludedAssemblyPrefixes.Get();
914-
return type.ResolveDependencies()
915-
.Any(dependency => IsDependencyViolation(dependency, ownNamespace, allowed, excludedPrefixes));
916-
}
907+
=> type.GetDependencyViolations(
908+
(dependency, ownNamespace, excludedPrefixes)
909+
=> IsDependencyViolation(dependency, ownNamespace, allowed, excludedPrefixes)).Any();
917910

918911
private static bool IsDependencyViolation(
919912
Type dependency, string? ownNamespace, NamespaceDependencyOptions allowed, string[] excludedPrefixes)
920-
{
921-
if (dependency.IsFrameworkDependency(excludedPrefixes))
922-
{
923-
return false;
924-
}
913+
=> !IsExemptDependency(dependency, ownNamespace, allowed.IncludeOwnSubNamespaces, excludedPrefixes) &&
914+
!allowed.Matches(dependency.Namespace);
915+
916+
/// <summary>
917+
/// Checks the exemptions shared by all only-on rules: framework dependencies and dependencies in the
918+
/// type's own namespace are always allowed.
919+
/// </summary>
920+
/// <remarks>
921+
/// Shared between the namespace-based and type-set-based violation predicates, so that a future rule
922+
/// change cannot be applied to one family and missed in the other.
923+
/// </remarks>
924+
private static bool IsExemptDependency(
925+
Type dependency, string? ownNamespace, bool includeOwnSubNamespaces, string[] excludedPrefixes)
926+
=> dependency.IsFrameworkDependency(excludedPrefixes) ||
927+
IsOwnNamespace(dependency.Namespace, ownNamespace, includeOwnSubNamespaces);
925928

926-
string? dependencyNamespace = dependency.Namespace;
927-
return !IsOwnNamespace(dependencyNamespace, ownNamespace, allowed.IncludeOwnSubNamespaces) &&
928-
!allowed.Matches(dependencyNamespace);
929+
/// <summary>
930+
/// Enumerates the <paramref name="type" />'s dependencies that the <paramref name="isViolation" />
931+
/// predicate flags as violations, supplying the type's own namespace and the configured excluded
932+
/// assembly prefixes.
933+
/// </summary>
934+
/// <remarks>
935+
/// Shared core of the namespace-based and type-set-based violation helpers, so that the two families
936+
/// cannot drift apart in how the dependencies are walked. Lazy, so verdict-only callers stop at the
937+
/// first violation.
938+
/// </remarks>
939+
private static IEnumerable<Type> GetDependencyViolations(
940+
this Type type, Func<Type, string?, string[], bool> isViolation)
941+
{
942+
string? ownNamespace = type.Namespace;
943+
string[] excludedPrefixes = Customize.aweXpect.Reflection().ExcludedAssemblyPrefixes.Get();
944+
return type.ResolveDependencies()
945+
.Where(dependency => isViolation(dependency, ownNamespace, excludedPrefixes));
929946
}
930947

931948
private static bool IsOwnNamespace(string? dependencyNamespace, string? ownNamespace, bool includeSubNamespaces)
@@ -940,26 +957,30 @@ private static bool IsOwnNamespace(string? dependencyNamespace, string? ownNames
940957
/// <remarks>
941958
/// Same framework and own-namespace rules as <see cref="GetDependencyNamespaceViolations" />, but the allowed
942959
/// set is a concrete set of types, so the violations are reported as formatted type names instead of
943-
/// namespaces. Requires <see cref="TypeSetDependencyOptions.Resolve" /> to have been awaited before.
960+
/// namespaces. Distinct violators sharing a formatted name (the same simple name in different namespaces)
961+
/// are qualified by their namespace, so they do not collapse into one indistinguishable entry.
962+
/// Requires <see cref="TypeSetDependencyOptions.Resolve" /> to have been awaited before.
944963
/// </remarks>
945964
internal static IReadOnlyList<string> GetDependencyTypeSetViolations(
946965
this Type type, TypeSetDependencyOptions allowed)
947966
{
948-
string? ownNamespace = type.Namespace;
949-
string[] excludedPrefixes = Customize.aweXpect.Reflection().ExcludedAssemblyPrefixes.Get();
950967
List<string> violations = [];
951-
HashSet<string> seen = new(StringComparer.Ordinal);
952-
foreach (Type dependency in type.ResolveDependencies())
953-
{
954-
if (!IsDependencyTypeSetViolation(dependency, ownNamespace, allowed, excludedPrefixes))
968+
foreach (IGrouping<string, Type> sameName in type.GetDependencyViolations(
969+
(dependency, ownNamespace, excludedPrefixes)
970+
=> IsDependencyTypeSetViolation(dependency, ownNamespace, allowed, excludedPrefixes))
971+
.Distinct()
972+
.GroupBy(dependency => Formatter.Format(dependency), StringComparer.Ordinal))
973+
{
974+
Type[] violators = sameName.ToArray();
975+
if (violators.Length == 1)
955976
{
956-
continue;
977+
violations.Add(sameName.Key);
957978
}
958-
959-
string display = Formatter.Format(dependency);
960-
if (seen.Add(display))
979+
else
961980
{
962-
violations.Add(display);
981+
violations.AddRange(violators.Select(violator => violator.Namespace is null
982+
? sameName.Key
983+
: $"{violator.Namespace}.{sameName.Key}"));
963984
}
964985
}
965986

@@ -976,24 +997,14 @@ internal static IReadOnlyList<string> GetDependencyTypeSetViolations(
976997
/// a verdict and not the violation list.
977998
/// </remarks>
978999
internal static bool HasDependencyTypeSetViolations(this Type type, TypeSetDependencyOptions allowed)
979-
{
980-
string? ownNamespace = type.Namespace;
981-
string[] excludedPrefixes = Customize.aweXpect.Reflection().ExcludedAssemblyPrefixes.Get();
982-
return type.ResolveDependencies()
983-
.Any(dependency => IsDependencyTypeSetViolation(dependency, ownNamespace, allowed, excludedPrefixes));
984-
}
1000+
=> type.GetDependencyViolations(
1001+
(dependency, ownNamespace, excludedPrefixes)
1002+
=> IsDependencyTypeSetViolation(dependency, ownNamespace, allowed, excludedPrefixes)).Any();
9851003

9861004
private static bool IsDependencyTypeSetViolation(
9871005
Type dependency, string? ownNamespace, TypeSetDependencyOptions allowed, string[] excludedPrefixes)
988-
{
989-
if (dependency.IsFrameworkDependency(excludedPrefixes))
990-
{
991-
return false;
992-
}
993-
994-
return !IsOwnNamespace(dependency.Namespace, ownNamespace, true) &&
995-
!allowed.Matches(dependency);
996-
}
1006+
=> !IsExemptDependency(dependency, ownNamespace, true, excludedPrefixes) &&
1007+
!allowed.Matches(dependency);
9971008

9981009
/// <summary>
9991010
/// Resolves the dependencies of the <paramref name="type" /> through which all assertions and filters go,

Source/aweXpect.Reflection/Options/TypeSetDependencyOptions.cs

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public TypeSetDependencyOptions(Filtered.Types target, Filtered.Types[] addition
3434
"The additional target collections of types must not be null.");
3535
}
3636

37-
Add(additional);
37+
Add(additional, nameof(additional));
3838
}
3939

4040
private TypeSetDependencyOptions(IEnumerable<Filtered.Types> targets)
@@ -61,17 +61,18 @@ public void OrOn(Filtered.Types[] targets)
6161
throw new ArgumentException("At least one collection of types must be specified.");
6262
}
6363

64-
Add(targets);
64+
Add(targets, nameof(targets));
6565
}
6666

67-
private void Add(Filtered.Types[] targets)
67+
private void Add(Filtered.Types[] targets, string paramName)
6868
{
69-
foreach (Filtered.Types target in targets)
69+
// Fully validate before mutating, so that a failed widening leaves the shared instance untouched.
70+
if (targets.Contains(null!))
7071
{
71-
_targets.Add(target ?? throw new ArgumentNullException(nameof(targets),
72-
"The target collections of types must not contain null."));
72+
throw new ArgumentNullException(paramName, "The target collections of types must not contain null.");
7373
}
7474

75+
_targets.AddRange(targets);
7576
// Widening invalidates a previously resolved set.
7677
_resolved = null;
7778
}
@@ -80,6 +81,11 @@ private void Add(Filtered.Types[] targets)
8081
/// Resolves the target collections once into their union set; subsequent <see cref="Matches" /> /
8182
/// <see cref="IsMatchedBy" /> calls use this set.
8283
/// </summary>
84+
/// <remarks>
85+
/// Each member is normalized via <see cref="TypeHelpers.StripElementTypes" />: dependencies are stored
86+
/// element-stripped at collection time, so array/by-ref/pointer targets must be unwrapped symmetrically
87+
/// (mirroring <see cref="TypeHelpers.MatchesType" /> in the specific-type overloads).
88+
/// </remarks>
8389
#if NET8_0_OR_GREATER
8490
public async ValueTask Resolve()
8591
{
@@ -93,7 +99,7 @@ public async ValueTask Resolve()
9399
{
94100
await foreach (Type type in target)
95101
{
96-
resolved.Add(type);
102+
resolved.Add(TypeHelpers.StripElementTypes(type));
97103
}
98104
}
99105

@@ -107,7 +113,10 @@ public Task Resolve()
107113
HashSet<Type> resolved = [];
108114
foreach (Filtered.Types target in _targets)
109115
{
110-
resolved.UnionWith(target);
116+
foreach (Type type in target)
117+
{
118+
resolved.Add(TypeHelpers.StripElementTypes(type));
119+
}
111120
}
112121

113122
_resolved = resolved;

Source/aweXpect.Reflection/ThatTypes.DependOn.cs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,14 @@ private static bool DependsOnNamespace(Type? type, NamespaceDependencyOptions op
176176
private static bool DoesNotDependOnNamespace(Type? type, NamespaceDependencyOptions options)
177177
=> type is not null && !options.IsMatchedBy(type);
178178

179+
private static bool DependsOnTypeSet(Type? type, TypeSetDependencyOptions options)
180+
=> type is not null && options.IsMatchedBy(type);
181+
182+
// A null item's dependencies cannot be verified, so it fails the negative assertion just like the
183+
// positive one (and like DependOnlyOn), instead of slipping through as "does not depend".
184+
private static bool DoesNotDependOnTypeSet(Type? type, TypeSetDependencyOptions options)
185+
=> type is not null && !options.IsMatchedBy(type);
186+
179187
private sealed class DependOnConstraint(
180188
string it,
181189
ExpectationGrammars grammars,
@@ -264,14 +272,14 @@ private sealed class DependOnTypeSetConstraint(
264272
public async Task<ConstraintResult> IsMetBy(IAsyncEnumerable<Type?> actual, CancellationToken cancellationToken)
265273
{
266274
await options.Resolve();
267-
return await SetAsyncValue(actual, type => type is not null && options.IsMatchedBy(type));
275+
return await SetAsyncValue(actual, type => DependsOnTypeSet(type, options));
268276
}
269277
#endif
270278

271279
public async Task<ConstraintResult> IsMetBy(IEnumerable<Type?> actual, CancellationToken cancellationToken)
272280
{
273281
await options.Resolve();
274-
return SetValue(actual, type => type is not null && options.IsMatchedBy(type));
282+
return SetValue(actual, type => DependsOnTypeSet(type, options));
275283
}
276284

277285
protected override void AppendNormalExpectation(StringBuilder stringBuilder, string? indentation = null)
@@ -307,18 +315,14 @@ private sealed class DoNotDependOnTypeSetConstraint(
307315
public async Task<ConstraintResult> IsMetBy(IAsyncEnumerable<Type?> actual, CancellationToken cancellationToken)
308316
{
309317
await options.Resolve();
310-
// A null item's dependencies cannot be verified, so it fails the negative assertion just like the
311-
// positive one (and like DependOnlyOn), instead of slipping through as "does not depend".
312-
return await SetAsyncValue(actual, type => type is not null && !options.IsMatchedBy(type));
318+
return await SetAsyncValue(actual, type => DoesNotDependOnTypeSet(type, options));
313319
}
314320
#endif
315321

316322
public async Task<ConstraintResult> IsMetBy(IEnumerable<Type?> actual, CancellationToken cancellationToken)
317323
{
318324
await options.Resolve();
319-
// A null item's dependencies cannot be verified, so it fails the negative assertion just like the
320-
// positive one (and like DependOnlyOn), instead of slipping through as "does not depend".
321-
return SetValue(actual, type => type is not null && !options.IsMatchedBy(type));
325+
return SetValue(actual, type => DoesNotDependOnTypeSet(type, options));
322326
}
323327

324328
protected override void AppendNormalExpectation(StringBuilder stringBuilder, string? indentation = null)

Tests/aweXpect.Reflection.Tests/TestHelpers/Dependencies/DependencyFixtures.cs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,14 @@ public class ViaLayer1GenericConstruction
231231
private TargetGeneric<TargetB> _target;
232232
}
233233

234+
// Two distinct disallowed dependencies share the simple name "AmbiguousTarget"; the violation list
235+
// must keep them apart by qualifying each with its namespace.
236+
public class WithSameNamedDependencies
237+
{
238+
private AmbiguousA.AmbiguousTarget _a;
239+
private AmbiguousB.AmbiguousTarget _b;
240+
}
241+
234242
public class WithAsyncMethod
235243
{
236244
public static async void MethodAsync() => await Task.CompletedTask;
@@ -329,3 +337,15 @@ public class CovariantReturnDerived : CovariantReturnBase
329337
}
330338
#endif
331339
}
340+
341+
// Two namespaces deliberately declaring the same simple type name, so that violation messages must
342+
// disambiguate (see WithSameNamedDependencies above).
343+
namespace aweXpect.Reflection.Tests.TestHelpers.Dependencies.AmbiguousA
344+
{
345+
public class AmbiguousTarget;
346+
}
347+
348+
namespace aweXpect.Reflection.Tests.TestHelpers.Dependencies.AmbiguousB
349+
{
350+
public class AmbiguousTarget;
351+
}

Tests/aweXpect.Reflection.Tests/ThatType.DependsOn.Tests.cs

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,19 @@ async Task Act()
534534
await That(Act).DoesNotThrow();
535535
}
536536

537+
[Fact]
538+
public async Task WhenTargetCollectionContainsArrayType_ShouldMatchElementTypeDependency()
539+
{
540+
// Dependencies are stored element-stripped, so an array target matches like its element
541+
// type, mirroring the specific-type overload DependsOn(typeof(TargetA[])).
542+
Type subject = typeof(ViaField);
543+
544+
async Task Act()
545+
=> await That(subject).DependsOn(In.Type(typeof(TargetA[])));
546+
547+
await That(Act).DoesNotThrow();
548+
}
549+
537550
[Fact]
538551
public async Task WhenTargetCollectionContainsFrameworkTypes_ShouldMatchThem()
539552
{
@@ -606,8 +619,11 @@ async Task Act()
606619
=> await That(subject)
607620
.DependsOn(Types.InNamespace(Layer1Namespace), (Filtered.Types)null!, (Filtered.Types)null!);
608621

622+
// The localized paramName suffix differs between frameworks ("(Parameter 'additional')" vs
623+
// "Parametername: additional"), so only the shared part is matched.
609624
await That(Act).Throws<ArgumentNullException>()
610-
.WithMessage("The target collections of types must not contain null.*").AsWildcard();
625+
.WithMessage("The target collections of types must not contain null.*additional*")
626+
.AsWildcard();
611627
}
612628

613629
[Fact]

Tests/aweXpect.Reflection.Tests/ThatType.DependsOnlyOn.Tests.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using aweXpect.Reflection.Tests.TestHelpers.Dependencies.Consumers;
2+
using aweXpect.Reflection.Tests.TestHelpers.Dependencies.Synthetic;
23
using Xunit.Sdk;
34

45
namespace aweXpect.Reflection.Tests;
@@ -224,6 +225,20 @@ async Task Act()
224225
await That(Act).DoesNotThrow();
225226
}
226227

228+
[Fact]
229+
public async Task WhenDistinctViolatorsShareTheSimpleName_ShouldQualifyThemByNamespace()
230+
{
231+
// Both AmbiguousTarget dependencies are disallowed; they must stay apart in the message
232+
// instead of collapsing into one indistinguishable "AmbiguousTarget" entry.
233+
Type subject = typeof(WithSameNamedDependencies);
234+
235+
async Task Act()
236+
=> await That(subject).DependsOnlyOn(In.Namespace(Layer1Namespace));
237+
238+
await That(Act).Throws<XunitException>()
239+
.WithMessage("*AmbiguousA.AmbiguousTarget*AmbiguousB.AmbiguousTarget*").AsWildcard();
240+
}
241+
227242
[Fact]
228243
public async Task WhenTargetCollectionIsEmpty_ShouldStillAllowFrameworkDependencies()
229244
{

0 commit comments

Comments
 (0)