An interesting discussion around a particular Breaking Change [Kit George]


So an interesting situation arose today I though I would share with everyone. I’d love your input on this issue as well, if you feel strongly one way or the other.

Curiously, the C# compiler (at least) allows you to write the following code:

public abstract class Test {
    int _val;
    public Test() {_val = 9;}
    public Test(int i) {_val = i;}
    public virtual int Value { get { return _val; } }
    public override string ToString() { return _val.ToString(“N”); }
}

What’s so interesting? Well, we have an abstract class with a public .ctor (indeed, two of them). The fact that it’s public is a bit of an odd thing, given that no-one can actually instantiate an instance of this class: it’s abstract! Now of course, subclasses can reference the public .ctor like so:

class Test2 : Test {
    public Test2() : base(12) {}
    public Test2(int i) : base(i) {}
}

But the fact the the .ctor is public on Test is misleading. It gives the impression that it can be called from any code, whereas it’s only use is the above: from a subclass. Because of this, it would have been far more suitable to write the following code in the base class:

public abstract class Test {
    int _val;
    protected Test() {_val = 9;}
    protected Test(int i) {_val = i;}
    public virtual int Value { get { return _val; } }
    public override string ToString() { return _val.ToString(“N”); }
}

The interesting part comes when we discovered today that some teams have changed APIs which originally had the public .ctor, to instead be protected. Traditionally, reducing the scope of a member in this way is considered a breaking change, but in this particular instance, there should be no consumer who is actually adversely affected: the only APIs that could originally take advantage of the public .ctor were subclasses, and they can still use the protected .ctor.

So the arguments for/against become:

  • Against: There is the off-chance that some code out there has a whacky way of using the public .ctor in some weird way. It’s a long-shot, but obviously, whenever you change code, there’s the 1-million shot. The benefit of changing from public to protected in this case is solely cleanliness of the API. It doesn’t make any functional difference for the key scenario, accessibility from a subclass. On this basis, the change is not worthwhile
  • For: Making a clean API is valuable. It is less confusing to the consumer, and moving forward, ensures people have no doubts to the usage of that API. The chance of someone actually being broken is so small in this situation, that it does not offset the value of having a clean API. Make the change

Where do you sit?


Comments (26)

  1. Anonymous says:

    "For."

    Make the change; I am always in support of cleaning up APIs, and nothing even needs to be depreciated in this case.

    Perhaps you should investigate issuing a compiler warning (or maybe an error?) in such situations to prevent or minimize it in the future.

  2. Anonymous says:

    What is the wacky way? I am curious. Cannot be reflection you are referring to — that can get to the protected as well.

    If the wacky way is really wacky there is not much chance of breaking anything.

  3. Anonymous says:

    Please don’t ever make breaking changes.

    I don’t want to spend the effort to deal with broken code, even if it was stupid in the first place.

    Feel free to add warnings, but never break working code.

  4. Anonymous says:

    "The interesting part comes when we discovered today that some teams have changed APIs which originally had the public .ctor, to instead be protected."

    Why do you have team members doing this? Don’t these "teams" have any kind of code review process in place? You don’t make that kind of a change to an existing API without some serious thought (and maybe a little bit of buy-in from the community wouldn’t hurt).

    I say to leave it the way it is. So what if it is a little bit "misleading". It doesn’t hurt anything. I wouldn’t even bother to through up a warning. There are a lot of people (including myself) who would be pretty upset if their working code suddenly started throwing compiler warnings because of something so trivial.

    Spend time on the real issues and quit making changes to things that already work!

  5. Anonymous says:

    One more point and then I’ll jump down off of my soap box….

    Making this change or issuing a compiler warning actually makes it less intuitive to new developers. I like things simple.

    Today, the following rule applies:

    – Constructors can have public, protected, or private access specifiers.

    By making a change you have rewritten the above rule to be:

    – Constructors can have public, protected, or private access specifiers… unless the class is marked as abstract, in which case it can only have protected and private constructors.

    That is significantly more difficult for a new programmer to understand and remember. Keep it simple.

    As long as the compiler throws an error if someone tries to directly instantiate an abstract class, then we are fine the way it is. After all, we have been using the Framework for many years and this is the first time that I have heard anyone even consider this "issue". It doesn’t sound like the API is very "misleading".

  6. Anonymous says:

    Matt: The same counter-argument can also be made.

    A general rule:

    – A class with a public constructor can be instantiated.

    Short and simple, yes. So the programmer tries to instantiate an abstract class with a public constructor and it correctly errors. Things apprently are not so simple.

    New general rule:

    – A class with a public constructor can be instantiated as long as it is not abstract, in which case it can not.

    Not so simple anymore.

    I think issues such as these need to be fixed and possibly warned about by the compiler.

  7. Anonymous says:

    If you try to instantiate an abstract class through reflection using a ConstructorInfo handle, you get a System.MemberAccessException ("Can not create an abstract class."). If you somehow manage to produce IL that attempts to invoke an abstract class constructor, it fails at runtime with a System.InvalidOperationException ("Instances of abstract classes can not be created.").

    I guess if someone were relying on the member being there, but not for purposes of ever calling it, it might be a problem. E.g. typeof(Test).GetConstructor(new Type[0]) would now be null, where it returned an instance previously. Seems the liklihood of this is rather low… 😉

  8. Anonymous says:

    For: Making a clean API is valuable. It’s a minor breaking change in my mind.

  9. Anonymous says:

    Make the change. No brainer. Clean API is paramount.

  10. Anonymous says:

    Change it to protected and make the compiler issue a warning.

    I doubt it’s a breaking change

  11. Anonymous says:

    For.

    And I am also in favor of a compiler warning for it.

    I support Ben Monroe’s view (above) in the ‘what about newbies?’ issue: "A class with a public constructor can be instantiated".

  12. Anonymous says:

    I also "For". But i also "for" go further and invent something like:

    public abstract class Plugin

    {

    public mandatory abstract Plugin();

    public mandatory Plugin( Context ctx ) { // … }

    protected Plugin( SomeStrangeStuff stuff ) { // … }

    public abstract void Do();

    }

    Where "mandatory" keyword marks constructors that each subclass must implement with the same access modifier as declared. In this case constructor for abstract class could also be "abstract".

    More grounds are in my post to other copy of this message (excuse my for such a scattering).

  13. Anonymous says:

    Maybe there is something like this internally, but I can’t find it with a quick search…

    I have found that it is generally best to have a standing published policy about different classes of changes.

    For instance, say the policy for technically-breaking-but-probably-only-for-a-few-people-doing-really-strange-things changes is "always choose to clean up the api, but support the old api for two releases and throw deprecation warnings".

    While there are always exceptional cases, if this policy is clear to all then consumers know exactly what to expect and can plan to manage upstream changes, and it becomes easier for the dev team to make consistent decisions as these cases come up.

    Over time, you get the clean api you want, while giving api consumers the ability to manage change in a reasonable way.

  14. Anonymous says:

    I can’t believe that so many people are looking to have this cleaned up and potentially add a compiler warning.

    I currently use the VS.NET "Add Class" feature when creating classes. By default it creates a public constructor. I therefore have a lot of abstract classes sitting in my code with public constructors. What is the harm? Maybe some trivial clarity issues (I still think that people are making too big of a deal about this). However, if you start throwing a compiler warning at me I’m going to be a little upset.

    A warning should indicate a possible problem. Not simply an issue with clarity. After all, if you intend to throw up warnings related to clarity then you would need to start warning people when they don’t match common programming standards or don’t use code commenting correctly.

    Given that the user cannot cause ANY damage by having a public constructor on a class marked as abstract, no warning should be given.

    If you want to make changes to the API to improve clarity then do so at your own risk. However, you will have a lot of developers on your back if you start throwing up compiler warnings regarding "clarity" issues.

  15. Anonymous says:

    If you are going to go to the trouble of adding a new compiler warning, here are a few others you might want to consider… 😉

    WARNING: You appear to be making too much money. The going rate in India for code of this quality is $12/hour.

    WARNING: C# is turning into another C++. You appear to only be using about 25% of the features available in the language. Please consult an approriate guide on how to implement the other 75%. Your coding prowess will be judged by your peers based on the inclusion of confusing Anonymous Methods throughout your code.

    WARNING: The book sitting on your bookshelf titled "Teach Yourself C# in 21 Days" is obsolete. Microsoft Press will soon be publishing a replacement titled "Teach Yourself C# in 42 Days".

    WARNING: Your project contains over 400,000 lines of code and does not contain any abstract classes and does not implement any interfaces. You may want to consult with the "Gang of Four" for assistance in refactoring your application.

    WARNING: You have marked a UserControl with the abstract keyword. Any classes derived from this class will not be editable in the Forms Designer. We don’t really know why.

    WARNING: Your program did not generate any warnings.

    WARNING: The "Academic’s" at Microsoft are more concerned about the clarity of the API than about actually providing useful classes. The API is subject to change at any time. Breaking changes may be added for the sole benefit of making the existing API more understandable to programmers who currently aren’t even confused by it. Issues that arise during discussions at lunch may be escalated to unnecessary levels and may result in unsanctioned API changes by internal departments.

    WARNING: The above warning has discredited you with the "Academic’s" at Microsoft. No other warnings will be read in this post. You should just give up now.

    WARNING: You are spending too much time on the Internet. You should get back to work to avoid losing your job.

  16. Anonymous says:

    For the API changes Against the warning. Since you can actually call a public constructor (See Code Below) I wouldn’t want the compiler to issue a warning even if people who write code like this should be fired. However the APIs should demonstrate best practices, since most new programmers follow what they see and it would be cleaner.

    abstract class Master

    {

    private int i;

    public Master(int i)

    {

    this.i = 10 + 1;

    }

    protected Master()

    {

    }

    public int Weird(int i)

    {

    return this.i + i;

    }

    [STAThread]

    static void Main(string[] args)

    {

    new Other();

    }

    }

    class Sub : Master

    {

    public Sub(int i)

    {

    }

    }

    class Other : object

    {

    public Other()

    {

    Master m = new Sub(0);

    Type t = m.GetType();

    ConstructorInfo [] info = t.BaseType.GetConstructors();

    Console.WriteLine(m.Weird(10));

    info[0].Invoke(m, new object[]{1});

    Console.WriteLine(m.Weird(10));

    }

    }

  17. Anonymous says:

    A few more thoughts and then I’ll try and let it go.

    In VS.NET, if you declare a sealed class with a protected constructor you get a compiler warning (albeit with a really lousy message that makes very little sense).

    I wouldn’t have added this warning message but it seems to me that adding a warning in this case is much more justified. After all, the VS.NET "Add Class" functionality creates a class with a default public constructor. In the above case, someone would have had to mark the class as sealed AND change the default access level of the constructor to be protected. This person is clearly confused. I still don’t necessarily advocate showing a warning but it makes more sense in that case. However, I am against any warning messages that attempt to decipher the "intent" of a programmer.

    So here are a few final notes:

    1) Since the compiler already issues a warning for a sealed class with a protected constructor, the original designers of Visual Studio may have already considered the case for an abstract class with a public constructor. If so, they must have decided not to issue a warning. If this is the case, what was the original reasoning? Should that reasoning be overturned now? Is there anyone around who might know the answer to this?

    2) The notion that the access level of a constructor is used as the main method of determining whether or not a class can be instantiated is wrong. For example, a class with a private constructor can still be instantiated using a static factory method. The programmer should assess the ability of a class to be instantiated by FIRST looking to see if it is abstract and THEN checking the access level of the constructor. Not the other way around. Similarly, to check whether or not a class can be inherited from you FIRST check to see if it is sealed and THEN check to see if it has an appropriate constructor.

    3) Any confusion caused by the current API should be considered as a one-time cost per programmer. Once a programmer gets over this little bump, the knowledge is transferrable to any other class in the API. On the other hand, any new warning message generated comes at a much greater cost. Each programmer on the team is hit with this recurring mess of warnings each time the application is compiled (until someone finally changes the code to keep the warnings from appearing). You should be able to easily see that adding a warning message will impact thousands of programmers hundreds of times over the years. The cost of adding that warning far outweighs the individual "learning curve" cost per programmer of a minimally confusing API.

    4) Raise your hand if this issue has EVER caused you confusion while using the API. Why am I still even talking about this?

  18. Anonymous says:

    +1 for changing the constructor.

    +1 for a compiler warning.

    If I write code that is clearly stupid and nonsensical, then personally, I want the computer to whap me upside the head and say, "That code is clearly stupid and nonsensical".

  19. Anonymous says:

    "If I write code that is clearly stupid and nonsensical, then personally, I want the computer to whap me upside the head and say, "That code is clearly stupid and nonsensical"."

    You just called the original implementers of the .NET framework stupid and nonsensical!

    I would hardly call having a public constructor on an abstract class nonsensical. As far as mistakes are concerned, this one is pretty minor. Nobody is going to be tripped up by it and it doesn’t cause any erroneous behavior. It is simply a matter of some people being much more anal than others. I am personally VERY anal about software and this one doesn’t bother me at all. That just HAS to tell you something (people who know me are shaking their heads in agreement right now).

    By the way, I’m not sure who started the "compiler warning" discussion but I don’t think that it was a BCL team member. Anyone considering the compiler warning idea hasn’t done a business analysis of the costs involved in doing that (and I’m not talking about the mere cost of the coding). That would be a major support disaster with virtually no payback.

  20. Anonymous says:

    -1 on the warning.

    Possibly -1 on the change:

    Having the constructors public in this case is a clue to implementors that perhaps they should implement public constructors with the same signatures. This depends on the usage patterns expected for the class – there must be some or it wouldn’t be there in the first place! If I see a protected method in an abstract class, I would not expect to implement it as a public method. A constructor is just another method – with a particular usage, true, but still just another method.

    If communicating the expected usage is worthwhile then it should communicate as much as possible. If you expect an implementation to be public, then surely the best way to communicate that is by making the base method public. This applies to constructors just as much as any other method, even though the usage of them is slightly different.

  21. Anonymous says:

    Take the middle road. Add the protected ones, move the implementation there, put an ObsoleteAttribute on the public .ctors and call the protected constructor from there. Then remove the public .ctors in vnext.

    All in all, I would add that to a design guideline, add a compiler warning and an FXCop rule and go over all of the code of the other teams to make sure that none of them did this as well.

  22. Anonymous says:

    +1 for the change. Many programmers are instructed to make their code look like Microsoft’s, so Microsoft should set a good example wherever possible, especially since the chance of breaking something here is incredibly remote… I’m not entirely sure if it’s even possible.

    +1 For a compiler warning. There is really no reason to have an abstract class with a public constructor, plus warnings never hurt anyone.

  23. Anonymous says:

    Based on the comments in this block, Kit asked me to respond with the Visual C++ point of view.

    In C++, the language design team has generally tried to avoid make assumptions about usefulness of some coding practices. Over and over, some things that the language design team assumed should not be done later proves to be widely used for a specific and useful purpose. Warnings certainly mitigate that, but we try to keep warnings limited to potentially dangerous behavior.

    Given that there is no evidence of possible danger here, we’ve decided to not implement a warning for this pattern. Instead, FxCop and other post-build analysis tools can provide scrutiny for practices like this.

    I hope that makes sense for anyone using Visual C++,

    Brandon Bray

    Visual C++ Compiler Program Manager