dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.37k stars 4.75k forks source link

Dataflow crash while generating a warning about an assembly-level attribute #79833

Closed jaedan closed 1 year ago

jaedan commented 1 year ago

Description

To be clear up front, I know Blazor Hybrid isn't supported with NativeAOT right now. This bug is a result of investigating exactly what would be needed to make that work. Right now, the compiler crashes with this stack track during publishing:

  System.NotImplementedException: Unexpected type system entity
     at ILCompiler.Dataflow.EcmaExtensions.GetOwningType(TypeSystemEntity) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/EcmaExtensions.cs:line 114
     at ILCompiler.Dataflow.CompilerGeneratedState.TryGetOwningMethodForCompilerGeneratedMember(TypeSystemEntity, MethodDesc& ) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs:line 619
     at ILCompiler.Logger.ShouldSuppressAnalysisWarningsForRequires(TypeSystemEntity, String) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Logger.cs:line 308
     at ILCompiler.Dataflow.ReflectionMarker.CheckAndWarnOnReflectionAccess(MessageOrigin& , TypeSystemEntity, Origin) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs:line 197
     at ILCompiler.Dataflow.ReflectionMarker.MarkTypeForDynamicallyAccessedMembers(MessageOrigin& , TypeDesc, DynamicallyAccessedMemberTypes, Origin, Boolean ) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs:line 51
     at ILLink.Shared.TrimAnalysis.RequireDynamicallyAccessedMembersAction.Invoke(ValueSet`1& , ValueWithDynamicallyAccessedMembers) in /_/src/coreclr/tools/aot/ILLink.Shared/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs:line 41
     at ILCompiler.Dataflow.AttributeDataFlow.RequireDynamicallyAccessedMembers(DiagnosticContext& , ValueSet`1& , ValueWithDynamicallyAccessedMembers, Origin, DependencyList&) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs:line 128

The code in EcmaExtensions.cs at line 114 is:

        public static TypeDesc GetOwningType(this TypeSystemEntity entity)
        {
            return entity switch
            {
                MethodDesc method => method.OwningType,
                FieldDesc field => field.OwningType,
                MetadataType type => type.ContainingType,
                PropertyPseudoDesc property => property.OwningType,
                EventPseudoDesc @event => @event.OwningType,
                _ => throw new NotImplementedException("Unexpected type system entity")
            };
        }

But I don't know what TypeSystemEntity subclass was actually passed in.

Reproduction Steps

  1. Clone this minimal example of WinForms + BlazorWebView + NativeAOT: https://github.com/jaedan/WinFormsBlazorHybridAOT/
  2. dotnet publish -r win-x64 --self-contained=true

The compiler will crash.

Expected behavior

The compiler shouldn't crash. It may still fail to compile as this scenario isn't supported.

Actual behavior

Compiler crashes with above stack trace.

Regression?

Not a regression

Known Workarounds

Don't build NativeAOT?

Configuration

.net 7 latest release, windows 11, x64. It's probably not specific to this configuration.

Other information

src/coreclr/tools/Common/Datalow/EcmaExtensions.cs is missing a type in GetOwningType's switch statement??

ghost commented 1 year ago

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

Issue Details
### Description To be clear up front, I know Blazor Hybrid isn't supported with NativeAOT right now. This bug is a result of investigating exactly what would be needed to make that work. Right now, the compiler crashes with this stack track during publishing: ``` System.NotImplementedException: Unexpected type system entity at ILCompiler.Dataflow.EcmaExtensions.GetOwningType(TypeSystemEntity) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/EcmaExtensions.cs:line 114 at ILCompiler.Dataflow.CompilerGeneratedState.TryGetOwningMethodForCompilerGeneratedMember(TypeSystemEntity, MethodDesc& ) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs:line 619 at ILCompiler.Logger.ShouldSuppressAnalysisWarningsForRequires(TypeSystemEntity, String) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Logger.cs:line 308 at ILCompiler.Dataflow.ReflectionMarker.CheckAndWarnOnReflectionAccess(MessageOrigin& , TypeSystemEntity, Origin) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs:line 197 at ILCompiler.Dataflow.ReflectionMarker.MarkTypeForDynamicallyAccessedMembers(MessageOrigin& , TypeDesc, DynamicallyAccessedMemberTypes, Origin, Boolean ) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs:line 51 at ILLink.Shared.TrimAnalysis.RequireDynamicallyAccessedMembersAction.Invoke(ValueSet`1& , ValueWithDynamicallyAccessedMembers) in /_/src/coreclr/tools/aot/ILLink.Shared/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs:line 41 at ILCompiler.Dataflow.AttributeDataFlow.RequireDynamicallyAccessedMembers(DiagnosticContext& , ValueSet`1& , ValueWithDynamicallyAccessedMembers, Origin, DependencyList&) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs:line 128 ``` The code in EcmaExtensions.cs at line 114 is: ``` public static TypeDesc GetOwningType(this TypeSystemEntity entity) { return entity switch { MethodDesc method => method.OwningType, FieldDesc field => field.OwningType, MetadataType type => type.ContainingType, PropertyPseudoDesc property => property.OwningType, EventPseudoDesc @event => @event.OwningType, _ => throw new NotImplementedException("Unexpected type system entity") }; } ``` But I don't know what TypeSystemEntity subclass was actually passed in. ### Reproduction Steps 1. Clone this minimal example of WinForms + BlazorWebView + NativeAOT: https://github.com/jaedan/WinFormsBlazorHybridAOT/ 2. dotnet publish -r win-x64 --self-contained=true The compiler will crash. ### Expected behavior The compiler shouldn't crash. It may still fail to compile as this scenario isn't supported. ### Actual behavior Compiler crashes with above stack trace. ### Regression? Not a regression ### Known Workarounds Don't build NativeAOT? ### Configuration .net 7 latest release, windows 11, x64. It's probably not specific to this configuration. ### Other information src/coreclr/tools/Common/Datalow/EcmaExtensions.cs is missing a type in GetOwningType's switch statement??
Author: jaedan
Assignees: -
Labels: `area-NativeAOT-coreclr`
Milestone: -
MichalStrehovsky commented 1 year ago

The type system entity in question is a module.

This is caused by this line in Maui:

https://github.com/dotnet/maui/blob/0f41a72bacf563520332334d48db9e0b47d94c7c/src/BlazorWebView/src/SharedSource/StaticContentHotReloadManager.cs#L12

The custom attribute declares that the members of this type are going to be a target of reflection, but some of the members are problematic and generate a warning. The compiler crashes while we're trying to generate the warning.

Fortunately, there's an easy workaround: add <MetadataUpdaterSupport>false</MetadataUpdaterSupport> to a PropertyGroup in the project. This specifically gets rid of the problematic attribute.

kant2002 commented 1 year ago

@MichalStrehovsky we have OP seems to be ready to build runtme. Let's see what he answers, but maybe if there are easy fix, you can guide @jaedan?

MichalStrehovsky commented 1 year ago

@MichalStrehovsky we have OP seems to be ready to build runtme. Let's see what he answers, but maybe if there are easy fix, you can guide @jaedan?

This is code shared with IL Linker (and it looks like it got updated in the dotnet/linker repo so it now looks different) - I don't immediately know what the fix is. To figure out the fix, I would run a similar scenario with IL Linker (ideally synced to an older version of the dotnet/linker repo where TryGetOwningMethodForCompilerGeneratedMember still looks the same) and find where we diverge. We'll also need a regression test.

Cc @dotnet/ilc-contrib

MichalStrehovsky commented 1 year ago

We might also want to fix Maui - adding <EnableTrimAnalyzer>true</EnableTrimAnalyzer> would probably flag the custom attribute at library build time. We probably need to put the nested delegate this is complaining about somewhere where the attribute doesn't apply.

jaedan commented 1 year ago

I tested <MetadataUpdaterSupport>false</MetadataUpdaterSupport> and indeed it fixes the crash and now I get an executable out. The executable doesn't run, but that is because the ComWrappers are missing the CoreWebView2 types.

I am all set up to build the runtime and nativeaot toolchain, plus the comwrappers. I can test whatever you all suggest or need.

My end goal is to build a simple demo of an application that spawns a single BlazorWebView control and implements a UI in Blazor (Hybrid), that compiles using NativeAOT. Basically, a faster, lighter-weight replacement for Electron using Blazor. I'm using WinForms to spawn the BlazorWebView only because it's the only UI framework that already supports NativeAOT (excepting WebView2 stuff it seems). I could switch it away from WinForms at some point to something that can spawn a BlazorWebView in a more cross-platform friendly way (maybe Photino? maybe something more purpose-built?) but I'll cross that bridge when I get to it. The NativeAOT part is the "new" part of this effort, as the rest has largely been done before.

vitek-karas commented 1 year ago

We might also want to fix Maui

Enabling the analyzer on MAUI would be great, but it would probably produce 100s of warnings elsewhere. The correct fix for the metadata updater is to create a subclass which purpose is to ONLY be the updater and nothing else (so no other members). We've done this in other places which ran into a similar problem. There's another issue that the metadata updater attribute requires too much about the type - https://github.com/dotnet/runtime/issues/66069 which would also help here.

vitek-karas commented 1 year ago

This has been fixed with https://github.com/dotnet/runtime/pull/80694. Since this is a scenario which is not really supported (or expected to happen often) on 7.0, I don't plan to port the fix to 7.0.