Harden against null arguments in simplifier

[jmarolf]

Fixes #5121

[jmarolf]

Reviewers: @balajikris, @brettfo, @CyrusNajmabadi, @davkean, @dpoeschl, @jasonmalinowski, @DustinCampbell, @Pilchie, @rchande

[DustinCampbell]

To be pedantic, this isn't really "hardening". It's making the APIs throw exceptions on null arguments like they already should have been. Are you in touch with the Refactoring Essentials folks per #5121?

👍

[davkean]

These are breaking changes. Do we take these through some sort of breaking change committee and/or document them?

[DustinCampbell]

Yes, they're technically breaking changes. Having null for any of these arguments would have failed very quickly but they wouldn't have failed with ArgumentNullExceptions and so would potentially fall through existing catch blocks.

[jmarolf]

@DustinCampbell filed icsharpcode/RefactoringEssentials#105 on RefactoringEssentials

[DustinCampbell]

Adding a few other folks to provide guidance on the API breaking changes. I'm totally comfortable with this one as I believe this is very seldom called by client code and the conditions under which a user would experience a behavior change are uncommon. In general, most client code calls this indirectly by applying simplification annotations in a CodeAction. The CodeAction engine calls the simplifier APIs on behalf of the client.

@mattwar, @AnthonyDGreen, @jaredpar, @srivatsn, @gafter

[balajikris]

@jmarolf I think Simplification tests are in EditorServicesTest2.
See: http://source.roslyn.io/#Roslyn.Services.Editor.UnitTests2/Simplification/AbstractSimplificationTests.vb

It was probably authored there to take advantage of VB xml literals but I'm not certain.. See if you should move your tests to that directory and/or unify their location.

[jmarolf]

@balajikris thanks, I'll move them.

[gafter]

These API changes are OK with me.

[jaredpar]

Fine with the behavior change in the API as well.

[jmarolf]

I'll make a wiki page to note this breaking change between 1.0 and 1.1.

[davkean]

I thought @gafter already created one.

[jmarolf]

@davkean I see, its under docs/Breaking API Changes.md I'll update this pull request to document the changes there.