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

required and GenerateReferenceAssemblySource #71559

Closed
Tracked by #93172
ManickaP opened this issue Jul 1, 2022 · 14 comments · Fixed by dotnet/arcade#10021
Closed
Tracked by #93172

required and GenerateReferenceAssemblySource #71559

ManickaP opened this issue Jul 1, 2022 · 14 comments · Fixed by dotnet/arcade#10021

Comments

@ManickaP
Copy link
Member

ManickaP commented Jul 1, 2022

When using the new required keyword, GenerateReferenceAssemblySource generates code like this:

[System.Runtime.CompilerServices.RequiredMemberAttribute]
public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    [System.ObsoleteAttribute("Constructors of types with required members are not supported in this version of your compiler.", true)]
    [System.Runtime.CompilerServices.CompilerFeatureRequiredAttribute("RequiredMembers")]
    public QuicClientConnectionOptions() { }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { get { throw null; } set { } }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw null; } set { } }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.EndPoint RemoteEndPoint { get { throw null; } set { } }
}

And then compiler complains that required keyword should be used instead:

System.Net.Quic.cs(10,6): error CS9033: Do not use 'System.Runtime.CompilerServices.RequiredMemberAttribute'. Use the 'required' keyword on required fields and properties instead.

I don't know which way is the correct way. @ViktorHofer @333fred

Follow up issue is that if I manually remove the attributes and add required keyword, the GeneratePlatformNotSupportedAssemblyMessage generator generates another code that cannot be compiled. See the missing get and set:

public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    public QuicClientConnectionOptions() { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public required System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } set { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } }
    public required System.Net.EndPoint RemoteEndPoint { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
}

Does anyone know where the GeneratePlatformNotSupportedAssemblyMessage lives and whom to contact?

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 1, 2022
@ghost
Copy link

ghost commented Jul 1, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

When using the new required keyword, GenerateReferenceAssemblySource generates code like this:

[System.Runtime.CompilerServices.RequiredMemberAttribute]
public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    [System.ObsoleteAttribute("Constructors of types with required members are not supported in this version of your compiler.", true)]
    [System.Runtime.CompilerServices.CompilerFeatureRequiredAttribute("RequiredMembers")]
    public QuicClientConnectionOptions() { }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { get { throw null; } set { } }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw null; } set { } }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.EndPoint RemoteEndPoint { get { throw null; } set { } }
}

And then compiler complains that required keyword should be used instead:

System.Net.Quic.cs(10,6): error CS9033: Do not use 'System.Runtime.CompilerServices.RequiredMemberAttribute'. Use the 'required' keyword on required fields and properties instead.

I don't know which way is the correct way. @ViktorHofer @333fred

Follow up issue is that if I manually remove the attributes and add required keyword, the GeneratePlatformNotSupportedAssemblyMessage generator generates another code that cannot be compiled. See the missing get and set:

public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    public QuicClientConnectionOptions() { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public required System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } set { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } }
    public required System.Net.EndPoint RemoteEndPoint { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
}

Does anyone know where the GeneratePlatformNotSupportedAssemblyMessage lives and whom to contact?

Author: ManickaP
Assignees: -
Labels:

untriaged, area-System.Net.Quic

Milestone: -

@ghost
Copy link

ghost commented Jul 1, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

When using the new required keyword, GenerateReferenceAssemblySource generates code like this:

[System.Runtime.CompilerServices.RequiredMemberAttribute]
public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    [System.ObsoleteAttribute("Constructors of types with required members are not supported in this version of your compiler.", true)]
    [System.Runtime.CompilerServices.CompilerFeatureRequiredAttribute("RequiredMembers")]
    public QuicClientConnectionOptions() { }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { get { throw null; } set { } }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw null; } set { } }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.EndPoint RemoteEndPoint { get { throw null; } set { } }
}

And then compiler complains that required keyword should be used instead:

System.Net.Quic.cs(10,6): error CS9033: Do not use 'System.Runtime.CompilerServices.RequiredMemberAttribute'. Use the 'required' keyword on required fields and properties instead.

I don't know which way is the correct way. @ViktorHofer @333fred

Follow up issue is that if I manually remove the attributes and add required keyword, the GeneratePlatformNotSupportedAssemblyMessage generator generates another code that cannot be compiled. See the missing get and set:

public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    public QuicClientConnectionOptions() { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public required System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } set { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } }
    public required System.Net.EndPoint RemoteEndPoint { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
}

Does anyone know where the GeneratePlatformNotSupportedAssemblyMessage lives and whom to contact?

Author: ManickaP
Assignees: -
Labels:

area-Infrastructure, untriaged

Milestone: -

@333fred
Copy link
Member

333fred commented Jul 1, 2022

It looks like the runtime's generator needs to be updated.

@ViktorHofer
Copy link
Member

Does anyone know where the GeneratePlatformNotSupportedAssemblyMessage lives and whom to contact?

Here is the task: https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.GenFacades/NotSupportedAssemblyGenerator.cs and the msbuild file with the target that invokes the task is here: https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.GenFacades/build/Microsoft.DotNet.GenFacadesNotSupported.targets.

cc @ericstj @tannergooding

@ViktorHofer
Copy link
Member

Maybe it's enough to bump the Microsoft.CodeAnalysis.CSharp version in arcade by updating the MsbuildTaskMicrosoftCodeAnalysisCSharpVersion and MicrosoftCodeAnalysisCSharpVersion properties?

@ghost
Copy link

ghost commented Jul 11, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

When using the new required keyword, GenerateReferenceAssemblySource generates code like this:

[System.Runtime.CompilerServices.RequiredMemberAttribute]
public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    [System.ObsoleteAttribute("Constructors of types with required members are not supported in this version of your compiler.", true)]
    [System.Runtime.CompilerServices.CompilerFeatureRequiredAttribute("RequiredMembers")]
    public QuicClientConnectionOptions() { }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { get { throw null; } set { } }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw null; } set { } }
    [System.Runtime.CompilerServices.RequiredMemberAttribute]
    public System.Net.EndPoint RemoteEndPoint { get { throw null; } set { } }
}

And then compiler complains that required keyword should be used instead:

System.Net.Quic.cs(10,6): error CS9033: Do not use 'System.Runtime.CompilerServices.RequiredMemberAttribute'. Use the 'required' keyword on required fields and properties instead.

I don't know which way is the correct way. @ViktorHofer @333fred

Follow up issue is that if I manually remove the attributes and add required keyword, the GeneratePlatformNotSupportedAssemblyMessage generator generates another code that cannot be compiled. See the missing get and set:

public sealed partial class QuicClientConnectionOptions : System.Net.Quic.QuicConnectionOptions
{
    public QuicClientConnectionOptions() { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public required System.Net.Security.SslClientAuthenticationOptions ClientAuthenticationOptions { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
    public System.Net.IPEndPoint? LocalEndPoint { get { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } set { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  } }
    public required System.Net.EndPoint RemoteEndPoint { throw new System.PlatformNotSupportedException(System.SR.SystemNetQuic_PlatformNotSupported);  }
}

Does anyone know where the GeneratePlatformNotSupportedAssemblyMessage lives and whom to contact?

Author: ManickaP
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@ViktorHofer
Copy link
Member

ViktorHofer commented Jul 12, 2022

Follow up issue is that if I manually remove the attributes and add required keyword, the GeneratePlatformNotSupportedAssemblyMessage generator generates another code that cannot be compiled. See the missing get and set:

I asked @ericstj to take a look as I'm unsure if bumping the CodeAnalysis version is sufficient to make this work.

@ViktorHofer ViktorHofer added this to the 7.0.0 milestone Jul 13, 2022
@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Jul 13, 2022
@ericstj
Copy link
Member

ericstj commented Jul 14, 2022

When new language features come along the arcade tools GenAPI and GenFacades need to be updated to understand them.

The issue with GenerateReferenceAssemblySource is because GenAPI doesn't know about that new language feature and instead just emitting the attribute based on the metadata. I'm surprised the compiler treats the use of those attributes as an error instead of a suppressable warning. If we wanted GenerateReferenceAssemblySource to understand that we'd need updates to GenAPI.

I asked @ericstj to take a look as I'm unsure if bumping the CodeAnalysis version is sufficient to make this work.

There's a very good chance that would work, though someone would need to try and see. The rewriter for the not-supported assemblies is just parsing the source and replacing the member bodies: https://github.com/dotnet/arcade/blob/3895dfc219f7cea2c028164db691389d0b1a73a8/src/Microsoft.DotNet.GenFacades/NotSupportedAssemblyGenerator.cs#L125-L130
Probably it's failing to understand the new keyword. @ManickaP would you want to try to make the change to GenFacades and see if it unblocks you?

@333fred
Copy link
Member

333fred commented Jul 14, 2022

I'm surprised the compiler treats the use of those attributes as an error instead of a suppressable warning.

I don't know of an attribute like this that we allow, even with a warning. For example, extension methods are represented by an attribute, but it is illegal to use that attribute in C# itself.

@ericstj
Copy link
Member

ericstj commented Jul 14, 2022

I don't know of an attribute like this that we allow, even with a warning. For example, extension methods are represented by an attribute, but it is illegal to use that attribute in C# itself.

I noticed some other ones like AsyncStateMachineAttribute and AsyncIteratorStateMachineAttribute. I'm not saying I have a huge opinion here, just saying I was surprised. I think of errors as things that would halt the compilation and this didn't feel like it was that severe.

@ericstj
Copy link
Member

ericstj commented Jul 14, 2022

Another thought here around GenFacades -- it's carrying its own copy of Roslyn. We should probably have it do the same thing that APICompat does and load the version that's already in use by the project.

@ericstj
Copy link
Member

ericstj commented Jul 14, 2022

It looks like required is a LangVersion=preview feature, but GenFacades just uses roslyn without specifying a langversion, so that's another issue here.

@ericstj
Copy link
Member

ericstj commented Jul 15, 2022

I went ahead and fixed this: dotnet/arcade#10021

@ericstj
Copy link
Member

ericstj commented Jul 15, 2022

This should be fixed in arcade now. You'll be able to use that change once it merges to dotnet/runtime - likely next week. Check on Monday if it's made it by looking for a build of GenAPI / GenFacades that's newer than 7.0.0-beta.22365.1. If it hasn't landed by then we can manually pick up the new packages to get this functionality before Darc flows the build.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants