30

① In following C# code, CS1729 occurs but I understand that CS0122 would be more appropriate.

namespace A
{
  class Program
  {
    static void Main()
    {
      Test test = new Test(1);
    }
  }
  class Test
  {
    Test(int i) { }
  }
}

CS1729: 'A.Test' does not contain a constructor that takes 1 arguments

CS0122: 'A.Test.Test(int) is inaccessible due to its protection level'

② In following C# code, CS0122 occurs but I understand that CS1729 would be more appropriate 

namespace A
{
  class Program
  {
    static void Main()
    {
      Test test = new Test();
    }
  }
  class Test
  {
    Test(int i) { }
  }
}

CS0122: 'A.Test.Test(int) is inaccessible due to its protection level'

CS1729: 'A.Test' does not contain a constructor that takes 0 arguments

Question: Is there any reason why CS0122 and CS1729 are swapped in ① and ② or is this C# compiler bug ?

P.S.: Errors in ① and ② can be reproduced with Microsoft Visual C# 2010 Compiler version 4.030319.1.

General Grievance
  • 4,555
  • 31
  • 31
  • 45
Fabien Launay
  • 639
  • 7
  • 16
  • +1 I agree with your analysis. One would assume that the compiler would perform resolution first, and then check accessibility. Instead, it appears that it is filtering on accessibility first, and then checking for a match. However, for performance reasons, the second may make more sense (depending completely on compiler internals, of course). – Jonathon Reinhart Feb 15 '14 at 22:25
  • 4
    To be fair, both error messages apply to both cases. Which one is discovered first and when the compiler stops reporting probably depends on internal algorithms. Demanding that the compiler picks 'the best' error (in human eyes) may be a bridge too far. – H H Feb 15 '14 at 22:31
  • 2
    Just a guess but maybe it's because C# automatically creates an empty constructor by default, when you don't pass a parameter, the compiler assumes the default one was used and checks accessibility first. In the first example, resolution of the method may have been deemed more efficient to check first since it's not the default behavious of a class. – keyboardP Feb 15 '14 at 22:40
  • 7
    @keyboardP Default, parameterless one is not added by compiler when you declare any other constructor explicitly. – MarcinJuraszek Feb 15 '14 at 22:42
  • 1
    @MarcinJuraszek - Indeed but I'm thinking that it could be a case of *when* the compiler checks to see if a default one should be added. I don't know much about the C# compiler so may be a wild guess (probably is :) ) – keyboardP Feb 15 '14 at 22:44
  • My assumption is that in case 1, the compiler is trying to find a constructor that matches that exact argument list and cannot, so reports this to you. In case 2, the compiler sees you using a default constructor and assumes you want to construct the object using any means necessary. Since no constructor is accessible in this case, it just reports the first constructor defined in the class. You can verify this by putting a Test(string s, int i){} constructor before the int one, and will see that in case 2 it is this constructor the compiler reports as not being accessible. – DavidN Feb 16 '14 at 14:48
  • @HenkHolterman Then what about the type [`System.Diagnostics.Eventing.Reader.EventBookmark`](http://msdn.microsoft.com/en-us/library/system.diagnostics.eventing.reader.eventbookmark.aspx)? If we say `new System.Diagnostics.Eventing.Reader.EventBookmark(new SerializationInfo(null, null), new StreamingContext(StreamingContextStates.All));`, we are told that no constructor takes two arguments. That is wrong! If we say `new System.Diagnostics.Eventing.Reader.EventBookmark()` it says that the constructor which does take two arguments is not accessible. Technically correct, but quite irrelevant. – Jeppe Stig Nielsen Feb 16 '14 at 21:48
  • (continued) If we care to check with reflection, `System.Diagnostics.Eventing.Reader.EventBookmark` also has an `internal` instance constructor which takes one parameter of type `string`. No parameterless constructor (not even `private` or `internal`) exists. – Jeppe Stig Nielsen Feb 16 '14 at 21:52
  • And on the other hand, `new System.Net.NetworkInformation.PingException();` says that no constructor takes `0` arguments, thereby revealing (to those who have understood what they read so far) that there must actually, on the contrary, be such a constructor, and yes there is (it is `internal`). By the way, I did this with Visual Studio 2013, targeting .NET 4.5.1. This also means that the strange behavior from the question, still exists in the newest release of the Visual C# compiler. **Reproduced.** – Jeppe Stig Nielsen Feb 16 '14 at 22:07

1 Answers1

13

Full disclosure: I work on the C# team at Microsoft.

Diagnostic reporting from a compiler is a tricky business! We spend a lot of time trying to ensure that the "best" diagnostic is reported for a particular error condition. However, this sometimes requires taking heuristics into account, and we don't always get that right. In this case, as @Henrik Holterman points out, both errors are reasonable (at least for the second case).

The first example is clearly a bug, though it's of low severity. After all, it's still an error with a somewhat "correct" (I'm being a bit gracious here) diagnostic. In the second example, both errors are correct, but the compiler failed to pick the "best", and hopefully, the most helpful diagnostic.

With the Roslyn C# compiler, we've had an opportunity to take a fresh look at our diagnostic reporting and make better choices. For these particular examples, the Roslyn compilers do in fact produce the errors that you were expecting. In the first example, CS0122 is reported, and in the second case, CS1729 is reported. So, you can rest assured that this is already fixed in a future release.

Dustin Campbell
  • 9,747
  • 2
  • 31
  • 32
  • If you see my comments (from three days ago) to the question, I cannot agree that both are applicable. For example saying that the overload does not exist just because that overload is `protected` (say) is not correct, technically, in my opinion. Similarly, complaining about the protection level of a completely irrelevant overload (the user specifically asked for another one) is not "correct". However, this may still be the best answer possible? – Jeppe Stig Nielsen Feb 19 '14 at 14:05
  • @Jeppe Stig Nielen -- yes, you are correct. That is clearly a bug. I tweaked my response to make the clearer. – Dustin Campbell Feb 19 '14 at 15:12
  • And in example (2) it is correct that the overload `A.Test.Test(int)` (secretly exists and) is inaccessible, but the user didn't even ask for that overload. It is just the overload which happens to be first in the code (yes, I tried with many inaccessible overloads present). – Jeppe Stig Nielsen Feb 19 '14 at 16:41
  • Yes, that's a correct analysis. In this case there are two potential diagnostics that could be reported depending on how overload resolution is implemented, but CS1729 is definitely the better one for the user. Note that this in the IDE experience this is very much a heuristic. We ensure that Go to Definition would still go to A.Test.Test(int) because it's not clear how the user might deal with the problem. Do they want to remove the parameter and make the constructor public, or do they want to add an argument at the call site? Fun stuff. :-) – Dustin Campbell Feb 19 '14 at 16:50