Share via

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

  • Article

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

  • Anonymous
    November 03, 2004
    Doing something "whacky" implies they are violating some rule set forth by the CLI, the documentation, etc. Just as when using reflection to access private properties, users should be cautious when doing something - while possible - they shouldn't.

    For example, in a little sample I wrote recently for the CodeProject forums I used the RegistryKey class but needed the HKEY for a call not encapsulated by the BCL. Reflection seemed like the obvious choice but at the same time I noted that the field could change. In 2.0, in fact, it did (to a different Type). When I used reflection I basically took it upon myself to make sure my code doesn't break with future Framework versions since - despite the warning about using reflection - I did anyway.

    I think it's a good idea to mark these as protected, if not for the sole purpose of a cleaner design.
  • Anonymous
    November 03, 2004
    I suppose it becomes a valuation of the confusion that a public constructor on an abstract class could cause vs. the frustration someone would experience by having their whacky/weird scenario broken. In my opinion, both values are extremely small. I think the FOR argument edges out the AGAINST argument merely because the net effect is more positive for the developer. Another way to look at it is that this is not likely to be a decision that will have to be made again (at least there internally) with the proliferation of FxCop and design guidelines since v1.
  • Anonymous
    November 03, 2004
    [Reposted since there seem to be two copies of this post.]

    "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.
  • Anonymous
    November 03, 2004
    FxCop has a rule to check for this situation, I thought. But, yes, the compiler should catch this as it is simply wrong.

    Does Intellisense catch the fact that this class can't be created? Or, does it only look for the public constructor when populating its list of items? Is there a kludge in place to handle this very situation? There shouldn't be.

    My vote is to clean up the library. Don't let this propagate any further.

    -Martin
  • Anonymous
    November 03, 2004
    I agree.

    Remove situations like this from the Framework, and don't worry about breaking anyone who was relying it.

    Windows is already full of workarounds for developers who do the wrong thing, don't make the Framework the same.
  • Anonymous
    November 03, 2004
    +1 Change to protected
  • Anonymous
    November 03, 2004
    Change the API - move the Framework forward.
  • Anonymous
    November 03, 2004
    Related thoughts: i have an abstract class which is a base for hierarchy of some kind of plugin classes. All my plugin classes must have two constructors:
    1. Plugin( Context ctx, Object obj )
    2. Plugin( Context ctx, ObjectDescriptor objDesc )

    Those constructors create plugin object in one of two possible states, and are used by reflection.

    In my setup, base abstract class could not be created with an object reference, so first constructor is meaningless for it. So, i've tried to declare it like this:

    public abstract class Plugin {
    protected Plugin( Context ctx ); // This one is not needed in plugin interface
    public Plugin( Context ctx, ObjectDescriptor objDesc ); // This is second necessay constructor
    // ... and there is no first constructor here
    ...

    , thinking that it could give a hint to plugin implementors about necessary constructors. Alas, it anyway seems clueless, so i've removed both constuctors and ended with only default one (setting context through property).

    So, one my opinion, this is a wider problem than it seems: currently there is no way to define an interface of object construction, that must be implemented by all subclasses of current one. In some cases such mechanism would be very helpful.
  • Anonymous
    November 03, 2004
    IMHO, This must be compile warning.
    This is very easy to forget to remove abstract modifier from class. Warning like this one must give hint to developer about this.
    As well - some kind of low-severity FxCop warning can be generated if class no any abstract members.

    As for defining contructor contracts - this is non-sence. You will be never able to predict mandatory arguments for sub-class contructors. Mandatory parameters can be added, as well sub-class can pass some predifined value to base class contructors.
    The only reasonable usage for contructor contracts are generics. But this must be solved by providing additional new() restrictions.
  • Anonymous
    November 04, 2004
    Another +1 for changing the API!
  • Anonymous
    November 04, 2004
    +1, make it a compiler warning
  • Anonymous
    November 05, 2004
    Another +1 for changing the API to protected.
  • Anonymous
    November 05, 2004
    The comment has been removed