-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Disallow interfaces with static virtual members as type arguments #5955
Comments
I understand that the language should provide language constructs which are executable at runtime and do not throw unneccessary exceptions. But I do not agree that constraints must not contain interfaces with static virtual members. This restricts the usefulness of such interfaces with static virtual members in a major way. Instead I recommend that the the language defines, that such constraints can only be fulfilled by non interface types like structs or classes (implementing this static members) . This should be checked by the compiler. Similar constraints exist today: 'where T : struct, I' and 'where T : class, I'. |
This was discussed and accepted in LDM: https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-03-28.md#type-hole-in-static-abstracts |
@jaredpar, would you please assign to whomever will be implementing this restriction? |
I read these discussions and understand the problem. |
From the example, class D : C<I>
{
public override void M<U>() => Console.WriteLine(U.P);
} This would mean |
Can you give an example?
Can you clarify why? |
As an example: Code which implements the strategy pattern.
When I understand the proposal right, then This is the point I'm not satisfied with and I think it should be possible in a release version bejond preview. |
I don't believe that's the case. T is not an interface containing static abstracts there. So there would be no issue. |
Maybe the proposal can clarify, where the runtime error is raised now and where the compiler error will be raised in future. |
The compiler error would be given at a site where an interface is used as a type argument and that interface contains at least one static without an impl (e.g. a static-abstract). In your case, here are the places where you supply type arguments (i've used interface INumber<T> {
//static abstract members here
}
interface ICalculationStrategy<T> where T : INumber<*T*> {
T PerformCalculation(T arg1, T arg2);
}
class ConcreteCalculationStrategy<T> : ICalculationStrategy<*T*> where T : INumber<*T*> {
public T PerformCalculation(T arg1, T arg2) {
// use static members of T for some math
}
}
// some concrete usage
ICalculationStrategy<*double*> strategy = new ConcreteCalculationStrategy<*double*>();
double result = strategy.PerformCalculation(1.0, 2.0); In none of those places are you passing an interface with abstract-statics. So this is all fine. -- I'm not sure on the runtime side yet. I'll have the runtime people weigh in on that. |
Thanks for your clarification. I see no direct problem now. |
Terrific! |
ProblemI understand the hole that this closes, but this seems very, very limiting. This change puts interfaces with static abstract members in a whole new category where you can't use them in most places you'd expect to, similar to ref structs. Except ref structs are usually used in specific, limited contexts. Interfaces are everywhere. Adding a new abstract static member to an interface should be a breaking change to implementors only, not to every single usage as a type parameter. You now can't use the interface type in common types such as Let's take the following interface: interface INode
{
string Text { get; }
static abstract INode Create(string text);
} The abstract static method is only for convenience, so you can do things like: T CreateNode<T>(string text) where T : INode => T.Create(text); which is awesome, and what abstract static methods were made for. Until you realize that all the following don't work anymore: var nodes = new List<INode>(); // CS8920
void Process(IEnumerable<INode> nodes) { } // CS8920
string[] GetTexts(INode[] nodes) => nodes.Select(node => node.Text).ToArray(); // CS8920
Task<INode> GetNodeAsync() { ... } // CS8920 Those methods aren't made to accept concrete implementations of By closing the small hole in the wall, you've ended up trapped in the basement. Solutions
In the The examples in the OP become: interface I
{
static abstract string P { get; }
}
class C<T> where T : concrete I
{
void M() { Console.WriteLine(T.P); }
}
new C<I>().M(); // CS8920 abstract class C<T>
{
public abstract void M<U>() where U : T;
public void M0() { M<T>(); }
}
interface I
{
static abstract string P { get; }
}
class D : C<I>
{
public override void M<U>() => Console.WriteLine(U.P); // CS8920
}
new D().M0(); |
I've said it before and I'll say it again: It'd be quite useful to bring a |
Just to make sure I understand the file decision made by the team here: shouldn't this issue be called "Disallow interfaces with static abstract members as type arguments" instead of "Disallow interfaces with static virtual members as type arguments"? Because as of now in DotNet 7 Preview 6, VS 2022 17.3 Preview 3 interfaces with |
This breaks a CRTP-style pattern I was using to pass along objects with many varied shapes (i.e. interface implementations) that conform to certain patterns (i.e. must provide certain static abstract members). In particular, there's an interaction with type-inference limits that's painful: I type inference will not infer type arguments of an interface, so if I have a |
|
This is still quite inconvenient, even simply for using the new INumber related interfaces, let alone trying to use this more broadly. The issue MrJul describes above applies even to examples using INumber; e.g.: using System.Numerics;
Console.WriteLine(Bla(new[] { 3, }.AsEnumerable()));
Console.WriteLine(Bla2(new[] { 3, }.AsEnumerable()));
Console.WriteLine(Bla3(new[] { new Num(3), }.AsEnumerable())); //only this actually compiles without error
Console.WriteLine(Bla4(new[] { new Num(4), }.AsEnumerable()));
static TResult Bla<TSelf, TResult>(IEnumerable<IMultiplicativeIdentity<TSelf, TResult>> nums)
where TSelf : IMultiplicativeIdentity<TSelf, TResult>
=> TSelf.MultiplicativeIdentity;
static TResult Bla2<TSelf, TResult>(IEnumerable<TSelf> nums)
where TSelf : IMultiplicativeIdentity<TSelf, TResult>
=> TSelf.MultiplicativeIdentity;
static TResult? Bla3<TSelf, TResult>(IEnumerable<IThing<TSelf, TResult>> nums)
where TSelf : IThing<TSelf, TResult>
=> default(TResult);
static TResult? Bla4<TSelf, TResult>(IEnumerable<TSelf> nums)
where TSelf : IThing<TSelf, TResult>
=> default(TResult);
interface IThing<TSelf, TResult> where TSelf : IThing<TSelf, TResult> { }
sealed record Num(int Value) : IThing<Num, int>; The reason to use interface-typed parameters rather than The issues with this solution to the hole therefore doesn't just limit where Given the fairly convoluted scenario this protects against, and the fact that even without this additional limitation (disallowing interfaces with static virtual members as type arguments) the error is at least caught at runtime - is this really worth it? This features makes understanding static abstract more complex in normal usage, and imposes inconvenient limitations; the upside is compile-time rather than runtime error-reporting in hopefully niche cases. I also support the idea MrJul proposed to add an additional |
Since this is not possible now with compile errors, how about a way to tell the C# compiler that an explicit static implementation must be overridden instead using an attribute to flag it as "Required static members that implements the 'X' interface has not been implemented in "Y'." error. |
I'm not sure if this is the right place for this feedback, but after the third time this week that I've stumbled upon this limitation with Preview 7, I thought I'd share a couple of bog-standard use-cases: // Bootstrapping...
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddSingleton<ILocator, GoodLocator>(); // causes compiler error
// Mocking...
var sub = NSubstitute.Substitute.For<ILocator>(); // causes compiler error
// Factories...
builder.Services.AddSingleton<ILocator>(services => LocatorFactory.GetLocator(true)); // causes compiler error
public interface ILocator
{
public static abstract string ConnectionString { get; }
}
public class GoodLocator : ILocator { public static string ConnectionString => "C:\\Data\\my.db"; }
public class BetterLocator : ILocator { public static string ConnectionString => "https:\\my.db"; }
public interface ILocatorFactory
{
public static abstract ILocator GetLocator(bool onlyTheBest);
}
public class LocatorFactory : ILocatorFactory
{
public static ILocator GetLocator(bool onlyTheBest) => onlyTheBest switch
{
true => new BetterLocator(),
false => new GoodLocator()
};
} All three cases throw this compiler error. The beauty of this static abstract feature is not just in greenfield math libs, but in our ability to remove cruft in existing code, where static methods were the answer all along. @MadsTorgersen explicitly mentions factories as a great use case in his recent NDC Copenhagen talk, for example. Am I understanding/utilizing this feature as intended? Is this a Preview 7 bug, or do I have it all wrong? Please inform! :) P.S. I'm only showing static methods for the implementations here, but let's assume there's some great brownfield instance-based members/methods on these classes...we're just sprinkling on a little more static abstract awesomeness.... |
Can you clarify exactly how you'd use the static abstract method there @dcuccia? Given that you're using a DI pattern, you're not operating on generic type parameters, so I don't know how you'd actually call that method. |
@333fred sure. I assumed above that there were instance methods/properties as well, I was just highlighting the incremental static additions I'd add. Swag below at a more real-world scenario. The work-around alternative is probably to segregate the using System.Reflection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using FluentAssertions;
using NSubstitute;
using Xunit;
public interface IStaticService
{
// cross-cutting concern: have services boostrap their own dependencies (opinionated approach...)
public static abstract void AddServiceDependencies(IServiceCollection services, IConfiguration configuration);
}
// this version of IService causes build error
// decorating concrete classes with IStaticService instead avoids the build error, but does not enforce
// the contract at the IService level
// public interface IService : IStaticService { string ServiceName { get; } }
public interface IService { string ServiceName { get; } }
public record Customer(string Id, string Name);
public interface ICustomerService : IService
{
List<Customer> GetAllCustomers();
}
public interface IRepository<T>
{
List<T> Find(Predicate<T> query);
}
public class CosmosDbRepository : IRepository<Customer>
{
public List<Customer> Find(Predicate<Customer> query) => throw new NotImplementedException();
}
public class CosmosDbCustomerService : ICustomerService, IStaticService
{
private readonly IRepository<Customer> _repository;
public CosmosDbCustomerService(IRepository<Customer> repository)
{
_repository = repository;
}
public string ServiceName => nameof(CosmosDbCustomerService);
public List<Customer> GetAllCustomers() => _repository.Find(c => c.Name != null);
public static void AddServiceDependencies(IServiceCollection services, IConfiguration configuration)
{
services.AddOptions();
services.TryAddSingleton<CosmosDbRepository>(); // boostraps the IRepository<T> container
services.TryAddSingleton<IRepository<Customer>, CosmosDbRepository>(); // boostraps the IRepository<T> container
}
}
public class RavenDbCustomerService : ICustomerService, IStaticService // IStaticService added here to avoid compilation error
{
public string ServiceName => nameof(RavenDbCustomerService);
public static void AddServiceDependencies(IServiceCollection services, IConfiguration configuration) { }
public List<Customer> GetAllCustomers() => new();
}
public enum CustomerServiceType
{
CosmosDb,
RavenDb
}
public static class CustomerServiceFactory
{
// simplify factories that don't themselves need instance members by using static contracts
public static ICustomerService GetCustomerService(IServiceProvider provider, CustomerServiceType type) => type switch
{
CustomerServiceType.CosmosDb => provider.GetRequiredService<CosmosDbCustomerService>(),
CustomerServiceType.RavenDb => provider.GetRequiredService<RavenDbCustomerService>(),
_ => throw new ArgumentOutOfRangeException()
};
}
public class BusinessApi
{
private readonly ICustomerService _customerService;
public BusinessApi(ICustomerService customerService) =>
_customerService = customerService;
public List<string> GetPrintableCustomerList() =>
_customerService
.GetAllCustomers()
.Select(c => $"{c.Id}: {c.Name}")
.ToList();
}
public static class BusinessApiServiceExtensions
{
public static void AddServicesForAssembly(this IServiceCollection services, IConfiguration configuration)
{
Type interfaceType = typeof(IService);
IEnumerable<TypeInfo> serviceTypesInAssembly = interfaceType.Assembly.DefinedTypes
.Where(x => !x.IsAbstract && !x.IsInterface && interfaceType.IsAssignableFrom(x));
foreach (var serviceType in serviceTypesInAssembly)
{
// add the service itself
services.TryAdd(new ServiceDescriptor(serviceType, serviceType, ServiceLifetime.Singleton)); // runtime error, I think
// add any required service dependencies, besides the service (e.g. what's needed for construction)
serviceType.GetMethod(nameof(IStaticService.AddServiceDependencies))! // BOOTSTRAPPING EXAMPLE
.Invoke(null, new object[] {services, configuration});
}
}
}
public class BusinessApiTests
{
[Fact]
public static void GetPrintableCustomerList_ReturnsEmptyList_WhenNoCustomersExist()
{
// Arrange
// compiler error CS8920 "static member does not have a most specific implementation of the interface"
ICustomerService subCustomerService = NSubstitute.Substitute.For<ICustomerService>();
subCustomerService.GetAllCustomers().Returns(new List<Customer>()); // MOCK EXAMPLE
BusinessApi api = new (subCustomerService);
// Act
List<string> shouldBeEmpty = api.GetPrintableCustomerList();
// Assert
shouldBeEmpty.Count.Should().Be(0);
}
[Fact]
public static void WebApplication_Build_ShouldNotThrowException()
{
// Arrange
WebApplicationBuilder builder = WebApplication.CreateBuilder();
builder.Services.AddServicesForAssembly(builder.Configuration);
// compiler error CS8920 "static member does not have a most specific implementation of the interface"
builder.Services.AddSingleton<ICustomerService>(services =>
CustomerServiceFactory.GetCustomerService(services, CustomerServiceType.CosmosDb));
// Act
WebApplication app = builder.Build();
ICustomerService customerService = app.Services.GetRequiredService<ICustomerService>();
// Assert
customerService.Should().NotBeNull();
}
} EDIT - I should mention the BusinessApiServiceExtensions boostrapping approach was borrowed from a YouTube presentation by Nick Chapsas. EDIT 2 - Simplified the AddServicesForAssembly extension and replaced erroneous ILocator reference with ICustomerService EDIT 3 - Updates to streamline the point, and make compilable (with dependencies). See here for full solution: https://github.com/dcuccia/StaticAbstractDemo |
It's genuinely unclear to me what would be expected to happen with this. You're providing IService as the type-arg, but IService legit does not have an impl for |
|
But you're calling |
Oh I see. Sorry, this was a result of my attempt to simplify some code, the generic argument of the IServiceProvider extension method is not the main point here, but I can try to clean that up/simplify. |
Thanks! |
Done - let me know if you'd like a full "working" project sample with refs to NSubsitute, Cosmos, Scrutor, etc. |
Can you remove all usages of 'var' and provide the actual type here? Thanks! |
What compiler error do you get here? |
Yes, that woudl be helpful. Trying the above code leads to just far too many errors to try to work through. |
@CyrusNajmabadi I updated the above based on your comments, made self-contained, and put up a full version here: https://github.com/dcuccia/StaticAbstractDemo Uncommenting the |
While I can see where you're coming from with this, this demonstrates the thing I was getting at: the only way to the invoke the method is via reflection, as you're not in a generic context. It seems to me that this is a scenario that actually is better served by separation of concerns: separating the concern of creating a service and using the service. It may be that most services end up implementing both interfaces, but the requirement for all of them doesn't make sense to me. |
For me, the point is opting-in to using static abstract on an existing interface will break code throughout a typical .Net codebase, so it doesn't feel very idiomatic to me. Mocks, factories, DI containers all use interfaces as generic type arguments, and with this limitation, the feature won't get nearly the utilization/adoption it would otherwise. |
As I said, I can understand where you're coming from with this. It's certainly possible we decide to accept this type hole in the future and loosen the restriction. As often do, we've started more narrow, and we're seeing where people run into limitations (as you are right now :) ). |
Have you guys taken a stance on the |
A great approach! Thanks for considering my feedback. :) |
That is already a thing. Indeed, a type with only static virtual members is valid to use as a type argument, as it does not suffer from the same hole as static abstract members does. |
Oh, I thought static virtual DIM was not being supported for this currently. Reading the original thread indicates it's not, but was being considered for possible future. |
Unfortunately, even the following very simple example produces a compile error due to the restriction. I think that this should work. Func<IFoo> getFoo = () => new Foo(); // Error: CS8920 The interface 'IFoo' cannot be used as type argument. Static member 'IFoo.P' does not have a most specific implementation in the interface.
interface IFoo
{
static abstract string P { get; }
}
class Foo : IFoo
{
public static string P => "I am implemented";
} |
I have exactly same problem. Any updates on this? |
This comment was marked as abuse.
This comment was marked as abuse.
@KatDevsGames Your post was hidden for violations of the .Net code of conduct: https://dotnetfoundation.org/about/policies/code-of-conduct Please refrain from similar posts. |
Why not disallow calling any virtual/abstract members of an interface and allow it as a type argument? |
I have a very simple use case of serializing an object of interface type to JSON. The interface happens to have a static abstract method that has nothing to do with the JSON serialization. However, this line of code currently cannot compile due to CS8920 in C#. JsonSerializer.Serialize(new Model { ... } as IModel); I think we should disallow calling any virtual/abstract members of an interface and allow it as a type argument like @calvin-charles suggested. |
Hi. I stumble around this issue. My main goal is to provide a static Create property (or a method whatever), defined from my interface. But then I fall into CS8920 error pit. Thank you. |
Am I crazy or isn't I suppose the real intended use case is still in heavy numeric code, and I guess most of that isn't async. I realize that the language team has a difficult problem here and I think they overall do a great job with these kinds of tradeoffs. And I certainly won't claim to have a novel solution. But anyway here's my hope that some more time gets put into trying. |
This proposal attempts to address a type hole that has been pointed out a number of times with static virtual members, e.g. here.
Static virtual members (Proposal: static-abstracts-in-interfaces.md, tracking issue: #4436) have a restriction disallowing interfaces as type arguments where the constraint contains an interface which has static virtual members.
This is to prevent situations like this:
If this were allowed, the call to
C<I>().M()
would try to executeI.P
, which of course doesn't have an implementation! Instead the compiler preventsI
(and interface) from being used as a type argument, because the constraint contains an interface (alsoI
) that has static virtual members.However, the situation can still occur in a way that goes undetected by the compiler:
Here
I
is not directly used as a constraint, so the compiler does not detect a violation. But it is still indirectly used as a constraint, when substituted for the type parameterT
which is used as a constraint forU
.The runtime protects against this case by throwing an exception when
M0
tries to instantiateM<>
withI
, but it would be better if the compiler could prevent it.I propose that we change the rule so that:
This simple rule protects the assumption that any type used as a type argument satisfies itself as a constraint. That is the assumption behind the language allowing e.g. the
M<T>
instantiation above, where the type parameterU
is constrained byT
itself.It is possible that this restriction would limit some useful scenarios, but it might be better to be restrictive now and then find mitigations in the future if and when we find specific common situations that are overly constrained by it.
The text was updated successfully, but these errors were encountered: