[Draft] Remove reference projects from nearly all out-of-band projects #65775
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsContributes to #58163 This is a proof of concept that demonstrates that most out-of-band reference The remaining out-of-band projects are more challenging as they depend on the Microsoft.Bcl.AsyncInterfaces (Facade) The moved ".ref.cs" file is auto-generated by ApiCompat which is now used @eristj @joperezr @eerhardt @carlossanlop
|
ViktorHofer
changed the title
ViktorHofer
force-pushed
the
AvoidRefProjectsInOOBs
branch
from
181b49d to
0823b07
Compare
ViktorHofer
added
area-Infrastructure-libraries
and removed
area-Meta
new-api-needs-documentation
labels
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsContributes to #58163 This is a proof of concept that demonstrates that most out-of-band reference The remaining out-of-band projects are more challenging as they depend on the Microsoft.Bcl.AsyncInterfaces (Facade) The moved ".ref.cs" files are auto-generated by GenAPI which is now used Observations:
@eristj @joperezr @eerhardt @carlossanlop
|
ViktorHofer
changed the title
| @@ -60,7 +60,7 @@ public static partial class CacheEntryExtensions | |||
| public static partial class CacheExtensions | |||
There was a problem hiding this comment.
Do these files need to go into the src directory? That breaks the DefaultCompileItems stuff that we need to work around. And it is kind of odd to have .cs files in the src directory that aren't compiled into the project.
There was a problem hiding this comment.
What would be a better location for these files? Also should we make it clearer that these files are auto-generated and shouldn't be mutated? I imagine we could update the file header to call that out and also name the files differently, i.e.
There was a problem hiding this comment.
What about leaving them in the ref folder?
Agreed - updating the header and naming the files to indicate they are generated would be useful.
There was a problem hiding this comment.
That breaks the DefaultCompileItems stuff that we need to work around.
Actually this doesn't break anything, does it? In the first commit I explicitly added that file to the exclusion list of glob patterns.
What about leaving them in the ref folder?
IMO having a dedicated "ref" folder just for this single file is weird... But I don't have a better suggestion aside from what I'm proposing here (src folder).
There was a problem hiding this comment.
I don't like having .cs files inside the src folder that aren't compiled into the project. I think it is confusing.
You mention in the OP that
It feels right to produce reference assemblies via Roslyn for all tfms
If we ever wanted to do this in the future, we would have more than a single file. I imagine we could have one ref .cs file for each TFM that we target. (And maybe if the APIs aren't different between TFMs, then just a single file.)
Either way, these files aren't part of the src project, so I wouldn't put them in the src folder.
There was a problem hiding this comment.
I agree with @eerhardt. These aren't src. I don't see why keeping them in ref is a problem.
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
Outdated
Show resolved
Hide resolved
| [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] | ||
| public static T? GetValue<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] T>(this Microsoft.Extensions.Configuration.IConfiguration configuration, string key) { throw null; } | ||
| public static T? GetValue<T>(this Microsoft.Extensions.Configuration.IConfiguration configuration, string key) { throw null; } |
There was a problem hiding this comment.
These .ref.cs files are auto-generated by GenAPI based on the by Roslyn generated reference assembly and don't serve as an input to anything. They exist just to have a record of public API changes. Is this maybe wrong in the source assembly or it GenAPI that makes this in-correct?
There was a problem hiding this comment.
When opening the generated reference assembly with ILSpy, this is what I see:
[RequiresUnreferencedCode("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")]
[return: Nullable(2)]
public static T GetValue<[Nullable(2)] [DynamicallyAccessedMembers(/*Could not decode attribute arguments.*/)] T>(this IConfiguration configuration, string key)
{
throw null;
}
Looks like GenAPI is the culprit.
There was a problem hiding this comment.
@ViktorHofer if these files are just to serve as a reference for the public API (and aren’t used to produce a ref assembly), would using something like the PublicApiAnalyzer fit the use case better?
There was a problem hiding this comment.
Looks like GenAPI is the culprit.
Can you log a GenAPI bug for this? These attributes need to be preserved in the ref file - they are important to the API.
There was a problem hiding this comment.
@ViktorHofer if these files are just to serve as a reference for the public API (and aren’t used to produce a ref assembly), would using something like the PublicApiAnalyzer fit the use case better?
@jkoritzinsky PublicApiAnalyzer is quite verbose in comparison to GenAPI, there are two files, a .shipping.txt and a .nonshipping.txt. Also you need two of these files for each target framework if you want to track the API surface of every tfm. In comparison, GenAPI emits the same C# file that we have been looking at for the last years and a C# allows to encode public api differences based on i.e. the target framework. @ericstj and I were discussion this more than a year ago and we spoke with the Roslyn team to see if PublicApiAnalyzer could instead emit C# code but we didn't get very far with that discussion.
src/libraries/Microsoft.Extensions.Configuration.Xml/Microsoft.Extensions.Configuration.Xml.sln
Show resolved
Hide resolved
| @@ -10,9 +10,9 @@ public static partial class ActivatorUtilities | |||
| { | |||
| public static Microsoft.Extensions.DependencyInjection.ObjectFactory CreateFactory([System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type instanceType, System.Type[] argumentTypes) { throw null; } | |||
| public static object CreateInstance(System.IServiceProvider provider, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type instanceType, params object[] parameters) { throw null; } | |||
| public static T CreateInstance<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] T>(System.IServiceProvider provider, params object[] parameters) { throw null; } | |||
| public static T CreateInstance<T>(System.IServiceProvider provider, params object[] parameters) { throw null; } | |||
....Specification.Tests/src/Microsoft.Extensions.DependencyInjection.Specification.Tests.ref.cs
Outdated
Show resolved
Hide resolved
| // Changes to this file must follow the https://aka.ms/api-review process. | ||
| // ------------------------------------------------------------------------------ | ||
|
|
||
| [assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(Microsoft.Extensions.DependencyInjection.ServiceCollection))] |
There was a problem hiding this comment.
Can we ensure this isn't broken?
There was a problem hiding this comment.
Yes, the type forward is present in the generated reference assembly:
I don't know if that's possible but would it make sense to teach GenAPI to include typeforwards in the ref file?
There was a problem hiding this comment.
I don't know if that's possible but would it make sense to teach GenAPI to include typeforwards in the ref file?
Considering this is how we verify public API changes, I think it should include TypeForwardedTo.
...braries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.ref.cs
Outdated
Show resolved
Hide resolved
| @@ -51,8 +49,8 @@ public partial class CompilationOptions | |||
| private readonly object _dummy; | |||
| private readonly int _dummyPrimitive; | |||
| public Dependency(string name, string version) { throw null; } | |||
| public readonly string Name { get { throw null; } } | |||
| public readonly string Version { get { throw null; } } | |||
| public string Name { get { throw null; } } | |||
There was a problem hiding this comment.
Is this actually a Roslyn reference assembly bug? ILSpy shows me the following in the reference assembly:
[System.Runtime.CompilerServices.NullableContext(1)]
[System.Runtime.CompilerServices.Nullable(0)]
public readonly struct Dependency : IEquatable<Dependency>
{
public string Name
{
[CompilerGenerated]
get
{
throw null;
}
}
There was a problem hiding this comment.
@eerhardt actually what we had in the reference source isn't valid C#:
There was a problem hiding this comment.
what we had in the reference source isn't valid C#:
I find that hard to believe, considering that we are compiling the reference source.
You are using a class in that snippet, this is on a struct.
There was a problem hiding this comment.
ILSpy shows me the following in the reference assembly:
Notice the readonly in the signature: public readonly struct Dependency.
There was a problem hiding this comment.
Got it. Another GenAPI bug that needs to be investigated.
...crosoft.Extensions.Caching.Abstractions/src/Microsoft.Extensions.Caching.Abstractions.ref.cs
Outdated
Show resolved
Hide resolved
| @@ -87,6 +85,6 @@ public partial class PollingWildCardChangeToken : Microsoft.Extensions.Primitive | |||
| public bool ActiveChangeCallbacks { get { throw null; } } | |||
| public bool HasChanged { get { throw null; } } | |||
| protected virtual System.DateTime GetLastWriteUtc(string path) { throw null; } | |||
| System.IDisposable Microsoft.Extensions.Primitives.IChangeToken.RegisterChangeCallback(System.Action<object?> callback, object? state) { throw null; } | |||
| System.IDisposable Microsoft.Extensions.Primitives.IChangeToken.RegisterChangeCallback(System.Action<object> callback, object state) { throw null; } | |||
There was a problem hiding this comment.
TODO: Understand this change
There was a problem hiding this comment.
@maxkoshevoi - is this another nullable mismatch between ref and src?
There was a problem hiding this comment.
This one is valid change (all tests call RegisterChangeCallback with null state, for example)
There was a problem hiding this comment.
If all tests call that method with state being null, wouldn't this change then be incorrect and the state should continue to be nullable?
There was a problem hiding this comment.
Oh, wait, by This one is valid change I meant that code in main branch is valid.
Change in this PR is strange indeed. Both ref and src are annotated as RegisterChangeCallback(System.Action<object?> callback, object? state) on main
ViktorHofer
force-pushed
the
AvoidRefProjectsInOOBs
branch
2 times, most recently
from
e42b682 to
d65a703
Compare
Contributes to dotnet#58163 This is a POC that demonstrates that most out-of-band reference projects in dotnet/runtime can be deleted. See reasoning in the linked issue. The remaining OOBs are more challenging as they are depend on the PNSE or PartialFacade infrastructure which itself depends on the reference source / reference assembly: System.Management (PNSE) System.Net.Http.WinHttpHandler (PNSE) System.Reflection.Context (PNSE) System.Security.Cryptography.Pkcs (Facade) System.Security.Cryptography.ProtectedData (Facade + PNSE) System.Security.Cryptography.Xml (Facade) System.Security.Permissions (Facade) System.ServiceModel.Syndication (Facade) System.ServiceProcess.ServiceController (Facade + PNSE) System.Speech (PNSE) System.Threading.AccessControl (Facade + PNSE) System.Windows.Extensions (PNSE)
ViktorHofer
force-pushed
the
AvoidRefProjectsInOOBs
branch
from
d65a703 to
1e04999
Compare
| public bool HasChanged { get { throw null; } } | ||
| protected virtual System.DateTime GetLastWriteUtc(string path) { throw null; } | ||
| System.IDisposable Microsoft.Extensions.Primitives.IChangeToken.RegisterChangeCallback(System.Action<object?> callback, object? state) { throw null; } | ||
| System.IDisposable Microsoft.Extensions.Primitives.IChangeToken.RegisterChangeCallback(System.Action<object> callback, object state) { throw null; } |
There was a problem hiding this comment.
nullability change, cc @eerhardt
There was a problem hiding this comment.
We are already discussing it here: #65775 (comment)
It looks to me that this is a bug in whatever tool you are using to gen the ref file.
| public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, Microsoft.Extensions.Primitives.StringValues right) { throw null; } | ||
| public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, object? right) { throw null; } | ||
| public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, string? right) { throw null; } | ||
| public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, string?[]? right) { throw null; } | ||
| public static bool operator !=(object? left, Microsoft.Extensions.Primitives.StringValues right) { throw null; } | ||
| public static bool operator !=(string? left, Microsoft.Extensions.Primitives.StringValues right) { throw null; } | ||
| public static bool operator !=(string left, Microsoft.Extensions.Primitives.StringValues right) { throw null; } |
There was a problem hiding this comment.
nullability change, cc @eerhardt
There was a problem hiding this comment.
This looks like a mistake in #57395. The == operator has string? left. This one should as well. So I believe the fix here is to fix the src file.
cc @maxkoshevoi
There was a problem hiding this comment.
@maryamariyan - can you fix this one in #66215?
| int System.Collections.Generic.IList<string?>.IndexOf(string? item) { throw null; } | ||
| void System.Collections.Generic.IList<string?>.Insert(int index, string? item) { } | ||
| void System.Collections.Generic.IList<string?>.RemoveAt(int index) { } | ||
| System.Collections.IEnumerator? System.Collections.IEnumerable.GetEnumerator() { throw null; } |
There was a problem hiding this comment.
nullability change, cc @eerhardt
There was a problem hiding this comment.
This looks like a bug in the tool you are using to generate the file. In https://raw.githubusercontent.com/dotnet/runtime/main/src/libraries/System.Runtime/ref/System.Runtime.cs, IEnumerable is declared as:
public partial interface IEnumerable
{
System.Collections.IEnumerator GetEnumerator();
}And in this src file:
runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringValues.cs
Lines 409 to 412 in 385fc88
| public partial struct Enumerator : System.Collections.Generic.IEnumerator<string?>, System.Collections.IEnumerator, System.IDisposable | ||
| { | ||
| private object _dummy; | ||
| private int _dummyPrimitive; | ||
| public Enumerator(ref Microsoft.Extensions.Primitives.StringValues values) { throw null; } | ||
| public string? Current { get { throw null; } } | ||
| object System.Collections.IEnumerator.Current { get { throw null; } } | ||
| object? System.Collections.IEnumerator.Current { get { throw null; } } |
There was a problem hiding this comment.
nullability chnage, cc @eerhardt
There was a problem hiding this comment.
This change looks like a valid change to me and was a mistake in #57395. The src file has object?:
cc @maxkoshevoi
There was a problem hiding this comment.
@maryamariyan - can you fix this one in #66215?
| int System.Collections.IList.Add(object value) { throw null; } | ||
| void System.Collections.IList.Clear() { } | ||
| bool System.Collections.IList.Contains(object? value) { throw null; } | ||
| int System.Collections.IList.IndexOf(object? value) { throw null; } | ||
| void System.Collections.IList.Insert(int index, object? value) { } | ||
| void System.Collections.IList.Remove(object? value) { } | ||
| bool System.Collections.IList.Contains(object value) { throw null; } | ||
| int System.Collections.IList.IndexOf(object value) { throw null; } | ||
| void System.Collections.IList.Insert(int index, object value) { } | ||
| void System.Collections.IList.Remove(object value) { } | ||
| void System.Collections.IList.RemoveAt(int index) { } | ||
| int System.Collections.IStructuralComparable.CompareTo(object? other, System.Collections.IComparer comparer) { throw null; } | ||
| bool System.Collections.IStructuralEquatable.Equals(object? other, System.Collections.IEqualityComparer comparer) { throw null; } | ||
| int System.Collections.IStructuralComparable.CompareTo(object other, System.Collections.IComparer comparer) { throw null; } | ||
| bool System.Collections.IStructuralEquatable.Equals(object other, System.Collections.IEqualityComparer comparer) { throw null; } |
There was a problem hiding this comment.
nullability changes, cc @eerhardt
There was a problem hiding this comment.
This looks like the same bug as #65775 (comment)
| @@ -27,14 +28,14 @@ public static partial class ConfigurationBinder | |||
| [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] | |||
| public static object? GetValue(this Microsoft.Extensions.Configuration.IConfiguration configuration, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] System.Type type, string key) { throw null; } | |||
| [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] | |||
| public static object? GetValue(this Microsoft.Extensions.Configuration.IConfiguration configuration, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] System.Type type, string key, object defaultValue) { throw null; } | |||
| public static object? GetValue(this Microsoft.Extensions.Configuration.IConfiguration configuration, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] System.Type type, string key, object? defaultValue) { throw null; } | |||
There was a problem hiding this comment.
nullability change, cc @eerhardt
There was a problem hiding this comment.
This looks like a mistake in #57418. I really wish API Compat would ensure these nullable annotations are consistent across src and ref.
Your change here is "good".
There was a problem hiding this comment.
@maryamariyan - can you fix this one in #66215?
| // ------------------------------------------------------------------------------ | ||
|
|
||
| namespace Microsoft.Extensions.Primitives | ||
| { | ||
| public partial class CancellationChangeToken : Microsoft.Extensions.Primitives.IChangeToken | ||
| { | ||
| public CancellationChangeToken(System.Threading.CancellationToken cancellationToken) { } | ||
| public bool ActiveChangeCallbacks { get { throw null; } } | ||
| public bool ActiveChangeCallbacks { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } } |
There was a problem hiding this comment.
Is this valuable in the ref file? It feels like noise to me.
There was a problem hiding this comment.
It's more than not valuable; it's leaking implementation details. We must not include such attributes in the ref.
|
Just to make sure I understand, what are the expectations regarding the timeline of this PR getting merged? |
I understand the ambition behind this proof of concept, but I don't think now is the right time. Things have been incredibly busy getting releases out the door this month as-is, and large changes have introduced issues (such as the issue fixed in #66176) that have been time-consuming to investigate and address while trying to hit release dates. We're not in a position to take in large changes like this unless there's a pressing need that justifies it and the acceptance criteria of the change are very clear. @ericstj / @ViktorHofer is there a pressing need here, or could this wait for reconsideration after Preview 4? |
|
This is a proof of concept and there are no plans to get this merged anytime soon. It's a PR as I depend on the additional CI validation and the valuable feedback of a few selected team mates like @eerhardt.
I'm doing this work on my spare time (I'm still on holiday) and as said don't plan to take up any of your team's time as it's just my personal project.
With any contribution there's a always a chance that something breaks. The changes that I contributed over the last six weeks made the product better in numerous ways (build perf, code quality, ObsoleteAttribute that weren't reaching customers because they were missing from the source, ...). I don't think you meant to stop me in my ambitions, neither do I think that you wanted to discourage me but you could have phrased this differently. I totally understand that you want to stabilize the product, avoid unnecessary risk and might be short on resources. I would have fixed the linked issue myself today but @joperezr was quicker and jumped on it, which is great as he was able to unblock builds sooner than I would have been able to. |
|
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
ghost
locked as resolved and limited conversation to collaborators
Contributes to #58163
This is a proof of concept that demonstrates that most out-of-band reference
projects in dotnet/runtime can be deleted. See reasoning in the linked
issue.
The remaining out-of-band projects are more challenging as they depend on the
PNSE or PartialFacade infrastructure which itself depends on the
reference source / reference assembly:
Microsoft.Bcl.AsyncInterfaces (Facade)
System.Management (PNSE)
System.Net.Http.WinHttpHandler (PNSE)
System.Reflection.Context (PNSE)
System.Security.Cryptography.Pkcs (Facade)
System.Security.Cryptography.ProtectedData (Facade + PNSE)
System.Security.Cryptography.Xml (Facade)
System.Security.Permissions (Facade)
System.ServiceModel.Syndication (Facade)
System.ServiceProcess.ServiceController (Facade + PNSE)
System.Speech (PNSE)
System.Threading.AccessControl (Facade + PNSE)
System.Windows.Extensions (PNSE)
The moved ".ref.cs" files are auto-generated by GenAPI which is now used
during the build to keep these files up-to-date. It takes the platform agnostic
NetCoreAppCurrent Roslyn generated reference assembly as an input. API
compatibility is checked during package validation.
Observations:
new-api-needs-documentationlabeler bot needs to be updated to also look for ".ref.g.cs" file changes.@ericstj @danmoseley @joperezr @eerhardt @carlossanlop