Winforms: ListBindingHelper.GetListName(1, new PropertyDescriptor[] { null }) throws NRE should return null

[hughbe]

[zsd4yr]

Hey @hughbe sorry to be a bummer -- but there is some on-going discussion internally about what the ebst way to handle these exceptions is, so you might want to hold your horses on working on these.

[zsd4yr]

My hope is to get back to you soon on a final determination here; sorry about the misunderstanding. Let's see where this goes first

[hughbe]

So this is an example where there's a real actual bug. We should simply return null instead of null refing - instead of throwing, we now return a meaningful value. This is breaking, but acceptable. This obviously differs from the other cases where we should throw ANE not NRE

[KlausLoeffelmann]

I am sorry. From my perspective, these are all changes which could break peoples code subtlety, so I strongly recommend against this, because I think the risk outwheigh the benefit _by far_! On top I would not accept this change based on promised test coverage. That should be included.

[hughbe]

I’ve submitted the PR with test coverage separately as the team previously wanted test prs away from cleanup prs away from bug fixes. IMO this is an improvement and a bug fix whereas one can argue changing NREs to ANEs is a less significant bug and is more about code quality. Clearly the team has internal disagreements so I’ll wait on you guys. I just think bug fixes where nres transform to useful returns being too risky is dangerous logic as we’ve had experience fixing these sorts of things in CoreFx and in .net core. Might be worth chatting with the other dotnet oss teams and see if their experience is something to reflect on :)

[KlausLoeffelmann]

I’ve submitted the PR with test coverage separately as the team previously wanted test prs away from cleanup prs away from bug fixes.

OK, I did not know. More to talk about! ;-)

I think the WinForms audience is somewhat different from the typical Core or even WPF audience (and if not now then definitely in the future, when we're getting closer to having a designer, and the migration wave from classic framework can really start). Folks will be having a hard time figuring out why their 20 year old, 1.000.000 loc WinForms app does behave weird in Core, and it'll be really subtle to debug this, especially there will be so many cases where the main code maintainer is probably gone since 10 years.

Don't get me wrong: I am totally in favor of code cleanup and improvements. But as I said: My greatest concern is, that WinForms core by this introduces so many (subtle) little breaking changes, that this will become a problem down the road and scare people off.

Maybe I am too conservative here, and that is why I'd really want to ask @terrajobst, @stephentoub and @karelz for their opinion.

[zsd4yr]

Okay going to update this thread with the decisions made among the team. I'm going to try to be as clear as possible so pardon the verbosity.

These recent pull requests have essentially 3 types of changes:

1. adding exceptions where there were none (for example: input validation)
2. removing exceptions where there were some (this PR); and
3. changing exceptions to be more appropriate

Number 1 (adding exceptions) should be fine as long as we do not go over-board; in particular there may be no reason to do this if we are about to make a system call which will also check for this and throw a similar exception. A good case to add exceptions is if we are about to perform an operation ourselves which will break if we do not check and throw exceptions where appropriate.

Number 2 (removing exceptions) should be handled with extreme care. We need to consider which existing applications might be try / catching for that particular exception and what removing and changing behavior might do to those applications. In particular, because these changes will only present themselves during runtime and not at compile time, they may be difficult to diagnosis. For cases where we would accept these changes, we would probably need a case-by-case justification and can't make this kind of change blanket across the board.

Number 3 (changing exceptions) is a trickier story. We are also super concerned here about breaking existing applications desirous to port to core. Because exceptions have an inheritance hierarchy, we can examine that to our advantage. See the below partial example of the c# exception hierarchy:

• Exception
  
  
  
  • ApplicationException
  
  
  • Your own exception types
  
  
  • SystemException
  
  
  • ArgumentException
    
    
    
    
    
    • ArgumentNullException
    
    
    
    • ArgumentOutOfRangeException
    
    
    
  
  
  • DivideByZeroException
  
  
  • IndexOutOfRangeException
  
  
  • InvalidOperationException
    
    
    
    
    
    • ObjectDisposedException
    
    
    
  
  
  • NullReferenceException
  
  
  • RankException
  
  
  • StackOverflowException
  
  
  • IOException
    
    
    
    
    
    • EndOfStreamException
    
    
    
    • FileNotFoundException
    
    
    
    • FileLoadException
    
    
    
  
  

What we can assume here is that anyone with an existing application try / catching a specific exception is going to be catching _that specific_ exception or a "more basic" exception. Because a try / catch for an InvalidOperationException will also catch an ObjectDisposedException (it is less basic, a more specific child), we can take advantage of this when choosing when to allow the changing of exceptions. So, for "changing exceptions", we again have 3 cases:

1. changing exceptions to be more specific (moving towards the leaves of the inheritance tree)
2. changing exceptions to be less specific (moving towards the root of the inheritance tree); and
3. changing exceptions to move across siblings / uncles or towards the leaves of those subtrees (for example, changing a Null Reference Exception to a Null Reference Exception)

For proposed exceptions, we have decided not to take case 2 or 3 here because of our concern in breaking existing applications desirous to port to core. We can only consider changes like case 1, where we move to a more specific exception.

So, I will go through soon and close issues / pull requests which have been opened addressing either of these changing type cases. For removing and adding exceptions as well as changing exceptions to be more specific, we will continue to consider these contributions during our 3.0 release timeframe.

[terrajobst]

We wouldn't make these changes in .NET Framework (due to the insane high compat bar) but we generally don't expect developers to catch NullReferenceException. Changing this to ArgumentNullException seems reasonable to me for .NET Core.

The other proposed behavior (instead of causing a null reference returning null) is generally acceptable as we don't consider turning an error case into a success case a breaking change.

[hughbe]

Okay so the team just closed all the NRE->ANE issues immo is there still not consensus?

[zsd4yr]

@terrajobst I spoke with Karel and the rest of the team and this is where we landed. If you think this is not reasonable, we should sync offline.

[terrajobst]

@terrajobst I spoke with Karel and the rest of the team and this is where we landed. If you think this is not reasonable, we should sync offline.

I don't think it's unreasonable, I just think the fixes seem reasonable. Any change is a risk, but the luxury of .NET Core is that we don't have to consider crazy scenarios where apps managed to depend on things like this; I'd say any code handling NullReferenceException and expect this to work well is unreasonable and we don't have to support this. At the same time, that doesn't mean that making these fixes is reasonable from a cost/benefit analysis. That's a decision that your team has to make. I was merely commenting on what I consider reasonable from a compatibility perspective.

[karelz]

@zsd4yr Immo has much higher authority here than I do. If he says it is fine, he is right :)

[zsd4yr]

Thoughts @dreddy-work @Tanya-Solyanik @KlausLoeffelmann @AdamYoblick @JuditRose ?

[hughbe]

So my perspective is basically meaningless but here’s what I think: I think changing NREs to ANEs and the like does impact compatibility, but it’s in the spirit of .NET Core to fix these bugs. In my opinion what’s the point of open sourcing Winforms and wpf as part of .net core if we don’t start improving the code - the approach of .net core fundamentally differs from netfx.

[zsd4yr]

So my perspective is basically meaningless but here’s what I think: I think changing NREs to ANEs and the like does impact compatibility, but it’s in the spirit of .NET Core to fix these bugs. In my opinion what’s the point of open sourcing Winforms and wpf as part of .net core if we don’t start improving the code - the approach of .net core fundamentally differs from netfx.

Certainly your opinion matters! In fact a lot -- as you say we open sourced this, in part, to better reach certain equivalence classes of the community. Definitely, your opinion matters.

There has been a very high compatibility bar in the .Net Framework, even more so for a project with as long and rich in history as Windows Forms. That said, part of the freedom of moving to core, as @terrajobst indicated, is the greater freedom we have when choosing to make breaking changes. I think we are more likely to take worthwhile breaking changes which can be caught at compile instead of at runtime. Again, I'd like to emphasize that @terrajobst is right in indicating that it may be considered unreasonable to code handle some of these exceptions at all.

I think we need to circle the wagons again and try to reach a wider consensus before proceeding.

[zsd4yr]

Followup here: We've got an _broader_ internal meeting scheduled for later this month. I will update this thread with the outcome of that meeting. For now, stalling these.

[zsd4yr]

@hughbe we have agreed to reconsider these changes. After speaking to higher ups and stakeholders in .NET Core, we believe there is value add here ans that the risk of regression may be allowable. We are going to individually triage these sometime soon, so look out for that in each open issue 😄

[zsd4yr]

Also I have re-opened those issues you created with I had previously closed

[zsd4yr]

approved