Language proposal: "mustoverride" method modifier for virtual methods

[Opiumtm]

Introduce optional mustoverride method modifier for virtual methods.

Every derived class should override this virtual method if modifier is applied on method in parent class.

Reason: some virtual method's implementations must be unique for every class. For example, if class supports "deep cloning" (or some customized serialization logic), every derived class should introduce its own version of DeepClone() method. If derived class doesn't override base class DeepClone(), it's an obvious bug.

At low-level it can be implemented as [MustOverride] attribute. Compiler should throw at least warning if any derived class doesn't override method.

[HaloFour]

Sounds like something accomplished easily through attributes and analyzers without requiring any modifications to the language.

[MgSam]

@CyrusNajmabadi It's great that you went out and made an analyzer for this purpose. Thank you for writing it. I certainly do not have the time to take on new side projects but I very much encourage you to submit to Roslyn Analyzers.

I think if people are encouraged to use analyzers as a solution they should be directed towards documentation and towards opening an issue on the Roslyn Analyzers repo for such a case.

All that said, it taking you 10 minutes to write is a straw man. I never implied it would be hard for you to write analyzers. You are a Roslyn expert, a compiler expert, and undoubtedly a smarter-than-average developer given your position on one of the most technical and high profile teams at Microsoft.

For the average developer, what took you 10 minutes might take them days, weeks, or even months to learn all the Roslyn machinery. Also, for the average developer, they have a full-time job which is almost certainly not writing compilers and thus getting management buy-in to spend the time learning to write an analyzer would be a challenge. I myself have not written an analyzer before, but have worked with Expression Trees and am currently suffering through writing a Source Generator (so syntax trees aren't new to me), and it would have taken me minimum a few days of research and experimentation to do what you did there.

To summarize, I've never said or implied that analyzers couldn't perform certain functions in lieu of new language features. They are very powerful and frankly I'd wish more features were analyzers rather than new syntax. My argument is and remains that it's too hard and too niche to expect users to go implement themselves and in my observation, the suggestion of using them has often been used as a proxy for shutting down discussion rather than enabling it.

[CyrusNajmabadi]

but I very much encourage you to submit to Roslyn Analyzers.

I do not feel like this particular space is important enough to warrant as part of Roslyn. The community interest is tepid (more downvotes than upvotes), and i personally find the design pattern suspect. I have no interest in codifying this as something roslyn would support. My demonstration was to show that this is extremely cheap and a viable way for people to get these sorts of changes in a very reasonable time period. If you want to get this in the language, yo uhave to convince us it is worthwhile (which this proposal isn't doing), and also get it implemented and shipped. Both of those are massive hurdles that analyzers get out of hte way.

[CyrusNajmabadi]

I think if people are encouraged to use analyzers as a solution they should be directed towards documentation and towards opening an issue on the Roslyn Analyzers repo for such a case.

I have not suggested that Roslyn-Analyzers be the place for these. Just that analyzers are a suitable solution. Roslyn Analyzers, like csharplang, has their own set of assessments they woudl need to see pass before accepting this sort of thing, or doing the work for it.

Again though, that's the point here. Instead of always making this into something that must be owned by a party that has to be both convinced of merit and of the costs involved, an analyzer can be immediately produced by those who do think it is important and who would want it now.

[CyrusNajmabadi]

All that said, it taking you 10 minutes to write is a straw man.

It is not a strawman. It is a demonstration for this precise issue that there aren't deep complexities or difficulties standing in the way. There is some stuff to learn, but that's literally how development works.

For the average developer, what took you 10 minutes might take them days, weeks, or even months to learn all the Roslyn machinery.

In practice, we see developers taking about a day to get ramped up and ready to go on this. THat's nothing. Again, that's orders of magnitude less time and effort than both convincing anyone to make such a language change here, and also the work necessary to ship such a language change. There are active and thriving communities for this in places like Discord and other venues. They have many people (including Roslyn team members) helping people learn this stuff and allowing more and more people to do this sort of thing.

That's teh point here that keeps getting ignored. What used to be something that would take years for a team to do, can now be done by someone experienced in minutes, or someone new in a day or so. That is absolutely teh antithesis of your idea that this is shutting things down. This is enabling the ecosystem and community to not be beholden to our schedules and processes and high-bars for features, but instead operate at a speed that actually makes this previously-impossible thing possible.

Also, for the average developer, they have a full-time job which is almost certainly not writing compilers and thus getting management buy-in to spend the time learning to write an analyzer would be a challenge

We also have full time jobs, which would not include work like this. Many of us spend our free time on it because that's what a community is. Instead of an ivory tower approach where only a select few can even consider this, and only accomplish it with vast resources, we are making these capabilities and knowledge available to all with engaged communities that help each other make this sort of thing possible and affordable for all.

[CyrusNajmabadi]

My argument is and remains that it's too hard and too niche

I don't see how that's the case. Just look at the gist i wrote up. It's extremely trivial and in line with how one would describe the problem space. Specifically you want to look at classes and see if htere are MustOverride members taht have not been overridden. Users familiar with reflection (which is a large portion fo .net since v1) would find this totally grokkable. And even brand new users could pick up the concepts demonstrated here fairly easily.

Indeed, i think this serves well to demonstrate just how awesome analyzers are. it's not a crazy amount of code. It doesn't need uber complex algorithms. It's just a simple "take a look at this type, and report if there's an issue" helper.

to expect users to go implement themselves and in my observation, the suggestion of using them has often been used as a proxy for shutting down discussion rather than enabling it.

It is not. It's a standard way of discussion and costing. When we think about things ourselves, we also ask ourselves "wait, why do a lang feature here when an analyzer/source-generator suffices?" It's a very relevant and important part of the discussion space here, and this very thread indicates why. Not discussing analyzers leaves this to languish with no viable path to shipping. There is more community dislike of the proposal than like. There is no LDM interest in this. There has been no movement on this for 5 years and likely will still see no movement for 5 more. Effectively, this doens't have a path through the language. However, analyzers mean that that isn't a blocker for someone. Yes, it may require someone new to spend a day reading some tutorials and learning this. But they can then benefit from that for every day after that once they make this analyzer. You view that one-day investment as not acceptable. But i compare it to the alternative, which is years of waiting, getting nothing out of it. To me, it's a slam dunk improvement that actually has allowed the community and ecosystem to solve tehse real problems they care about, without being beholden to the extremely slow pace that language design necessitates.

[jnm2]

@HaloFour I can't enforce usage of a certain analyzer if someone uses my library. How else could I enforce overriding of a virtual method?

[HaloFour]

@jnm2

You can't, just like you couldn't if they decide to use a different version of the C# compiler without this support, or another compiler, or IL. It's a facility that doesn't exist in the CLR.

[Opiumtm]

@HaloFour value tuples with named members (C#7 feature) also doesn't exists on CLR side and are emulated by C# compiler.

[HaloFour]

@Opiumtm

Indeed, but tuples aren't trying to enforce anything. You can treat them as their raw type with no consequences at all. The worst thing that an errant compiler could do is ignore the names.

A better comparison would be non-nullable references. Similarly they would only exist as attribute-based metadata and require a compiler that understands and can enforce the rules. That would make it just as easily defeatable as this proposal. However, there is significantly more benefit for non-nullable reference types than there is for this proposal given the frequency in which the use case occurs.

[jnm2]

@HaloFour The ideal outcome in my mind is for older versions of the compiler to fail to be able to create a derived class at all. Aren't there modreqs for this sort of thing?

[Opiumtm]

@HaloFour We always have some ways to cheat. It's OK when some thing is enforced to prevent unintentional bugs. If someone intentionally seek a way to overcome restriction - he would find this way, no matter how you try to prevent him. Developer at any time can invoke private method with reflection, after all. And it's OK if he have serious reasons to do so. And it's not an argument to abandon private members of classes, if anyone can cheat this restriction by using reflection.

[jnm2]

What's at stake here must be enforced with the same rigor as any other type safety rule. Anything less than enforcing this at the CLR level is worthless.

[HaloFour]

@jnm2

IIRC modreq only applies to fields, parameters and return values, not to types or methods as a whole. And all you need to do to satisfy the signature requirement of modreq is to include the same in the calling signature, so it's just as easy to defeat as an attribute would be.

@Opiumtm

Type safety is pretty well enforced by the CLR. It's not like you can cheat by trying to define a generic type that doesn't respect a generic parameter constraint; that results in a runtime exception. Sure, you can call private methods through reflection, but doing so

In any case, in my opinion the frequency in which a feature like this would really be useful is quite low so I don't see it gaining a lot of traction, particularly if it would involve CLR modifications in order to properly support. And the specific grammar of the proposal, a mustoverride keyword, is pretty out of place for the C# language. It's a VB.NET keyword afterall, and it simply means abstract.

[Opiumtm]

@HaloFour If you try hard enough, you can totally overcome CLR type safety. You can use unsafe code to manipulate raw memory and even mutate one class into another at runtime. I have seen "proof of concept" article where someone have changed int to string by overriding virtual method table pointer at runtime inside unsafe code block. It works.

[HaloFour]

@Opiumtm

Maybe, but you couldn't produce a perfectly verifiable and trustable assembly which does any of that. At the point that you're executing unmanaged/unsafe code all bets are off. But that's not particularly relevant to this discussion.

If the goal is to provide a way for developers to avoid unintentional bugs through warnings then an analyzer seems like the appropriate tool. Modifying the compiler (and potentially the CLR, if you want any kind of actual enforcement) seems like quite a stretch to support what are likely only a handful of actual use cases.

[svick]

Do I understand it correctly that the base method is not abstract, because the derived method is expected to call the base method as part of its implementation? And that the obvious workaround of making the base method abstract and putting the base logic into a separate method is not sufficient?

[jnm2]

@svick In the cases I'm thinking of, the base type must be instantiable. That rules out abstract methods.

[YaakovDavis]

Regarding CLR vs. C# safety:
Since CLR evolution is stalled, C# IMO can no longer tie its safety features to those provided by the CLR.

I've already seen multiple safety proposals dismissed due to lack of "backing" by the CLR.

If we want to evolve C#'s safety features, we need to break some ties, and make compromises. Some things that are going to be "unsafe" at the code-level, will be totally safe at the CLR level.

This is not really new. e.g. Private members can be accessed in an "unsafe" manner at the CLR level, but not at the source level.

[jnm2]

@YaakovDavis

This is not really new. e.g. Private members can be accessed in an "unsafe" manner at the CLR level, but not at the source level.

I'm pretty sure this is incorrect. I've had the CLR reject plenty of stuff I've tried to do at the CLR level. Can you give an example? Reflection is by definition outside the safety rules of both IL and C#, so that doesn't count.

[jnm2]

@YaakovDavis There must be a fundamental difference between ILGenerator and LambdaExpression.Compile. Do compiled expressions generate unverified IL? If so, the unverified part is what puts it in the category of reflection.

[jnm2]

@CyrusNajmabadi NAudio's MidiEvent hierarchy. (To be fair, it could have been designed better, but this is where we are...)

[YaakovDavis]

@jnm2

There must be a fundamental difference between ILGenerator and LambdaExpression.Compile

Expression compilation in fact uses ILGenerator. Check out the LambdaCompiler type from System.Core.dll.

I don't think IL verification even checks member visibility. Visibility aspects are part of the CTS, which is logically a level above what the JIT operates on.
Though I might be wrong on some of the details of this.

[Opiumtm]

@HaloFour

I personally wouldn't advocate this due to the extremely niche use cases. I personally wouldn't advocate this due to the extremely niche use cases.

Not so niche. It greatly impact architecture design of libraries and frameworks. Especially if library classes are expected to be extended. It's a clear way to minimize ability to "shoot yourself in a foot" when logic is required to be unique for each derived class (abstract or not).

[HaloFour]

@Opiumtm

Not so niche. It greatly impact architecture design of libraries and frameworks. Especially if library classes are expected to be extended. It's a clear way to minimize ability to "shoot yourself in a foot" when logic is required to be unique for each derived class (abstract or not).

How many other examples can you enumerate besides trying to force "deep cloning"? I'd suggest the number of concrete use cases can be counted on one hand. That's not a great impact.

[Opiumtm]

@HaloFour
Practically any scenarios which depends on actual data fields/properties of classes in data manipulation logic. You can imagine any. For example, data-based (not reference-based) equality comparison is the case. If you don't override Equals(object) method or IEquatable<> interface members, derived class instance would count as equal to base class instance if all base fields/properties values are same. It's obviously wrong if derived class introduce new data fields/properties. Even if base class do check argument type with GetType(), derived class logic would be obviously broken with no visible signs of why it's broken.

So, you can count any scenario which depend on actual data layout of class hierarchy (especially when new data fields/properties are introduced in derived classes and should be taken into account in overrided method logic) or require strict type identity.

[HaloFour]

@Opiumtm

Practically any scenarios which depends on actual data fields/properties of classes in data manipulation logic. You can imagine any.

"You can use it anywhere" is not a use case. "Deep cloning" and some very narrow definition of equality, that's two use cases so far. Both are offered by contract in the BCL and those definitions can't be changed so it's not like you can improve either of them.

[Opiumtm]

@HaloFour
I mean, this case is not narrow if code use data-classes hierarchies (which is very common at data business-logic tier) and complex data manipulation logic. "Deep cloning" and "equality" are just simple and generic examples. As I mentioned, mustoverride is needed in data-manipulation scenarios when data classes are organized in hierarchy. There could be "map itself into intermediate over-the-wire packet or some network proxy" instead of "deep cloning". Just use your imagination.

Do you say, data manipulation with data classes hierarchies are uncommon in C# projects? Are you sure?

Both are offered by contract in the BCL and those definitions can't be changed so it's not like you can improve either of them.

It's easy to improve. Just mark those methods as 'mustoverride' in specific base class of own data classes hierarchy.

[dstarkowski]

No, it doesn't. Analyzers for now isn't mature tool for use it by all developers. Analyzers have no way to be enforced by library author.

You supply analyzers with your library and say: "Guys, you should really use those analyzers. Otherwise you may make some common mistakes." You did your job. If developers choose to ignore your warning, it's their own fault when they make those mistakes. The same as it's their fault for ignoring docs and best practices.

There are definitely some use cases for this proposal. I could find few in some of my previous projects. Still the benefit is tiny next to mentioned non-nullable references that I can imagine using all over my code.

[yaakov-h]

In theory I believe you could even include your custom analysers in the same package as the library, so that they get auto-installed by NuGet. Not sure that'd work for xplat, .NET Core, or consumers using paket etc., but it is an option.

[MkazemAkhgary]

mustoverride is basically abstract method. so that is already achievable. if base method have implementation you can use abstract override to override virtual method abstractly,

note that class that implements or overrides abstractly must be itself abstract. if you say it shouldn't be abstract then its design problem. because method must have implementation and non abstract class must have defined behavior.

example

class A
{
    virtual Foo(){}
}

abstract class B : A
{
    override abstract Foo();
}

class C : B // you get compiler error even though A has implementation but B forces that requirement.
{
    override Foo(){}
}

[jnm2]

No, the base class must not be abstract in this case. For example, a Clone method. You want that to be a virtual method but you want any derived class to override it.

[MgSam]

You can easily enforce this at runtime. Compare the type of this to the type of the declaring class, and if they're different, throw.

While I can see having this at compile time might be nice, I agree that is better suited to an analyzer.

[sharwell]

No, it doesn't. Analyzers for now isn't mature tool for use it by all developers.

NuGet-installed analyzers work as well as the compiler itself. The approach may still be new, but it's stable and reliable. There are two ways to distribute analyzers with a library (to form a "code aware library"):

1. Use one NuGet package containing both the analyzer(s) and the library itself
2. Create three NuGet packages
   • A package containing the analyzer
   • A package containing the library
   • A metadata-only package that declares a dependency on the analyzer and library

When using the metadata package, option 2 behaves like option 1. However, it is more flexible because it allows the analyzer to be upgraded separately from the library, and also allows the library to be used without the analyzer if desired.

Analyzers have no way to be enforced by library author.

This statement isn't exactly true. While the compiler doesn't specifically provide and document this as a feature, there are ways it can be done. However, it comes at non-trivial risk for the person using the analyzer and I can't see any reason it would ever be worth the risk. As a result it is rather actively discouraged when the topic comes up.

[JohnGalt1717]

I agree this is needed. It's great in dart (although implemented as an attribute that the compiler knows about.)

[MustOverride] and [MustCallSuper] in their case. I think they should be modifiers on the call however and enforced even if no analyzer exists.