Error cs0030 cannot convert type string to int

Hi,

you need to implicitly convert an string to an int as here:

string strNumber = "123";
int nTest = 0;
Int32.TryParse(strNumber, out nTest);

Updated 19-Jan-11 22:43pm

I couldn’t figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

This issue is to ask approval to set RuleLevel for that analyzer as BuildWarning.

If I changed your highlighted example to:

List<object> x = new List<object>() { "hello" };
foreach (string s in x) { ... }

is that going to warn? If yes, I don’t think we should have this analyzer at all, nevermind what its default warning level is. It’s going to be incredibly noisy, I fear. If we do add it, it should at most be info by default. The statement «If explicit cast is intended, user should do it explicitly, so that intent is clear» highlights that this is really a rule about style.
cc: @mavasani

Copy link

Contributor

Author

I don’t think those cases are common (at least I didn’t encounter them)

They’re very common, especially when working with non-generic collection types. I tried the rule from your PR on dotnet/runtime, and it yielded 1199 warnings; I sampled them, and none of the ones I looked at were bugs.

I don’t think we should be adding this rule, or if others really think it’s impactful, it should default to a level of None.

Copy link

Contributor

Author

I tend to agree with @stephentoub. Given the high false positive rate, this rule if approved, should most likely be a disabled by default rule OR silent/hidden so it’s a refactoring.

Copy link

Contributor

Author

Proposed analyzer is targeted at cases where foreach will try to cast base class to it child

Right. I’m saying that such use is the minority, and when it does exist, the developer is using the foreach as a way to write the cast. In your example, if someone has:

var x = new List<IComparable>() { "hello" };
foreach (string s in x)
{
}

your rule is going to warn, but this is perfectly valid, and either they’re going to be forced to suppress the rule or change their code to the less concise / less readable:

var x = new List<IComparable>() { "hello" };
foreach (IComparable i in x)
{
    string s = (string)i;
}

I don’t see the benefit.

Copy link

Contributor

Author

Such code might be written by mistake.

So, why not write a rule that flags every explicit cast from a base type to a derived type? Maybe the developer made a mistake.

That’s essentially what this rule is proposing doing. (And obviously we’re not going to add such a rule

Copy link

Contributor

Author

I mean, if user has written such manually (B)a, it’s obviously doesn’t need to be flagged

I don’t see the difference. The developer is saying «given this collection, I would like to enumerate every element in it, referring to each as an xyz». Whether they write it as foreach (xyz item in collection) { ... } or foreach (object o in collection) { xyz item = (xyz)o; }, they’re saying the same thing. I think your objection is to this behavior of foreach, but that’s how foreach has always worked. And I don’t see how the latter form is any more explicit or intentional than the former form. You described refactoring a loop and accidentally not changing the foreach and getting an exception at runtime; such an issue is no less likely to happen if you’d written it with an explicit cast.

Copy link

Contributor

Author

I understand that this was done because foreach was created when generics didn’t exists, and it would be tedious to cast from object manually every time, but now we have generics.

I don’t believe analyzers we ship in the SDK are a place to retcon behaviors of the C# language.

Copy link

Contributor

Author

If there is case where we can know that something is always a runtime error then I agree that it is an excellent case for a potential analyzer. Unfortunately, I don’t see how this analyzer can ever be definitive in its recommendations.

As @stephentoub says the compiler already warns for cases that we know will cause runtime failures:

using System;
using System.Collections.Generic;

public class C {
    public void M() {
        var list = new List<string>() { "Hello World" };
        foreach (int i in list) // error CS0030: Cannot convert type 'string' to 'int'
        {
            Console.WriteLine(i);
        }
    }
}

My understanding of what is being proposed is that we now warn for this case as well:

using System;
using System.Collections.Generic;

public class C {
    public void M() {
        var list = new List<object>() { "Hello World" };
        foreach (int i in list) // New warning about casting object to int
        {
            Console.WriteLine(i);
        }
    }
}

I can understand the implicit cast in foreach can be surprising but that’s how the language works. Whenever you see foreach (int value in list) you should think for (int i = 0, value = 0; i < list.Count; value=(int)list[i], i++). If you do not want that to happen you can use var which will type the value as whatever it actually is instead of attempting to cast it.

You could require this pattern in your codebase so all casts are explicit:

using System;
using System.Collections.Generic;

public class C {
    public void M() {
        var list = new List<object>() { "Hello World" };
        foreach (var value in list) 
        {
            int i = (int)value;
        }
    }
}

Or this:

using System;
using System.Collections.Generic;
using System.Linq;

public class C {
    public void M() {
        var list = new List<object>() { "Hello World" };
        foreach (var i in list.Cast<int>()) 
        {
            
        }
    }
}

But this really becomes a style argument about what the best way to express to readers that you intend the cast that is happening.

Is there a guide somewhere to the philosophy of analyzers? I can see correctness-style analyzers falling under three general categories.

1. The code you’ve written is going to fail at runtime. Don’t even bother hitting F5. (See existing error code CS0030.)
2. The code you’ve written may succeed at runtime or it mail fail at runtime. Practically speaking, it will always* go down the «success» path or it will always go down the «failure» path, but we can’t really tell from static analysis which world we’re in. If you hit F5 you’ll find out quickly which side you’re on.
3. The code you’ve written may succeed or fail at runtime, but it will do so non-deterministically, or the variables which affect success vs. failure may be environment-dependent and very difficult to reason about. Your production server might observe different behaviors than your test server.

My opinion (and just my opinion) is that these are also in increasing order of importance for an analyzer to flag. [3] is the most important because it’s alerting you to a problem that you might not even know about until your prod server falls over unexpectedly. [1] and [2] are less important. We should flag them if we can, but if we miss something it’s not the end of the world, as the developer will immediately see any runtime failure as soon as they exercise this code path.

For [2], I’d rather err on the side of false negatives instead of false positives. Other posters in this thread described their concerns behind a false positive rate. On the other hand, the worst consequence of a false negative is that you see the failure the first time you run your app, and you spend a minute or two debugging it. That doesn’t seem all that bad, to be honest.

* I’m using «always» a bit liberally. That is, if your test cases all populate the List<object> with strings, and you assume this is representative of a typical scenario, then you «always» go down the success path for typical inputs. And if you happen to receive an atypical input, your implicit cast acts as a quasi Debug.Assert("I really expected this to be a string and haven't tested this code for other data types."). Score one for invariant checking!

Copy link

Contributor

Author

I do appreciate all the effort here, but let’s not add this to roslyn-analyzers. The concern here is about style, and it would be better suited to an analyzer suite focused on style, e.g. stylecop.

Copy link

Contributor

Author

I would prefer this rule to be in stylecop as it fits more into style.

Besides it warns when people use objects, now there is a chance that for cases like ISomeInterfaceHere upcasting to a class that implements said interface might not be a bug at all.

However it would be nice if foreach did have a way to pattern match instead of casting (like from ISomeInterfaceHere to MyClassIneheritingFromSomeInterfaceHere) instead of the cast.

However there is another issue:

• what if code that would not be using foreach pattern matching gets falsely detected as a pattern match by the compiler.
• other issues.

And yes the only places I can see foreach pattern matching of any use are:

• loading types from 1 assembly using reflection (after loading them using AppDomains or AssemblyLoadContexts)
• base classes -> implementing ones.
• etc.

However I do think pattern matching foreach would be great to add to avoid implicit casting inside of them (and makes the intent more clearer if one did not want to use var).

However in my code even on my own Plugin Loaders, using AppDomain or not, uses var on everything just because I prefer var for everything unless I explicitly cannot use var for something.

Also another note one can also do this if upcasting is intended as well too:

foreach (var something in somethings)
{
    // use pattern matching to determine the real types of the type of 'something'
    // (Interface or whatever) and do things based on whatever the type really is.
}

safeguards users from potential runtime issues

I don’t believe it does.

safeguards users from potential runtime issues

This same argument could be made for warning about all explicit casts. The developer may be new to C# and be unaware that the cast will cause runtime errors. My understanding of the problem as it happened is:

start with code like this

List<Student> People {get;}

// some levels of object-oriented layering later...

foreach(Student student in People)
{
	...
}

We then add a base type like say Person that Student inherits from and update our collection to contain the base type

List<Person> People {get;}

// some levels of object-oriented layering later...

foreach(Student student in People)
{
	...
}

the compiler doesn’t warn because this cast could succeed. There may even be paths were nothing happens and ones where there is a runtime exception. While I am sympathetic, I do not see a universal way to address this problem today. If I am an analyzer, the developer needs to communicate to me what their intent is. There are millions of lines of code that do this today and I shouldn’t warn them just because there exists some situations where this could be non-intentional.

If, in my codebase, this sort of refactoring is happening all the time I would personally use var in a lot of places. I might even recommend var be used everywhere. That was one of the reasons var became so common in the IDE codebase of roslyn, because we kept refactoring different layers and we didn’t want to deal with the fallout of updating thousands of variable declaration locations.

However, these sorts of refactors never happen at the compiler layer. For that reason the roslyn compiler layer does not use var at all.

This is the reason the style rules for https://github.com/dotnet/roslyn are what they are and why I think this analysis is best expressed in something like stylecop.

Is there a guide somewhere to the philosophy of analyzers? I can see correctness-style analyzers falling under three general categories.

1. The code you’ve written is going to fail at runtime. Don’t even bother hitting F5. (See existing error code CS0030.)
2. The code you’ve written may succeed at runtime or it mail fail at runtime. Practically speaking, it will always* go down the «success» path or it will always go down the «failure» path, but we can’t really tell from static analysis which world we’re in. If you hit F5 you’ll find out quickly which side you’re on.
3. The code you’ve written may succeed or fail at runtime, but it will do so non-deterministically, or the variables which affect success vs. failure may be environment-dependent and very difficult to reason about. Your production server might observe different behaviors than your test server.

Interestingly I do see things a little differently. For cases like [1] I am annoyed as a developer if the tooling could have told me something was wrong sooner but «wasted my time» by assuming I would figure it out eventually. In general, false positives are extremely detrimental to analysis being successful. If warnings are only sometimes correct it’s just another cognitive load on the developer. Now in addition to whatever they have on their plate they need to take the time to reason about all these diagnostics and decide if they are false positives. The urge to just silence all warnings goes up for every false positive that it encountered. I see these as being in priority order:

1. The code will compile and run without error, but will silently not do what you want (like CA2247 or passing async void delegates to something that takes an Action). roslyn#12649 is the most extreme example I can think of. These are the obvious slam dunks and should be on by default. Some of these will even come directly from the compiler instead of analyzers.
2. The code will fail at runtime 100% of the time. Also an obvious analyzer candidate as it makes my tools smarter and the amount of time I spend figuring out what went wrong is reduced. This is the category all the platform analyzers fall into for me.
3. The code you’ve written may succeed at runtime or it may fail at runtime. CA2015 is in this category. Here the question is «What is the probability if will fail at runtime vs. succeed?» AND «How many codebases have this pattern in them already?». This is the central metric I think we need to apply for everything after [0] and [1]. In the case of CA2015 there is a 99% chance the code will fail and do something horrific with a slim chance that it will be fine «just for debugging». It is also likely there are only a handful of codebases on earth that need to implement the MemoryManager<T> type.

Applying these principled to the rule proposed in this issue:

• What are the chances the code succeeds (What is the false positive rate)?
  • This issue will only present in code that hasn’t run, so newly written or modified code. Because of this it seems unlikely that any analysis warnings that appear in existing foreach loops will contain actual bugs. By this metric I would expect every single warning that appears in the dotnet/runtime repo to be a false positive. Again, because this issue is likely to appear in newly written or newly changed code and foreach loops are used a lot.
• How many code bases already have this pattern
  • Hard to say but everything we know says that var usage in foreach is about 50/50
  • foreach (var is used ~150K on github so lets say ~150K don’t use var in foreach. Thats a lot of code that is has been running for years without issue and would potentially be flagged.

Because this has a potential for a high false positive rate in a large number of codebases I think its a non-starter.