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

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

  • Comments 26

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?

  • 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.
  • 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));
    }

    }
  • 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?
  • +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".
  • "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.
  • -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.
  • 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.
  • +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.
  • 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
  • PingBack from http://paidsurveyshub.info/story.php?title=bcl-team-blog-an-interesting-discussion-around-a-particular-breaking

  • PingBack from http://debtsolutionsnow.info/story.php?id=10803

Page 2 of 2 (26 items) 12