Add infrastructure to test WASM linking#29900
Conversation
ghost
added
the
pranavkm
force-pushed
the
prkrishn/linker-tests
branch
10 times, most recently
from
0c6c84a to
8246fc8
Compare
| @@ -0,0 +1,11 @@ | |||
| <linker> | |||
| <assembly fullname="Microsoft.AspNetCore.Components"> | |||
| <type fullname="Microsoft.AspNetCore.Components.ComponentBase" required="false" /> | |||
There was a problem hiding this comment.
I'm not sure I follow this. Is this "type level" granularity trimming?
| ValueSupplier = valueSupplier; | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072:RequiresUnreferencedCode", Justification = "Requires a gesture that ensures components are always preserved. https://github.com/mono/linker/issues/1806")] |
There was a problem hiding this comment.
I wonder if it would be better to baseline these in an .xml file, like we do in dotnet/runtime. That way we know to come back to them when the linker feature is done.
The benefit is that the application will still get a warning, because what this is doing is unsafe.
| } | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2080", Justification = "This is not a trimmer safe pattern.")] |
There was a problem hiding this comment.
This shouldn't be suppressed. But instead have RequiresUnreferencedCode attribute.
| } | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026", Justification = "This pattern is not linker friendly.")] |
There was a problem hiding this comment.
This shouldn't be suppressed. But instead have RequiresUnreferencedCode attribute.
| @@ -0,0 +1,22 @@ | |||
| <linker> | |||
| <assembly fullname="Microsoft.AspNetCore.Components.Web"> | |||
| <type fullname="Microsoft.AspNetCore.Components.Forms.EditForm" required="false" preserve="all" /> | |||
There was a problem hiding this comment.
Do we really need the whole Type of all these classes?
There was a problem hiding this comment.
These are all components, and preserving these entirely is the safest course of action at this point.
There was a problem hiding this comment.
The ones for M.A.Components don't have preserve="all". Is the difference just that it's more work (and maintenance cost) to reason about the exact preservability rules for all these extra components in .Web?
There was a problem hiding this comment.
I'll make the two consistent. preserve="all" is the default:
<!-- No "preserve" attribute and no members specified means preserve all members -->
<type fullname="Assembly.B" />
https://github.com/mono/linker/blob/main/docs/data-formats.md#preserve-a-type
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)] Type type, | ||
| BindingFlags bindingFlags) |
There was a problem hiding this comment.
I'm not sure this is enough. The current behavior in linker is that it follow reflection exactly. So this annotation is equivalent to calling type.GetProperties(bindingflags). It does NOT translate to calling the same on the base type. Specifically, private properties on base type are not preserved, but the code here will try to access them.
The reason there's no warning is that linker has a known issue with tracking backward branches, and so the while loop is not correctly analyzed (it sees the .BaseType "too late" and doesn't realize that it will call .GetProperties() on it).
I think the only "safe" annotation is All in this case (which does preserve everything, including private properties on all base types).
Side note: Why do we allow private reflection here... we're telling everybody not to do it, and here we allow it.
There was a problem hiding this comment.
Why do we allow private reflection here... we're telling everybody not to do it, and here we allow it.
Our programming model allows for property injection (using DI) for components. We recommend / encourage these to be private since these aren't meant to be internal implementation detail of the component and not visible outside of it.
| new ConcurrentDictionary<Key, LegacyRouteTable>(); | ||
| public static readonly IComparer<LegacyRouteEntry> RoutePrecedence = Comparer<LegacyRouteEntry>.Create(RouteComparison); | ||
|
|
||
| [RequiresUnreferencedCode("This API calls Assembly.ExportedTypes. Types and members of the loaded assembly might be removed during trimming.")] |
There was a problem hiding this comment.
This will be a public facing warning message. So explaining internal implementation details doesn't help. Ideally it should tell the user what to do about this - call a different api, or something similar.
There was a problem hiding this comment.
This particular API is internal. The bigger issue is that user code isn't really involved in this code path at all so even if we do annotate this correctly, it terminates at some point in the framework code that users do not have any agency in.
The higher order patterns are somewhat similar across ASP.NET Core stacks. Maybe we could talk about it as part of dotnet/linker#1806 and see how we would want the experience to be? (Btw, user experiences are for 7.0 for now we're only looking to trim the framework).
is somewhat similar
|
|
||
| namespace Microsoft.AspNetCore.Components.WebAssembly.Hosting | ||
| { | ||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026", Justification = "This type loads resx files. We don't expect it's dependencies to be trimmed in the ordinary case.")] |
There was a problem hiding this comment.
I admit I don't fully understand what this does, but the justification sounds like "we hope this will work". That's typically not good enough. Maybe we need a different solution here?
There was a problem hiding this comment.
Blazor WebAssembly is responsible for it's own assembly loading. With resx files, it waits until after the app has started Program.MainAsync has executed to load satellite assemblies based on current culture. These resource assemblies are part of the initial set of assemblies that are trimmed, so we're not loading any assemblies that hadn't participated during linking.
| /// <summary> | ||
| /// Flags for a member that is JSON (de)serialized. | ||
| /// </summary> | ||
| public const DynamicallyAccessedMemberTypes JsonSerialized = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicProperties; |
There was a problem hiding this comment.
These don't apply recursively. So if the type in question is something like:
class Email { public Attachment Attachment {get; set;} }
class Attachment { public Name {get; set;} }Annotating Email with this, will not preserve Attachment.Name which the serializer will try to access.
Currently there's no annotation which can be added for serialization purposes. For System.Text.Json we basically hope that source generators will solve this.
There was a problem hiding this comment.
That makes sense. These annotations mirror the JsonSerializer's current annotations. Once we have a AOT support there, we'll make sure these APIS are correctly set up to work with the source generator.
pranavkm
force-pushed
the
prkrishn/linker-tests
branch
from
88d9607 to
68b15e7
Compare
| { | ||
| var instance = Activator.CreateInstance(componentType); | ||
| if (!(instance is IComponent component)) | ||
| if (!typeof(IComponent).IsAssignableFrom(componentType)) |
There was a problem hiding this comment.
Is the change here in order to improve things for linkability, or just a stylistic preference?
It's probably not a big deal but the implementation before avoided repeated type compatibility checks.
There was a problem hiding this comment.
I think it might allow the linker to better statically analyze the code particularly if it's unable to piece together the control flow that this is a component type.
Do you think it's worthwhile removing the type compatibility check (or putting it behind a Debug.Assert if it helps the linker analysis) and letting the cast throw? The exception message you get for the invalid cast isn't significantly worse than what you currently get.
There was a problem hiding this comment.
It could help analysis, but illink doesn't work that way right now.
The purpose is to ensure that if someone passes a random type here, we don't get a MissingMemberException due to trimming before we would do the type check.
The behavior of the app before/after trimming shouldn't change and that includes error cases like this - they make troubleshooting harder.
There was a problem hiding this comment.
Do you think it's worthwhile removing the type compatibility check (or putting it behind a Debug.Assert if it helps the linker analysis) and letting the cast throw? The exception message you get for the invalid cast isn't significantly worse than what you currently get.
I'm certainly fine with letting the error come from the cast if that gives us some linkability benefit. I agree the message on a failed cast exception is clear enough.
| <assembly fullname="Microsoft.AspNetCore.Components, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60"> | ||
| <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> | ||
| <argument>ILLink</argument> | ||
| <argument>IL2026</argument> |
There was a problem hiding this comment.
It's not obvious from reading this file what warnings are being suppressed. What are IL2026 and the others? Could you add comments stating what is the purpose of the suppressions?
There was a problem hiding this comment.
The files were generated by the linker so it'd be a bit of an upkeep to document it. A lot of these are complaining about the various reflection patterns with components and I'm hoping we get a linker feature that removes this need.
| } | ||
|
|
||
| var componentTypes = key.Assemblies.SelectMany(a => a.ExportedTypes.Where(t => typeof(IComponent).IsAssignableFrom(t))); | ||
| var componentTypes = RouteTableFactory.GetRouteableComponents(key.Assemblies); |
There was a problem hiding this comment.
Is it possible to leave this file unchanged? The whole idea of the Legacy* variants of the routing logic is that they matched an exact point in time that we shipped before, and we keep them decoupled from the newer code so we don't have to reason about how any changes in the new code might impact behaviors in the legacy code.
Eventually (.NET 7?) we'll just remove the whole of the legacy version.
| { | ||
| // Arrange & Act | ||
| var routes = RouteTableFactory.Create(new[] { typeof(MyComponent), typeof(MyInheritedComponent), }); | ||
| var routes = RouteTableFactory.Create(new List<Type> { typeof(MyComponent), typeof(MyInheritedComponent), }); |
There was a problem hiding this comment.
Curious about why these changes
There was a problem hiding this comment.
I should undo these. The linker does not flow suppressions to compiler generated state machines. The previous code relied on Linq + yield return which resulted in warnings in the compiler generated code that I did not know how to suppress.
|
|
||
| <ItemGroup> | ||
| <Folder Include="Properties\" /> | ||
| </ItemGroup> |
|
|
||
| public IDisposable DotNetReference { get; } | ||
|
|
||
| [DynamicDependency(nameof(NotifyChange))] |
There was a problem hiding this comment.
This is subtle. Having to reason about all the possible code paths that could result in a given [JSInvokable] method to be used. I don't have a better suggestion than what you've done here, but it something we'll have to think hard about in all code reviews that could impact the possible code paths that lead to a JS invokable member being used.
There was a problem hiding this comment.
You're right. I think JSInvokable is one of the few feature that currently has no way to signal a linker warning and you would only find out at runtime. Makes @javiercn's issue to test these as part of our builds doubly important.
|
This looks good to me, @pranavkm! Do you think there's any value in trying to do any specific E2E testing around post-linked applications? I know we have that for the default template currently. Maybe that's enough. |
|
@SteveSandersonMS there's a separate issue for it here |
pranavkm
force-pushed
the
prkrishn/linker-tests
branch
from
0fc8560 to
d731057
Compare
| } | ||
|
|
||
| var componentTypes = key.Assemblies.SelectMany(a => a.ExportedTypes.Where(t => typeof(IComponent).IsAssignableFrom(t))); | ||
| var componentTypes = GetRouteableComponents(key.Assemblies); |
| /// <summary> | ||
| /// Flags for a component | ||
| /// </summary> | ||
| public const DynamicallyAccessedMemberTypes Component = DynamicallyAccessedMemberTypes.All; |
There was a problem hiding this comment.
This looks a bit drastic. Do you really need nested types or private fields?
There was a problem hiding this comment.
Blazor relies on being able to reflect on private properties on Components. @vitek-karas suggested using All as the most reliable way to preserve these.
|
Why are Microsoft.JSInterop assemblies not included? |
|
They are imported transitively, but I'll go ahead and add them explicitly |
pranavkm
force-pushed
the
prkrishn/linker-tests
branch
from
d731057 to
63c00d4
Compare
|
Hello @pranavkm! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
pranavkm
force-pushed
the
prkrishn/linker-tests
branch
from
29b2476 to
4bb4696
Compare
pranavkm
force-pushed
the
prkrishn/linker-tests
branch
from
4bb4696 to
a800ff1
Compare
@pranavkm - once dotnet/linker#1839 is flown throughout the system, you can just add |
|
Hi @eerhardt. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
Yup, @marek-safar noted this as part of #30286 |
9 participants
The intent is to have a project that links Blazor assemblies and provide trimming analysis. This causes the build to fail if there's an API that isn't trimmer-safe or has been annotated or suppressed. Based on the pattern that the runtime uses - https://github.com/dotnet/runtime/blob/master/src/libraries/illink-sharedframework.targets
Missing the Auth assemblies, but I was able to verify that the others work fine. Note that we have no E2E integration for this as yet since the list of assemblies to type-granular trim is in the SDK which hasn't been modified outside of my dev box.