Make authentication linker friendly #24708
Conversation
- Preserve constructors whereever open generics or type arguments exist
using System;
namespace ConsoleApp1
{
class Program
{
static void Main(string[] args)
{
var bar = new Bar<MyThing>(new Lazy<MyThing>());
Console.WriteLine(bar.ToString());
}
}
public class MyThing
{
}
public class Bar<T> where T : new()
{
public Bar(Lazy<T> lazy)
{
}
}
}This seems like a linker bug. It's asking me to do this: public class Bar<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]T> where T : new()
{
public Bar(Lazy<T> lazy)
{
}
}But that should be inferred from the new constraint. Maybe the same issue as this dotnet/linker#1412? |
| @@ -50,6 +51,7 @@ public AuthenticationScheme(string name, string? displayName, Type handlerType) | |||
| /// <summary> | |||
| /// The <see cref="IAuthenticationHandler"/> type that handles this scheme. | |||
| /// </summary> | |||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] | |||
There was a problem hiding this comment.
Does the attribute do anything when the property isn't settable?
There was a problem hiding this comment.
It's tracked via the constructor assignment when the linker does flow control.
There was a problem hiding this comment.
The annotation works both ways:
- When assigned to, linker makes sure the input satisfies the requirements - so either it needs to have a similar annotation or it's a statically recognized type for which linker then includes public .ctors.
- When read from - if the value of the property is passed to another method and that methods also has the attribute, linker will compare the attributes and if they're compatible it won't issue a warning since it knows that types stored in the property fulfill the requirements
| @@ -16,6 +17,8 @@ public abstract class AuthenticationHandler<TOptions> : IAuthenticationHandler w | |||
| private Task<AuthenticateResult>? _authenticateTask; | |||
|
|
|||
| public AuthenticationScheme Scheme { get; private set; } = default!; | |||
|
|
|||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] | |||
There was a problem hiding this comment.
Does the attribute do anything when the property isn't (publically) settable?
Curious why the attribute is here and not on the TOptions generic arg on the class.
There was a problem hiding this comment.
This is a good question. It probably doesn't need to be on here but the linker complains about the backing field for the property. It's possible this attribute should automagically flow to the compiler generated backing field to avoid that.
The other thing is that we have a new constraint so this shouldn't be needed at all... Likely a linker issue.
There was a problem hiding this comment.
This is weird - the attribute on this property doesn't make any sense - it should only be on properties of type System.Type or System.String. Linker currently doesn't warn about this (I think it should - dotnet/linker#1420) - but in reality it will simply ignore the attribute in this case.
I don't know what the original warning was about the backing field you mention. If it has to do with TOptions then the attribute should be on the generic parameter. (with the linker fix it might not be needed due to the new constraint anyway).
There was a problem hiding this comment.
I'll remove these on the properties
There was a problem hiding this comment.
I'll remove these on the properties
It's just this one place that is an issue, right?
with the linker fix it might not be needed due to the new constraint anyway
That fix has been checked in (dotnet/linker#1414). @davidfowl - can you try the latest mono/linker and see if the warning just goes away here?
Yeah, looks like the same issue. We have a fix, but it has been waiting for Vitek to come back from vacation. |
| @@ -50,6 +51,7 @@ public AuthenticationScheme(string name, string? displayName, Type handlerType) | |||
| /// <summary> | |||
| /// The <see cref="IAuthenticationHandler"/> type that handles this scheme. | |||
| /// </summary> | |||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] | |||
There was a problem hiding this comment.
The annotation works both ways:
- When assigned to, linker makes sure the input satisfies the requirements - so either it needs to have a similar annotation or it's a statically recognized type for which linker then includes public .ctors.
- When read from - if the value of the property is passed to another method and that methods also has the attribute, linker will compare the attributes and if they're compatible it won't issue a warning since it knows that types stored in the property fulfill the requirements
| @@ -16,6 +17,8 @@ public abstract class AuthenticationHandler<TOptions> : IAuthenticationHandler w | |||
| private Task<AuthenticateResult>? _authenticateTask; | |||
|
|
|||
| public AuthenticationScheme Scheme { get; private set; } = default!; | |||
|
|
|||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] | |||
There was a problem hiding this comment.
This is weird - the attribute on this property doesn't make any sense - it should only be on properties of type System.Type or System.String. Linker currently doesn't warn about this (I think it should - dotnet/linker#1420) - but in reality it will simply ignore the attribute in this case.
I don't know what the original warning was about the backing field you mention. If it has to do with TOptions then the attribute should be on the generic parameter. (with the linker fix it might not be needed due to the new constraint anyway).
Why can't these be addressed? |
|
Because the new constraint should take care of them. |
Remaining warnings:
dotnet/linker#1416 - - Compiler generated generic parameters
dotnet/linker#1403 - Compiler generated backing fields
Mostly constructors and compiler generated code.