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

[Binding SG] Dogfood binding source generator in the MAUI codebase #23393

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Jul 2, 2024

Description of Change

This PR replaces as many uses of TypedBinding.ForSingleNestingLevel(...) (introduced in #20567) with the source generated interceptors where possible.

Some observations:

Issues Fixed

Contributes to #22384

/cc @jkurdek @vitek-karas @jonathanpeppers @StephaneDelcroix

Comment on lines -8 to -12
<!--
NOTE: Keep this project on C# 9 to avoid changes in overload resolution:
src/Controls/tests/Core.UnitTests/TemplatedItemsListTests.cs(543,11): error CS0121: The call is ambiguous between the following methods or properties: 'Assert.That(TestDelegate, IResolveConstraint)' and 'Assert.That<TActual>(TActual, IResolveConstraint)'
-->
<LangVersion>9.0</LangVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Did the problem mentioned here, go away somehow?

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 can't find any Assert.That call in that file. Also the tests passed just fine. This seems outdated to me.

@@ -52,6 +52,16 @@
<ItemGroup Condition="$(TargetFramework.Contains('-windows')) == true ">
</ItemGroup>

<PropertyGroup>
<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.Maui.Controls.Generated</InterceptorsPreviewNamespaces>
Copy link
Member

Choose a reason for hiding this comment

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

So, if an interceptor is declared outside of a namespace listed here, it will get a C# compiler error.

It sounds like they will rename this to $(InterceptorsNamespaces) one day:

No problem here, just fyi for later.

@simonrozsival
Copy link
Member Author

simonrozsival commented Jul 2, 2024

There is a problem on Windows in the XamlPreCompile target because it doesn't pass InterceptorsPreviewNamespaces to the Csc task.

This is a known bug: dotnet/msbuild#9785

Comment on lines +49 to +70
namespace System.Runtime.CompilerServices
{
using System;
using System.CodeDom.Compiler;

{{GeneratedCodeAttribute}}
[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
file sealed class InterceptsLocationAttribute : Attribute
{
public InterceptsLocationAttribute(string filePath, int line, int column)
{
FilePath = filePath;
Line = line;
Column = column;
}

public string FilePath { get; }
public int Line { get; }
public int Column { get; }
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This might seem like a weird change but we need to make the attribute file-scoped so that there aren't any collisions when there are assemblies with reference between them and they see each others internals (as in the case of Microsoft.Maui.Controls.Core.dll and Microsoft.Maui.Controls.Compatibility.dll

@simonrozsival
Copy link
Member Author

@StephaneDelcroix I had to update the PR to fix failing CI, please re-approve.

@simonrozsival simonrozsival enabled auto-merge (squash) July 20, 2024 11:17
@simonrozsival simonrozsival merged commit a85362c into dotnet:net9.0 Jul 23, 2024
111 checks passed
@samhouts samhouts added the fixed-in-net9.0-nightly This may be available in a nightly release! label Aug 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-net9.0-nightly This may be available in a nightly release!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants