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
Anonymous
November 03, 2004
"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
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.Anonymous
November 03, 2004
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.Anonymous
November 03, 2004
"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!Anonymous
November 03, 2004
The comment has been removedAnonymous
November 03, 2004
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.Anonymous
November 03, 2004
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... ;)Anonymous
November 03, 2004
For: Making a clean API is valuable. It's a minor breaking change in my mind.Anonymous
November 03, 2004
Make the change. No brainer. Clean API is paramount.Anonymous
November 03, 2004
Change it to protected and make the compiler issue a warning.
I doubt it's a breaking changeAnonymous
November 03, 2004
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".Anonymous
November 03, 2004
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).Anonymous
November 03, 2004
Do it! Cleanup.Anonymous
November 04, 2004
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.Anonymous
November 04, 2004
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.Anonymous
November 04, 2004
The comment has been removedAnonymous
November 04, 2004
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));
}
}Anonymous
November 04, 2004
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?Anonymous
November 04, 2004
The comment has been removedAnonymous
November 04, 2004
The comment has been removedAnonymous
November 04, 2004
-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.Anonymous
November 04, 2004
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.Anonymous
November 05, 2004
+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.Anonymous
November 16, 2004
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 ManagerAnonymous
May 29, 2009
PingBack from http://paidsurveyshub.info/story.php?title=bcl-team-blog-an-interesting-discussion-around-a-particular-breakingAnonymous
June 19, 2009
PingBack from http://debtsolutionsnow.info/story.php?id=10803