Proposal: Add optional warning for implicit runtime casts

[binki]

Version Used:

Visual Studio 15 Preview 4 (unsure of how to find what version of roslyn it uses).

Steps to Reproduce:

1. Set warning level to 4 (maximum).

2. Compile a program which has implicit casts such as the following:

   using System;
   using System.Linq;
   
   class Program
   {
       static void Main(string[] args)
       {
           // Implicit cast because var was not used.
           foreach (string thing in new object[] { 1, "asdf", 4, })
               Console.WriteLine(thing);
   
           // Implicit cast because the type was specified.
           var stuff = from string s in new object[] { "2", 4, "6", } select s;
           foreach (var thing in stuff)
               Console.WriteLine(thing);
       }
   }
   

3. Observe that there are no warnings and yet runtime exceptions occur at each place where an implicit cast was used.

Expected Behavior:

I’d like to have compiler support in eliminating accidental usage of implicit casts from my codebase (e.g., something to pass to /warnaserror). These casts are not always obvious and they don’t always compile to runtime casts, but when they do they can result in errors that are not seen until runtime. One great strength of C# is its static typing. But if the language makes it so easy to accidentally write code that emits runtime casts, without even an optional warning, the compiler’s support for static typing is wasted.

I understand that a lot of people rely on these constructs to use legacy APIs which use non-generic collections. Thus I suggest that this new warning be made opt-in. I don’t know the best way to go about that though…

I’d like usage of the construct foreach («TypeName» item in collection) to emit this warning if it compiles to a runtime cast and for from «TypeName» item in collection select item to emit the same warning if it compiles to a call to Cast<«typeName»>(). Something like a message describing both the target and source types of the implicit cast:

Program.cs(9,18): warning CSXXXX: Specifying type results in a runtime cast from 'object' to 'string'. Consider using 'object' or 'var' instead.

Or for a more real-life example:

Program.cs(9,18): warning CSXXXX: Specifying type results in a runtime cast from 'IContainer' to 'Container'. Consider using 'IContainer' or 'var' instead.

Actual Behavior:

No warnings.

[svick]

I think this sounds like something that would be suitable for an analyzer.

It's not something that's likely an error in your code, so I think that a warning built into the compiler is not suitable. But it is something that you might reasonably decide to not allow in your code style. So I think an analyzer would be ideal.

[binki]

@svick

It's not something that's likely an error in your code

It’s something that is likely an error after refactoring. For example, you may have an ArrayList that contains ClassA instances prior to refactoring but ClassB instances after refactoring and ClassB cannot be assigned to ClassA. The foreach (ClassA x in/from ClassA x in constructs will continue compiling but start throwing runtime exceptions. Making this a warning would help people migrate to generic types (even if they have to write generic-supporting wrappers for winforms APIs to do so safely) and also help people discover errors during compile time.

It would be fine as an analyzer because that can be used to generate warnings too. I am thinking more about this and maybe writing analyzers is easy enough that even I could do it. Don’t know if/when I’ll get to it, though.

However, I do have a concern about writing this as an analyzer. Depending on the Roslyn API, which I have not looked into yet, it might be impossible for an analyzer to hook into all places where implicit runtime casts happen. I only pointed out two places that I know of this happening: foreach («TypeName» and from «TypeName». Are these really the only two places in csharp where this happens? Do the tools which Roslyn provides let me generally discover all locations where implicit runtime casts happen regardless of language or would I have to explicitly know about all the possible constructs and then update my analyzer when new features are added to csharp? If, instead, this were a core compiler warning, it could be:

1. Versioned with the compiler and with the language to catch exactly the constructs that result in implicit runtime casts for a particular version of the language (including future, yet-to-be-imagined features which allow implicit runtime casts).
2. Guaranteed by the Roslyn team to work correctly instead of relying on me/other outsider guessing where all the implicit runtime casts are in the language.

[binki]

Thought that I’d update here to mention that I have C# foreach basically implemented in https://github.com/binki/OhNoPub.ImplicitCastAnalyzer and available as https://www.nuget.org/packages/OhNoPub.ImplicitCastAnalyzer/ . To finish C# support I need to add LINQ query syntax support and I also want to add the equivalent VB.net support.

It is hardcoded to emit a warning. As suggested earlier in this thread, I can make this into a compilation error with:

<PropertyGroup>
  <WarningsAsErrors>OhNoPubImplicitCastForeach</WarningsAsErrors>
</PropertyGroup>

It also has a codefix which is hardcoded to provide var (which I want to keep as an option, though I think some users would prefer to also have a codefix to use the explicit type or even be able to configure the analyzer to only show either var or explicit type).

Criticism/suggestions/PRs (as long as I can be convinced of them ;-)) welcome, including for the horrendous typos in the README which I just haven’t gotten around to fixing :-/.