Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[XC] Produce warning when x:DataType is inherited from outer scope of DataTemplate #22803

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Controls/src/Build.Tasks/BuildException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class BuildExceptionCode
public static BuildExceptionCode BPMissingGetter = new BuildExceptionCode("XC", 0021, nameof(BPMissingGetter), "");
public static BuildExceptionCode BindingWithoutDataType = new BuildExceptionCode("XC", 0022, nameof(BindingWithoutDataType), "https://learn.microsoft.com/dotnet/maui/fundamentals/data-binding/compiled-bindings"); //warning
public static BuildExceptionCode BindingWithNullDataType = new BuildExceptionCode("XC", 0023, nameof(BindingWithNullDataType), "https://learn.microsoft.com/dotnet/maui/fundamentals/data-binding/compiled-bindings"); //warning
public static BuildExceptionCode BindingWithXDataTypeFromOuterScope = new BuildExceptionCode("XC", 0024, nameof(BindingWithXDataTypeFromOuterScope), "https://learn.microsoft.com/dotnet/maui/fundamentals/data-binding/compiled-bindings"); //warning

//Bindings, conversions
public static BuildExceptionCode Conversion = new BuildExceptionCode("XC", 0040, nameof(Conversion), "");
Expand Down
7 changes: 5 additions & 2 deletions src/Controls/src/Build.Tasks/ErrorMessages.resx
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,13 @@
</data>
<data name="BindingWithoutDataType" xml:space="preserve">
<value>Binding could be compiled to improve runtime performance if x:DataType is specified. See https://learn.microsoft.com/dotnet/maui/fundamentals/data-binding/compiled-bindings for more information.</value>
</data>
</data>
<data name="BindingWithNullDataType" xml:space="preserve">
<value>Binding could be compiled to improve runtime performance if x:DataType is not explicitly null. See https://learn.microsoft.com/dotnet/maui/fundamentals/data-binding/compiled-bindings for more information.</value>
</data>
</data>
<data name="BindingWithXDataTypeFromOuterScope" xml:space="preserve">
<value>Binding might be compiled incorrectly since the x:DataType annotation comes from an outer scope. Make sure you annotate all DataTemplate XAML elements with the correct x:DataType. See https://learn.microsoft.com/dotnet/maui/fundamentals/data-binding/compiled-bindings for more information.</value>
</data>
<data name="BindingPropertyNotFound" xml:space="preserve">
<value>Binding: Property "{0}" not found on "{1}".</value>
<comment>0 is property name, 1 is type name</comment>
Expand Down
13 changes: 13 additions & 0 deletions src/Controls/src/Build.Tasks/SetPropertiesVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -417,13 +417,20 @@ static IEnumerable<Instruction> CompileBindingPath(ElementNode node, ILContext c
skipNode = GetParent(node);
}

bool xDataTypeIsInOuterScope = false;
while (n != null)
{
if (n != skipNode && n.Properties.TryGetValue(XmlName.xDataType, out dataTypeNode))
{
break;
}

if (n.XmlType.Name == nameof(Microsoft.Maui.Controls.DataTemplate)
&& n.XmlType.NamespaceUri == XamlParser.MauiUri)
Comment on lines +428 to +429
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other types of scopes we would need future changes for?

Wondering if any of these have an issue:

  • ResourceDictionary
  • ControlTemplate

In theory, you could have a {Binding} inside a Style in a {ResourceDictionary} and the type could be different based on the XAML using the Style? Maybe the existing warning already works for this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this applies to ResourceDictionary (BTW we're not compiling these bindings at the moment: #22023), but it might apply to ControlTemplate 🤔 In general, this should be whenever there's a different BindingContext in the outer scope and in the inner scope. What do you think, @StephaneDelcroix?

{
xDataTypeIsInOuterScope = true;
}

n = GetParent(n);
}

Expand All @@ -434,6 +441,12 @@ static IEnumerable<Instruction> CompileBindingPath(ElementNode node, ILContext c
yield break;
}

if (xDataTypeIsInOuterScope)
{
context.LoggingHelper.LogWarningOrError(BuildExceptionCode.BindingWithXDataTypeFromOuterScope, context.XamlFilePath, node.LineNumber, node.LinePosition, 0, 0, null);
// continue compilation
}

if (dataTypeNode is ElementNode enode
&& enode.XmlType.NamespaceUri == XamlParser.X2009Uri
&& enode.XmlType.Name == nameof(Microsoft.Maui.Controls.Xaml.NullExtension))
Expand Down
16 changes: 13 additions & 3 deletions src/Controls/src/Build.Tasks/XamlCTask.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ class LoggingHelperContext
public IList<int> WarningsAsErrors { get; set; }
public IList<int> WarningsNotAsErrors { get; set; }
public IList<int> NoWarn { get; set; }
public bool HasLoggedError { get; set; }
public string PathPrefix { get; set; }
}

static LoggingHelperContext Context { get; set; }
internal static List<BuildException> LoggedErrors { get; set; }

public static void SetContext(
this TaskLoggingHelper loggingHelper,
Expand Down Expand Up @@ -98,7 +98,8 @@ public static void LogWarningOrError(this TaskLoggingHelper loggingHelper, Build
|| (Context.WarningsAsErrors != null && Context.WarningsAsErrors.Contains(code.CodeCode)))
{
loggingHelper.LogError("XamlC", $"{code.CodePrefix}{code.CodeCode:0000}", code.HelpLink, xamlFilePath, lineNumber, linePosition, endLineNumber, endLinePosition, ErrorMessages.ResourceManager.GetString(code.ErrorMessageKey), messageArgs);
Context.HasLoggedError = true;
LoggedErrors ??= new();
LoggedErrors.Add(new BuildException(code, new XmlLineInfo(lineNumber, linePosition), innerException: null));
}
else
{
Expand Down Expand Up @@ -309,7 +310,16 @@ public override bool Execute(out IList<Exception> thrownExceptions)
}
else
{
success &= !LoggingHelper.HasLoggedErrors;
if (LoggingHelperExtensions.LoggedErrors is List<BuildException> errors)
{
foreach (var error in errors)
{
success = false;
(thrownExceptions = thrownExceptions ?? new List<Exception>()).Add(error);
}

LoggingHelperExtensions.LoggedErrors = null;
}
}

if (initComp.HasCustomAttributes)
Expand Down
28 changes: 28 additions & 0 deletions src/Controls/tests/Xaml.UnitTests/Issues/Maui22714.xaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?xml version="1.0" encoding="UTF-8"?>
<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Microsoft.Maui.Controls.Xaml.UnitTests.Maui22714"
xmlns:local="clr-namespace:Microsoft.Maui.Controls.Xaml.UnitTests">

<StackLayout>
<ScrollView x:DataType="local:PageViewModel22714">
<StackLayout BindableLayout.ItemsSource="{Binding Items}">
<BindableLayout.ItemTemplate>
<DataTemplate x:DataType="local:ItemViewModel22714">
<Label Text="{Binding Title}" />
</DataTemplate>
</BindableLayout.ItemTemplate>
</StackLayout>
</ScrollView>

<ScrollView x:DataType="local:PageViewModel22714">
<StackLayout BindableLayout.ItemsSource="{Binding Items}">
<BindableLayout.ItemTemplate>
<DataTemplate>
<Label Text="{Binding Title}" />
</DataTemplate>
</BindableLayout.ItemTemplate>
</StackLayout>
</ScrollView>
</StackLayout>
</ContentPage>
71 changes: 71 additions & 0 deletions src/Controls/tests/Xaml.UnitTests/Issues/Maui22714.xaml.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
using Microsoft.Maui.ApplicationModel;
using Microsoft.Maui.Dispatching;

using Microsoft.Maui.Controls.Core.UnitTests;
using Microsoft.Maui.UnitTests;
using NUnit.Framework;

namespace Microsoft.Maui.Controls.Xaml.UnitTests;

[XamlCompilation(XamlCompilationOptions.Skip)]
public partial class Maui22714
{
public Maui22714()
{
InitializeComponent();
}

public Maui22714(bool useCompiledXaml)
{
//this stub will be replaced at compile time
}

[TestFixture]
public class Test
{
[SetUp]
public void Setup()
{
Application.SetCurrentApplication(new MockApplication());
DispatcherProvider.SetCurrent(new DispatcherProviderStub());
}

[TearDown] public void TearDown() => AppInfo.SetCurrent(null);

[Test]
public void TestNonCompiledResourceDictionary(
[Values(false, true)] bool useCompiledXaml,
[Values(false, true)] bool treatWarningsAsErrors)
{
if (useCompiledXaml)
{
if (treatWarningsAsErrors)
{
Assert.Throws(
new BuildExceptionConstraint(22, 32, static msg => msg.Contains(" XC0024 ", System.StringComparison.Ordinal)),
() => MockCompiler.Compile(typeof(Maui22714), treatWarningsAsErrors: true));
}
else
{
MockCompiler.Compile(typeof(Maui22714), treatWarningsAsErrors: false);
}
}
else
{
// This will never affect non-compiled builds
_ = new Maui22714(useCompiledXaml);
}
}
}
}

public class PageViewModel22714
{
public string Title { get; set; }
public ItemViewModel22714[] Items { get; set; }
}

public class ItemViewModel22714
{
public string Title { get; set; }
}
7 changes: 4 additions & 3 deletions src/Controls/tests/Xaml.UnitTests/MockCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ namespace Microsoft.Maui.Controls.Xaml.UnitTests
{
public static class MockCompiler
{
public static void Compile(Type type, string targetFramework = null)
public static void Compile(Type type, string targetFramework = null, bool treatWarningsAsErrors = false)
{
Compile(type, out _, targetFramework);
Compile(type, out _, targetFramework, treatWarningsAsErrors);
}

public static void Compile(Type type, out MethodDefinition methodDefinition, string targetFramework = null)
public static void Compile(Type type, out MethodDefinition methodDefinition, string targetFramework = null, bool treatWarningsAsErrors = false)
{
methodDefinition = null;
var assembly = type.Assembly.Location;
Expand All @@ -32,6 +32,7 @@ public static void Compile(Type type, out MethodDefinition methodDefinition, str
ValidateOnly = true,
Type = type.FullName,
TargetFramework = targetFramework,
TreatWarningsAsErrors = treatWarningsAsErrors,
BuildEngine = new MSBuild.UnitTests.DummyBuildEngine()
};

Expand Down
Loading