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?

  • "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.
  • 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.
  • 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.
  • "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!
  • 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".
  • 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.
  • 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... ;)
  • For: Making a clean API is valuable. It's a minor breaking change in my mind.
  • Make the change. No brainer. Clean API is paramount.
  • Change it to protected and make the compiler issue a warning.

    I doubt it's a breaking change
  • 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".

  • 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).
  • Do it! Cleanup.
  • 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.
  • 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.
Page 1 of 2 (26 items) 12