From 6bb9f924463bcdbcb88a5bba65d2ddec1d9db172 Mon Sep 17 00:00:00 2001
From: Steven Kirk <grokys@users.noreply.github.com>
Date: Wed, 18 Oct 2023 14:37:22 +0200
Subject: [PATCH] Log errors from property accessors.

---
 .../Data/Core/BindingExpression.cs            | 46 ++++++++++++-------
 .../PropertyInfoAccessorFactory.cs            |  5 +-
 .../Data/BindingTests_Logging.cs              | 31 +++++++++++++
 3 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/src/Avalonia.Base/Data/Core/BindingExpression.cs b/src/Avalonia.Base/Data/Core/BindingExpression.cs
index c6e7576865..2dfa2a3314 100644
--- a/src/Avalonia.Base/Data/Core/BindingExpression.cs
+++ b/src/Avalonia.Base/Data/Core/BindingExpression.cs
@@ -286,6 +286,16 @@ internal class BindingExpression : IObservable<object?>,
     /// <param name="value">The <see cref="ExpressionNode.Value"/>.</param>
     internal void OnNodeValueChanged(int nodeIndex, object? value)
     {
+        if (value is BindingNotification notification && 
+            notification.ErrorType == BindingErrorType.Error &&
+            notification.Error is not null &&
+            ShouldLogError(out var target))
+        {
+            // Log any errors the arrive via a node value change. This is mainly to make sure that
+            // errors which come from property accessors get logged.
+            Log(target, notification.Error.Message, CalculateErrorPoint(nodeIndex));
+        }
+
         if (nodeIndex == _nodes.Count - 1)
         {
             // The leaf node has changed. If the binding mode is not OneWayToSource, publish the
@@ -336,15 +346,10 @@ internal class BindingExpression : IObservable<object?>,
         if (_observer is null || _mode == BindingMode.OneWayToSource)
             return;
 
-        // Build a string describing the binding chain up to the node that errored.
-        var errorPoint = new StringBuilder();
-
-        if (nodeIndex >= 0)
-            _nodes[nodeIndex].BuildString(errorPoint);
-        else
-            errorPoint.Append("(source)");
+        var errorPoint = CalculateErrorPoint(nodeIndex);
 
-        LogWarningIfNecessary(error, errorPoint.ToString());
+        if (ShouldLogError(out var target))
+            Log(target, error, errorPoint);
 
         var e = new BindingChainException(error, Description, errorPoint.ToString());
         _observer.OnNext(new BindingNotification(
@@ -353,17 +358,17 @@ internal class BindingExpression : IObservable<object?>,
             ConvertFallback(FallbackValue, nameof(FallbackValue))));
     }
 
-    private void LogWarningIfNecessary(string error, string errorPoint)
+    private string CalculateErrorPoint(int nodeIndex)
     {
-        if (!_target.TryGetTarget(out var target))
-            return;
+        // Build a string describing the binding chain up to the node that errored.
+        var result = new StringBuilder();
 
-        if (_nodes.Count > 0 &&
-            _nodes[0] is SourceNode sourceNode &&
-            !sourceNode.ShouldLogErrors(target))
-            return;
+        if (nodeIndex >= 0)
+            _nodes[nodeIndex].BuildString(result);
+        else
+            result.Append("(source)");
 
-        Log(target, error, errorPoint);
+        return result.ToString();
     }
 
     private void Log(AvaloniaObject target, string error, LogEventLevel level = LogEventLevel.Warning)
@@ -393,6 +398,15 @@ internal class BindingExpression : IObservable<object?>,
             error);
     }
 
+    private bool ShouldLogError([NotNullWhen(true)] out AvaloniaObject? target)
+    {
+        if (!_target.TryGetTarget(out target))
+            return false;
+        if (_nodes.Count > 0 && _nodes[0] is SourceNode sourceNode)
+            return sourceNode.ShouldLogErrors(target);
+        return true;
+    }
+
     private void Start()
     {
         if (_observer is null)
diff --git a/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs b/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs
index c8e65ff672..6c00df5b62 100644
--- a/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs
+++ b/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs
@@ -132,7 +132,10 @@ namespace Avalonia.Markup.Xaml.MarkupExtensions.CompiledBindings
                 var value = Value;
                 PublishValue(value);
             }
-            catch { }
+            catch (Exception e)
+            {
+                PublishValue(new BindingNotification(e, BindingErrorType.Error));
+            }
         }
 
         private void SubscribeToChanges()
diff --git a/tests/Avalonia.Markup.UnitTests/Data/BindingTests_Logging.cs b/tests/Avalonia.Markup.UnitTests/Data/BindingTests_Logging.cs
index b060271bd5..5792b98653 100644
--- a/tests/Avalonia.Markup.UnitTests/Data/BindingTests_Logging.cs
+++ b/tests/Avalonia.Markup.UnitTests/Data/BindingTests_Logging.cs
@@ -2,8 +2,12 @@ using System;
 using System.Collections.Generic;
 using Avalonia.Controls;
 using Avalonia.Data;
+using Avalonia.Data.Core;
+using Avalonia.Data.Core.Plugins.Reflection;
 using Avalonia.Input;
 using Avalonia.Logging;
+using Avalonia.Markup.Xaml.MarkupExtensions;
+using Avalonia.Markup.Xaml.MarkupExtensions.CompiledBindings;
 using Avalonia.Reactive;
 using Avalonia.UnitTests;
 using Xunit;
@@ -273,6 +277,33 @@ namespace Avalonia.Markup.UnitTests.Data
             }
         }
 
+        public class CompiledBinding
+        {
+            [Fact]
+            public void Should_Log_For_Invalid_DataContext_Type()
+            {
+                var target = new TestRoot { DataContext = 48 };
+                var stringLengthProperty = new ClrPropertyInfo(
+                    "Length",
+                    x => ((string)x).Length,
+                    null,
+                    typeof(int));
+                var bindingPath = new CompiledBindingPathBuilder()
+                    .Property(stringLengthProperty, PropertyInfoAccessorFactory.CreateInpcPropertyAccessor)
+                    .Build();
+                var binding = new CompiledBindingExtension(bindingPath);
+
+                using (AssertLog(
+                       target,
+                       bindingPath.ToString(),
+                       "Unable to cast object of type 'System.Int32' to type 'System.String'.",
+                       "Length"))
+                {
+                    target.Bind(Control.TagProperty, binding);
+                }
+            }
+        }
+
         private static IDisposable AssertLog(
             AvaloniaObject target, 
             string expression,