Fix And/Or not being commutative when combining rules

ArchUnitNET/Fluent/ArchRule.cs

@@ -1,5 +1,4 @@
 using System.Collections.Generic;
-using System.Linq;
 using ArchUnitNET.Domain;
 using ArchUnitNET.Fluent.Syntax;
 
@@ -23,57 +22,32 @@ public ArchRule<TRuleType> WithoutRequiringPositiveResults()
 
         public bool HasNoViolations(Architecture architecture)
         {
-            if (_ruleCreator.RequirePositiveResults)
-            {
-                return Evaluate(architecture).All(e => e.Passed);
-            }
-            else
-            {
-                return _ruleCreator.HasNoViolations(architecture);
-            }
+            return _ruleCreator.HasNoViolations(architecture);
         }
 
         public IEnumerable<EvaluationResult> Evaluate(Architecture architecture)
         {
-            var result = _ruleCreator.Evaluate(architecture).ToList();
-
-            // To require positives, we only ever need to add
-            // a non-passing result if there are no results.
-            if (_ruleCreator.RequirePositiveResults && result.Count == 0)
-            {
-                result.Add(
-                    new EvaluationResult(
-                        this,
-                        new StringIdentifier(Description),
-                        false,
-                        $"The rule requires positive evaluation, not just absence of violations. Use {nameof(WithoutRequiringPositiveResults)}() or improve your rule's predicates.",
-                        this,
-                        architecture
-                    )
-                );
-            }
-
-            return result;
+            return _ruleCreator.Evaluate(architecture);
         }
 
         public CombinedArchRuleDefinition And()
         {
-            return new CombinedArchRuleDefinition(_ruleCreator, LogicalConjunctionDefinition.And);
+            return new CombinedArchRuleDefinition(this, LogicalConjunctionDefinition.And);
         }
 
         public CombinedArchRuleDefinition Or()
         {
-            return new CombinedArchRuleDefinition(_ruleCreator, LogicalConjunctionDefinition.Or);
+            return new CombinedArchRuleDefinition(this, LogicalConjunctionDefinition.Or);
         }
 
         public IArchRule And(IArchRule archRule)
         {
-            return new CombinedArchRule(_ruleCreator, LogicalConjunctionDefinition.And, archRule);
+            return new CombinedArchRule(this, LogicalConjunctionDefinition.And, archRule);
         }
 
         public IArchRule Or(IArchRule archRule)
         {
-            return new CombinedArchRule(_ruleCreator, LogicalConjunctionDefinition.Or, archRule);
+            return new CombinedArchRule(this, LogicalConjunctionDefinition.Or, archRule);
         }
     }
 }

ArchUnitNET/Fluent/ArchRuleCreator.cs

@@ -25,12 +25,29 @@ public ArchRuleCreator(BasicObjectProvider<TRuleType> basicObjectProvider)
 
         public bool HasNoViolations(Architecture architecture)
         {
-            return HasNoViolations(GetAnalyzedObjects(architecture), architecture);
+            return Evaluate(architecture).All(result => result.Passed);
         }
 
         public IEnumerable<EvaluationResult> Evaluate(Architecture architecture)
         {
-            return EvaluateConditions(GetAnalyzedObjects(architecture), architecture);
+            var filteredObjects = GetAnalyzedObjects(architecture).ToList();
+            var results = EvaluateConditions(filteredObjects, architecture).ToList();
+            if (RequirePositiveResults && results.Count == 0)
+            {
+                return new[]
+                {
+                    new EvaluationResult(
+                        this,
+                        new StringIdentifier(Description),
+                        false,
+                        "The rule requires positive evaluation, not just absence of violations. Use WithoutRequiringPositiveResults() or improve your rule's predicates.",
+                        this,
+                        architecture
+                    ),
+                };
+            }
+
+            return results;
         }
 
         public void AddPredicate(IPredicate<TRuleType> predicate)
@@ -96,7 +113,10 @@ public void SetCustomConditionDescription(string description)
         private void SetRequirePositiveResults(bool requirePositive)
         {
             if (_requirePositiveResults != null && _requirePositiveResults != requirePositive)
+            {
                 throw new InvalidOperationException("conflicting positive expectation");
+            }
+
             _requirePositiveResults = requirePositive;
         }
 
@@ -106,14 +126,6 @@ public bool RequirePositiveResults
             set => SetRequirePositiveResults(value);
         }
 
-        private bool HasNoViolations(
-            IEnumerable<TRuleType> filteredObjects,
-            Architecture architecture
-        )
-        {
-            return EvaluateConditions(filteredObjects, architecture).All(result => result.Passed);
-        }
-
         private IEnumerable<EvaluationResult> EvaluateConditions(
             IEnumerable<TRuleType> filteredObjects,
             Architecture architecture

ArchUnitNET/Fluent/CombinedArchRuleCreator.cs

@@ -1,5 +1,4 @@
-using System;
-using System.Collections.Generic;
+using System.Collections.Generic;
 using System.Linq;
 using ArchUnitNET.Domain;
 using ArchUnitNET.Fluent.Conditions;
@@ -14,7 +13,6 @@ public class CombinedArchRuleCreator<TRuleType> : IArchRuleCreator<TRuleType>
         private readonly ArchRuleCreator<TRuleType> _currentArchRuleCreator;
         private readonly LogicalConjunction _logicalConjunction;
         private readonly ICanBeEvaluated _oldRule;
-        private bool? _requirePositiveResults;
 
         public CombinedArchRuleCreator(
             ICanBeEvaluated oldRule,
@@ -111,17 +109,10 @@ public void SetCustomConditionDescription(string description)
             _currentArchRuleCreator.SetCustomConditionDescription(description);
         }
 
-        private void SetRequirePositiveResults(bool requirePositive)
-        {
-            if (_requirePositiveResults != null && _requirePositiveResults != requirePositive)
-                throw new InvalidOperationException("conflicting positive expectation");
-            _requirePositiveResults = requirePositive;
-        }
-
         public bool RequirePositiveResults
         {
-            get => _requirePositiveResults ?? true;
-            set => SetRequirePositiveResults(value);
+            get => _currentArchRuleCreator.RequirePositiveResults;
+            set => _currentArchRuleCreator.RequirePositiveResults = value;
         }
 
         public override string ToString()

ArchUnitNET/Fluent/Slices/SliceRule.cs

@@ -26,22 +26,22 @@ public IEnumerable<EvaluationResult> Evaluate(Architecture architecture)
 
         public CombinedArchRuleDefinition And()
         {
-            return new CombinedArchRuleDefinition(_ruleCreator, LogicalConjunctionDefinition.And);
+            return new CombinedArchRuleDefinition(this, LogicalConjunctionDefinition.And);
         }
 
         public CombinedArchRuleDefinition Or()
         {
-            return new CombinedArchRuleDefinition(_ruleCreator, LogicalConjunctionDefinition.Or);
+            return new CombinedArchRuleDefinition(this, LogicalConjunctionDefinition.Or);
         }
 
         public IArchRule And(IArchRule archRule)
         {
-            return new CombinedArchRule(_ruleCreator, LogicalConjunctionDefinition.And, archRule);
+            return new CombinedArchRule(this, LogicalConjunctionDefinition.And, archRule);
         }
 
         public IArchRule Or(IArchRule archRule)
         {
-            return new CombinedArchRule(_ruleCreator, LogicalConjunctionDefinition.Or, archRule);
+            return new CombinedArchRule(this, LogicalConjunctionDefinition.Or, archRule);
         }
     }
 }

ArchUnitNETTests/Fluent/Syntax/Elements/LogicalConjunctionTests.cs

@@ -680,6 +680,200 @@ public void OrTest()
             Assert.True(otherCondition2OrThisCondition2.HasNoViolations(Architecture));
             Assert.True(falseThisCondition1OrFalseThisCondition2.HasNoViolations(Architecture));
         }
+
+        [Fact]
+        public void Or_WithArchRule_IsCommutative()
+        {
+            // TT
+            Assert.True(ThisClassExists.Or(OtherCondition1).HasNoViolations(Architecture));
+            Assert.True(OtherCondition1.Or(ThisClassExists).HasNoViolations(Architecture));
+
+            // TF / FT
+            Assert.True(ThisClassExists.Or(ThisClassDoesNotExist).HasNoViolations(Architecture));
+            Assert.True(ThisClassDoesNotExist.Or(ThisClassExists).HasNoViolations(Architecture));
+
+            // FF
+            Assert.False(
+                ThisClassDoesNotExist.Or(FalseThisShouldCondition1).HasNoViolations(Architecture)
+            );
+            Assert.False(
+                FalseThisShouldCondition1.Or(ThisClassDoesNotExist).HasNoViolations(Architecture)
+            );
+
+            // Empty-predicate rule (no matching objects): counts as failing with RequirePositiveResults
+            var emptyRule = Classes()
+                .That()
+                .HaveName("NotTheNameOfAnyObject")
+                .Should()
+                .Be(ThisClass);
+            Assert.False(emptyRule.HasNoViolations(Architecture));
+            Assert.False(emptyRule.Or(ThisClassDoesNotExist).HasNoViolations(Architecture));
+            Assert.False(ThisClassDoesNotExist.Or(emptyRule).HasNoViolations(Architecture));
+        }
+
+        [Fact]
+        public void And_WithArchRule_IsCommutative()
+        {
+            // TT
+            Assert.True(ThisClassExists.And(OtherCondition1).HasNoViolations(Architecture));
+            Assert.True(OtherCondition1.And(ThisClassExists).HasNoViolations(Architecture));
+
+            // TF / FT
+            Assert.False(ThisClassExists.And(ThisClassDoesNotExist).HasNoViolations(Architecture));
+            Assert.False(ThisClassDoesNotExist.And(ThisClassExists).HasNoViolations(Architecture));
+
+            // FF
+            Assert.False(
+                ThisClassDoesNotExist.And(FalseThisShouldCondition1).HasNoViolations(Architecture)
+            );
+            Assert.False(
+                FalseThisShouldCondition1.And(ThisClassDoesNotExist).HasNoViolations(Architecture)
+            );
+
+            // Empty-predicate rule (no matching objects): counts as failing with RequirePositiveResults
+            var emptyRule = Classes()
+                .That()
+                .HaveName("NotTheNameOfAnyObject")
+                .Should()
+                .Be(ThisClass);
+            Assert.False(emptyRule.HasNoViolations(Architecture));
+            Assert.False(emptyRule.And(ThisClassExists).HasNoViolations(Architecture));
+            Assert.False(ThisClassExists.And(emptyRule).HasNoViolations(Architecture));
+        }
+
+        // rule.Or(other) and rule.Or().other must give the same verdict
+        [Fact]
+        public void Or_FluentAndDirectForms_AreConsistent()
+        {
+            // FT: first fails, second passes
+            Assert.True(ThisClassDoesNotExist.Or(OtherCondition1).HasNoViolations(Architecture));
+            Assert.True(
+                ThisClassDoesNotExist
+                    .Or()
+                    .Classes()
+                    .That()
+                    .Are(OtherClass)
+                    .Should()
+                    .Be(OtherClass)
+                    .HasNoViolations(Architecture)
+            );
+
+            // TF: first passes, second fails
+            Assert.True(OtherCondition1.Or(ThisClassDoesNotExist).HasNoViolations(Architecture));
+            Assert.True(
+                OtherCondition1
+                    .Or()
+                    .Classes()
+                    .That()
+                    .Are(ThisClass)
+                    .Should()
+                    .NotBe(ThisClass)
+                    .HasNoViolations(Architecture)
+            );
+
+            // FF
+            Assert.False(
+                ThisClassDoesNotExist.Or(FalseThisShouldCondition1).HasNoViolations(Architecture)
+            );
+            Assert.False(
+                ThisClassDoesNotExist
+                    .Or()
+                    .Classes()
+                    .That()
+                    .Are(ThisClass)
+                    .Should()
+                    .NotBe(ThisClass)
+                    .HasNoViolations(Architecture)
+            );
+        }
+
+        [Fact]
+        public void And_FluentFormWithEmptySecondRule_ReportsFailingResult()
+        {
+            var thisClassExistsAndEmptyRule = ThisClassExists
+                .And()
+                .Classes()
+                .That()
+                .HaveName("NotTheNameOfAnyObject")
+                .Should()
+                .Be(ThisClass);
+
+            Assert.False(thisClassExistsAndEmptyRule.HasNoViolations(Architecture));
+            Assert.Contains(thisClassExistsAndEmptyRule.Evaluate(Architecture), e => !e.Passed);
+        }
+
+        // rule.And(other) and rule.And().other must give the same verdict
+        [Fact]
+        public void And_FluentAndDirectForms_AreConsistent()
+        {
+            // TT
+            Assert.True(ThisClassExists.And(OtherCondition1).HasNoViolations(Architecture));
+            Assert.True(
+                ThisClassExists
+                    .And()
+                    .Classes()
+                    .That()
+                    .Are(OtherClass)
+                    .Should()
+                    .Be(OtherClass)
+                    .HasNoViolations(Architecture)
+            );
+
+            // TF
+            Assert.False(ThisClassExists.And(ThisClassDoesNotExist).HasNoViolations(Architecture));
+            Assert.False(
+                ThisClassExists
+                    .And()
+                    .Classes()
+                    .That()
+                    .Are(ThisClass)
+                    .Should()
+                    .NotBe(ThisClass)
+                    .HasNoViolations(Architecture)
+            );
+
+            // FT
+            Assert.False(ThisClassDoesNotExist.And(OtherCondition1).HasNoViolations(Architecture));
+            Assert.False(
+                ThisClassDoesNotExist
+                    .And()
+                    .Classes()
+                    .That()
+                    .Are(OtherClass)
+                    .Should()
+                    .Be(OtherClass)
+                    .HasNoViolations(Architecture)
+            );
+        }
+
+        [Fact]
+        public void And_FluentFormWithNotExistOnEmptySecondRule_Passes()
+        {
+            var thisClassExistsAndNoClassNamedXExists = ThisClassExists
+                .And()
+                .Classes()
+                .That()
+                .HaveName("NotTheNameOfAnyObject")
+                .Should()
+                .NotExist();
+
+            Assert.True(thisClassExistsAndNoClassNamedXExists.HasNoViolations(Architecture));
+        }
+
+        [Fact]
+        public void And_FluentFormWithEmptySecondRule_WithoutRequiringPositiveResults_Passes()
+        {
+            var thisClassExistsAndEmptyRule = ThisClassExists
+                .And()
+                .Classes()
+                .That()
+                .HaveName("NotTheNameOfAnyObject")
+                .Should()
+                .Be(ThisClass)
+                .WithoutRequiringPositiveResults();
+
+            Assert.True(thisClassExistsAndEmptyRule.HasNoViolations(Architecture));
+        }
     }
 
     internal class OtherClassForLogicalConjunctionTest { }