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

Mark interceptors feature as stable and deprecate file path based attributes #76312

Merged
merged 18 commits into from
Dec 13, 2024

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Dec 6, 2024

Related to #72855

  • Update feature doc to indicate stability of the feature.
  • Remove [Experimental(RoslynExperiments.Interceptors, ...)] attributes from public APIs.
  • Add a warning when [InterceptsLocation(path, line, column)] is used in user code.
    • After we have had a long enough notice period, we will completely rip out support for this.
  • Migrate tests to use [InterceptsLocation(version, data)] where possible.
    • I considered several ways to not have to churn tons of tests but eventually decided to just bite the bullet right here. This brings us way closer to being able to rip out the path-based attribute support anyway.
    • For several tests where this didn't make sense, I simply added the new warnings to the test baselines. Such tests will likely be deleted when support is removed.
    • Filed Interceptors should not internally identify calls using paths #76341 to follow up on an issue I observed when porting the tests.

@RikkiGibson RikkiGibson requested a review from a team as a code owner December 6, 2024 23:28
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 6, 2024
@RikkiGibson RikkiGibson requested a review from a team as a code owner December 9, 2024 17:28
@dotnet-policy-service dotnet-policy-service bot added the Needs API Review Needs to be reviewed by the API review council label Dec 9, 2024
@jcouv jcouv assigned jcouv and cston Dec 11, 2024
@RikkiGibson
Copy link
Contributor Author

@jcouv @cston does our policy require two reviews on the test changes in this PR?

@jcouv
Copy link
Member

jcouv commented Dec 12, 2024

Yes, this requires two sign-offs. From quick glance, I see at least one product code change diagnostics.Add(ErrorCode.WRN_InterceptsLocationAttributeUnsupportedSignature, attributeLocation);

@@ -14467,7 +14467,18 @@ public InterceptsLocationAttribute(string filePath, int line, int character)
""";
var generator = new SingleFileTestGenerator(generatedSource, "Generated.cs");

VerifyOutput(dir, src, includeCurrentAssemblyAsAnalyzerReference: false, additionalFlags: ["/langversion:preview", "/out:embed.exe", "/features:InterceptorsNamespaces=Generated"], generators: [generator], analyzers: null);
// Future note: it would be good to have end-to-end tests using InterceptableLocation APIs.
Copy link
Member

Choose a reason for hiding this comment

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

Future note

Would it make sense to reference #72855 here, so that we consider adding end-to-end tests before closing that issue?

Copy link
Member

Choose a reason for hiding this comment

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

Is EndToEndTests.Interceptors() that test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EndToEndTests.Interceptors is more of a load test, while these are more like integration tests (does configuration flow properly thru the command line, into CommonCompiler, into generators, and allow producing a valid final compilation).

The final version of the tests in this file would likely look a little more like real world versions of the "Usage Examples" section of #72133.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, we will be prompted to revisit these tests when support for the path-based attributes is dropped, since the tests will stop working completely. Happy to add the link, but I am also not worried about forgetting about them.

@cston
Copy link
Member

cston commented Dec 12, 2024

    // note: it seems a little surprising that this warning is reported, but it's not important enough to fix.

Don't we normally report the diagnostics from any CompilationReference instances? If so, let's remove this comment.


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs:591 in 99a9290. [](commit_id = 99a9290, deletion_comment = False)

@cston
Copy link
Member

cston commented Dec 12, 2024

    var compilation = CreateCompilation([source, interceptors, s_attributesSource], parseOptions: RegularWithInterceptors);

Should source be named "Program.cs"? (It looks like the ERR_InterceptorPathNotInCompilation errors are unrelated to the original test.)


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs:1472 in 99a9290. [](commit_id = 99a9290, deletion_comment = False)

@@ -3,7 +3,7 @@
## Summary
[summary]: #summary

*Interceptors* are an experimental compiler feature planned to ship in .NET 8 (with support for C# only). The feature may be subject to breaking changes or removal in a future release.
*Interceptors* is a C# compiler feature, first shipped experimentally in .NET 8.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Consider recording the version where it'll be "shipped"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am expecting the "stable" designation to arrive in 9.0.2xx, so that's what I'll record here.

@@ -1171,6 +1170,8 @@ private void DecodeInterceptsLocationAttributeExperimentalCompat(
var attributeSyntax = arguments.AttributeSyntaxOpt;
Debug.Assert(attributeSyntax is object);
var attributeLocation = attributeSyntax.Location;
diagnostics.Add(ErrorCode.WRN_InterceptsLocationAttributeUnsupportedSignature, attributeLocation);
Copy link
Member

Choose a reason for hiding this comment

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

We've added a warning, but it doesn't look like support was actually removed. Is that intended? Should we keep the tracking issue to remove the logic below at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, the warning is here to give folks notice that we will fully eliminate support here. At which point the warning will likely be upgraded to error.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 15)

@RikkiGibson RikkiGibson merged commit afdd413 into dotnet:main Dec 13, 2024
24 checks passed
@RikkiGibson RikkiGibson deleted the interceptors-stable branch December 13, 2024 19:00
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Dec 13, 2024
@dibarbet dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025
jonathanpeppers added a commit to dotnet/maui that referenced this pull request Jan 13, 2025
Changes required:

* Get appropriate `net9` branded workload manifests from dotnet/runtime

* Fix NuGet package downgrades:

    src\SingleProject\Resizetizer\src\Resizetizer.csproj : error NU1605:
      Warning As Error: Detected package downgrade: System.Runtime.CompilerServices.Unsafe from 6.1.0 to 6.0.0. Reference the package directly from the project to select a different version.
       Microsoft.Maui.Resizetizer -> Microsoft.Bcl.AsyncInterfaces 10.0.0-alpha.1.25058.4 -> System.Threading.Tasks.Extensions 4.6.0 -> System.Runtime.CompilerServices.Unsafe (>= 6.1.0)
       Microsoft.Maui.Resizetizer -> System.Runtime.CompilerServices.Unsafe (>= 6.0.0)
    src\Core\src\Core.csproj : error NU1605:
      Warning As Error: Detected package downgrade: System.Numerics.Vectors from 4.6.0 to 4.5.0. Reference the package directly from the project to select a different version.
       Microsoft.Maui.Core -> Microsoft.Extensions.Logging.Abstractions 10.0.0-alpha.1.25057.17 -> System.Memory 4.6.0 -> System.Numerics.Vectors (>= 4.6.0)
       Microsoft.Maui.Core -> System.Numerics.Vectors (>= 4.5.0)

* Update to iOS, tvOS, MacCatalyst 18.2

    10.0.100-alpha.1.25059.14\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(259,5):
    error NETSDK1140:
        18.0 is not a valid TargetPlatformVersion for MacCatalyst. Valid versions include:
        18.2
    10.0.100-alpha.1.25059.14\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(259,5):
    error NETSDK1140:
        18.0 is not a valid TargetPlatformVersion for iOS. Valid versions include:
        18.2

* Temporarily ignore `CS9270`:
  * dotnet/roslyn#76312
  * dotnet/roslyn#72133

    artifacts\obj\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.BindingSourceGen\Microsoft.Maui.Controls.BindingSourceGen.BindingSourceGenerator\D--src-maui-src-Controls-src-Core-BindableLayout-BindableLayout.cs-GeneratedBindingInterceptors-235-10.g.cs(43,4): error CS9270: 'InterceptsLocationAttribute(string, int, int)' is not supported. Move to 'InterceptableLocation'-based generation of these attributes instead. (dotnet/roslyn#72133)
    artifacts\obj\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.BindingSourceGen\Microsoft.Maui.Controls.BindingSourceGen.BindingSourceGenerator\D--src-maui-src-Controls-src-Core-ContentConverter.cs-GeneratedBindingInterceptors-72-13.g.cs(43,4): error CS9270: 'InterceptsLocationAttribute(string, int, int)' is not supported. Move to 'InterceptableLocation'-based generation of these attributes instead. (dotnet/roslyn#72133)
    artifacts\obj\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.BindingSourceGen\Microsoft.Maui.Controls.BindingSourceGen.BindingSourceGenerator\D--src-maui-src-Controls-src-Core-ContentConverter.cs-GeneratedBindingInterceptors-77-13.g.cs(43,4): error CS9270: 'InterceptsLocationAttribute(string, int, int)' is not supported. Move to 'InterceptableLocation'-based generation of these attributes instead. (dotnet/roslyn#72133)

* iOS API changes:

    error CS0618: 'CLLocation.AccurracyBestForNavigation' is obsolete: 'Use 'AccuracyBestForNavigation' instead.'

    Screenshot.ios.cs(57,69): error CS8602: Dereference of a possibly null reference.

    Platform\iOS\KeyboardAutoManagerScroll.cs(867,55): error CS8602: Dereference of a possibly null reference.

    src\Core\src\Platform\iOS\MauiSwipeView.cs(601,8): error CS8625: Cannot convert null literal to non-nullable reference type.
    src\Core\src\Platform\iOS\MauiSwipeView.cs(634,8): error CS8625: Cannot convert null literal to non-nullable reference type.

    ResignFirstResponderTouchGestureRecognizer.iOS.cs(70,12): error CS8600: Converting null literal or possible null value to non-nullable type.

    GesturePlatformManager.iOS.cs(282,21): error CS8602: Dereference of a possibly null reference.

Most notably:

* `UIView.Window` can be null.

* Typo in API fixed `AccurracyBestForNavigation` -> `AccuracyBestForNavigation`
rmarinho added a commit to dotnet/maui that referenced this pull request Jan 23, 2025
* Build with .NET 10 successfully

Changes required:

* Get appropriate `net9` branded workload manifests from dotnet/runtime

* Fix NuGet package downgrades:

    src\SingleProject\Resizetizer\src\Resizetizer.csproj : error NU1605:
      Warning As Error: Detected package downgrade: System.Runtime.CompilerServices.Unsafe from 6.1.0 to 6.0.0. Reference the package directly from the project to select a different version.
       Microsoft.Maui.Resizetizer -> Microsoft.Bcl.AsyncInterfaces 10.0.0-alpha.1.25058.4 -> System.Threading.Tasks.Extensions 4.6.0 -> System.Runtime.CompilerServices.Unsafe (>= 6.1.0)
       Microsoft.Maui.Resizetizer -> System.Runtime.CompilerServices.Unsafe (>= 6.0.0)
    src\Core\src\Core.csproj : error NU1605:
      Warning As Error: Detected package downgrade: System.Numerics.Vectors from 4.6.0 to 4.5.0. Reference the package directly from the project to select a different version.
       Microsoft.Maui.Core -> Microsoft.Extensions.Logging.Abstractions 10.0.0-alpha.1.25057.17 -> System.Memory 4.6.0 -> System.Numerics.Vectors (>= 4.6.0)
       Microsoft.Maui.Core -> System.Numerics.Vectors (>= 4.5.0)

* Update to iOS, tvOS, MacCatalyst 18.2

    10.0.100-alpha.1.25059.14\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(259,5):
    error NETSDK1140:
        18.0 is not a valid TargetPlatformVersion for MacCatalyst. Valid versions include:
        18.2
    10.0.100-alpha.1.25059.14\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(259,5):
    error NETSDK1140:
        18.0 is not a valid TargetPlatformVersion for iOS. Valid versions include:
        18.2

* Temporarily ignore `CS9270`:
  * dotnet/roslyn#76312
  * dotnet/roslyn#72133

    artifacts\obj\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.BindingSourceGen\Microsoft.Maui.Controls.BindingSourceGen.BindingSourceGenerator\D--src-maui-src-Controls-src-Core-BindableLayout-BindableLayout.cs-GeneratedBindingInterceptors-235-10.g.cs(43,4): error CS9270: 'InterceptsLocationAttribute(string, int, int)' is not supported. Move to 'InterceptableLocation'-based generation of these attributes instead. (dotnet/roslyn#72133)
    artifacts\obj\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.BindingSourceGen\Microsoft.Maui.Controls.BindingSourceGen.BindingSourceGenerator\D--src-maui-src-Controls-src-Core-ContentConverter.cs-GeneratedBindingInterceptors-72-13.g.cs(43,4): error CS9270: 'InterceptsLocationAttribute(string, int, int)' is not supported. Move to 'InterceptableLocation'-based generation of these attributes instead. (dotnet/roslyn#72133)
    artifacts\obj\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.BindingSourceGen\Microsoft.Maui.Controls.BindingSourceGen.BindingSourceGenerator\D--src-maui-src-Controls-src-Core-ContentConverter.cs-GeneratedBindingInterceptors-77-13.g.cs(43,4): error CS9270: 'InterceptsLocationAttribute(string, int, int)' is not supported. Move to 'InterceptableLocation'-based generation of these attributes instead. (dotnet/roslyn#72133)

* iOS API changes:

    error CS0618: 'CLLocation.AccurracyBestForNavigation' is obsolete: 'Use 'AccuracyBestForNavigation' instead.'

    Screenshot.ios.cs(57,69): error CS8602: Dereference of a possibly null reference.

    Platform\iOS\KeyboardAutoManagerScroll.cs(867,55): error CS8602: Dereference of a possibly null reference.

    src\Core\src\Platform\iOS\MauiSwipeView.cs(601,8): error CS8625: Cannot convert null literal to non-nullable reference type.
    src\Core\src\Platform\iOS\MauiSwipeView.cs(634,8): error CS8625: Cannot convert null literal to non-nullable reference type.

    ResignFirstResponderTouchGestureRecognizer.iOS.cs(70,12): error CS8600: Converting null literal or possible null value to non-nullable type.

    GesturePlatformManager.iOS.cs(282,21): error CS8602: Dereference of a possibly null reference.

Most notably:

* `UIView.Window` can be null.

* Typo in API fixed `AccurracyBestForNavigation` -> `AccuracyBestForNavigation`

* Disable tizen net10.0 temporarily

* TargetFramework=net10.0

* TargetFramework=net10.0

* Unused yaml: `TestTargetFrameworks`

* Update RunnerGenerator.cs

Workaround: dotnet/android@715a36a

* Put iOS simulator versions back to 18.0

* Update RunnerGenerator.cs

* Update xUnitSharedAttributes.cs

    D:\src\maui\src\TestUtils\src\TestShared\xUnitSharedAttributes.cs(75,89): error CS0117: 'DynamicallyAccessedMemberTypes' does not contain a definition for 'PublicParameterlessConstructors'
    D:\src\maui\src\TestUtils\src\TestShared\xUnitSharedAttributes.cs(83,63): error CS0117: 'DynamicallyAccessedMemberTypes' does not contain a definition for 'PublicParameterlessConstructors'

* Update Core.DeviceTests.Shared.csproj

* Update RunnerGenerator.cs

* Remove Tizen project

* Update ios.cake

* Update Core.DeviceTests.csproj

* Update android

* Update xcode

* Disable trimming warnings for now

* [iOS] Move to 18.2 simulators

* Ignore more warnings

* Ignore more warnings

* Update MainViewController.cs

* Ignore more warnings

* Disable more warnings

* Update xUnitSharedAttributes.cs

* Put trimmer warnings back

* EnableTrimAnalyzer=false for test projects across repo

* Make `EnableTizen()` a no-op

* Update Controls.DeviceTests.csproj

* Install `mobile-librarybuilder-net9`

Context: xamarin/xamarin-macios@cb13402

* Update WindowsTemplateTest.cs

* `mobile-librarybuilder-net9` on test lanes

* Update Essentials.Sample.csproj

* See if newer .NET SDK fixes maui-blazor

Context: dotnet/sdk@26fd6d1...60e9a46

I see a dotnet/aspnetcore bump in the diff.

* Essentials.Sample.csproj is a "sample project"

* > darc update-dependencies --id 252455

* Revert "Update RunnerGenerator.cs"

This reverts commit b9f32f6.

* Revert "Update RunnerGenerator.cs"

This reverts commit 9f6d36d.

* Revert "Update RunnerGenerator.cs"

This reverts commit 0f43f7d.

* darc update-dependencies --id 252613

* darc update-dependencies --coherency-only

* Update Version.Details.xml

* SuppressTrimAnalysisWarnings=true for test/sample projects

* Update Controls.DeviceTests.csproj

* Revert "Update Controls.DeviceTests.csproj"

This reverts commit 5ce0390.

* Disable iOS test for now

* Update provision.yml

* Update versions

* Update versions

* Revert "Update provision.yml"

This reverts commit 33a8ee5.

* Update LabelExtensions.cs

* Update ButtonTests.iOS.cs

* Try these new aspnet versions

* Update AssemblyInfoTests.cs

* $(TrimmerSingleWarn)=false

* Update MauiBlazorWebView.DeviceTests.csproj

* $(IsTestProject) -> $(MauiTestProject)

The old name conflicts with dotnet/arcade.

* Update Core.UnitTests.csproj

* Update MacTemplateTest.cs

* Update MacTemplateTest.cs

* Revert "Update MacTemplateTest.cs"

This reverts commit 7ed2b89.

* Revert "Update MacTemplateTest.cs"

This reverts commit 882c45e.

* EnableTrimAnalyzer=false for default projects

Context: xamarin/xamarin-macios#21351

This will avoid some of the fallout of the above PR.

* $(EnableTrimAnalyzer) take 2

* Update TestUtils.DeviceTests.Sample.csproj

* Update Microsoft.Maui.Controls.Common.targets

* Update Microsoft.Maui.Controls.Common.targets

* Update SimpleTemplateTest.cs

* Update Microsoft.Maui.Controls.Common.targets

* System.Text.Json.Serialization.Converters.EnumConverterFactory warning is gone

* Potential fix for M.E.DI failure

Fixes?

    [15:12:46.2484560] 2025-01-22 15:12:46.242709-0800 RunOniOSmauiRel1586373188[97228:54445246] Received unhandled Objective-C exception that was marshalled from a managed exception: A suitable constructor for type 'Microsoft.Maui.Hosting.FontsMauiAppBuilderExtensions+FontInitializer' could not be located. Ensure the type is concrete and services are registered for all parameters of a public constructor. (System.InvalidOperationException)
[15:12:46.2484780]    at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite(ResultCache, ServiceIdentifier, Type, CallSiteChain) + 0x4de
[15:12:46.2485210]    at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(ServiceDescriptor, ServiceIdentifier, CallSiteChain, Int32) + 0x1a5
[15:12:46.2485640]    at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateEnumerable(ServiceIdentifier, CallSiteChain) + 0x42f
[15:12:46.2485930]    at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateCallSite(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain) + 0xfb
[15:12:46.2486100]    at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.GetCallSite(ServiceIdentifier, CallSiteChain) + 0x92
[15:12:46.2486220]    at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(ServiceIdentifier serviceIdentifier) + 0x68
[15:12:46.2486330]    at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0xdc
[15:12:46.2486790]    at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0x39
[15:12:46.2486900]    at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x3d
[15:12:46.2487000]    at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider) + 0x2f
[15:12:46.2487100]    at Microsoft.Maui.MauiContextExtensions.InitializeAppServices(MauiApp) + 0x1b
[15:12:46.2487190]    at Microsoft.Maui.Hosting.MauiAppBuilder.Build() + 0x67
[15:12:46.2487420]    at Microsoft.Maui.MauiUIApplicationDelegate.WillFinishLaunching(UIApplication application, NSDictionary launchOptions) + 0x44
[15:12:46.2487530]    at RunOniOSmauiRel1586373188!<BaseAddress>+0xe200a

* Revert "Potential fix for M.E.DI failure"

This reverts commit 802e4c1.

* Update versions with CoherentParentDependency

* Update versions with coherent Microsoft.JSInterop

* Use strongly-typed Transient() for M.E.DI

* MauiVersionCurrent / MauiVersionPrevious

* Update FontsMauiAppBuilderExtensions.cs

* Revert "Update FontsMauiAppBuilderExtensions.cs"

This reverts commit 6b8e48a.

* Revert "Use strongly-typed Transient() for M.E.DI"

This reverts commit b0e0e45.

* Comment a few NativeAOT tests

---------

Co-authored-by: Rui Marinho <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Interceptors Needs API Review Needs to be reviewed by the API review council untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants