roslyn: prefer var not working as expected

Assuming the list above is the complete set of rules for determining if something is apparent, it seems the only problem I have is with generics. List and List<T> are not the same type, and there’s no question that T is not apparent under the current rules/examples above.

The rules for “apparent” should meet the following: The static type of var can be correctly stated by a reader by looking only at the initializing expression, without access to the rest of the file. I’m not sure the rules need a new configuration option, but we should consider updating the current rules if they fail to meet this.

ToList is considered a ‘conversion’ method. Because it’s “To” + Type-Name. So if you have methods like ToDateTime it’s apparent that that’s it’s producing a ‘DateTime’.

In this case there is ‘ToList’ which is producing a ‘List’. So the type is apparent and ‘var’ is used.

–

This behavior was present when the feature was added here: https://github.com/dotnet/roslyn/blob/a534500c8f7d473fbfdb72e4ede133ae99c55af8/src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpTypingStyleDiagnosticAnalyzerBase.State.cs#L191

–

I have used stylecop and resharper on and off over the years, and the general rule has always been if the type is somewhere to the right of the equals, use var (meaning it is apparent). If the type is not listed, use the explicit type.

In this case, the type is considered to be on the right of the equals (because it’s in ‘ToList’).

I tend to agree with the original complaint; I’ve never been able to make sense of the rules for “type is apparent”.

This feature really needs a revisit. You can see it’s been requested numerous times, and if you want a big example of why the current options aren’t good enough, the dotnet runtime repo (among others) enforces a code style policy that var is only allowed for new or explicit casting – no ToList or similar – which can’t be enforced through VS.

You could enforce it through a custom analyzer, but it wouldn’t participate in Ctrl+K, Ctrl+E or other code generating actions.

I think this comparison is flawed:

It’s the same reason why “ToDateTime” is clear enough, even though i technically don’t know which namespace “DateTime” is referring to. I don’t need the function to be “ToSystemDateTime”.

This essentially puts ‘containing namespace’ and ‘type argument’ in the same category. That seems arbitrary and, to me at least, surprising.

A big difference with the namespace-based example you give is that it is allowable (and indeed normal) not to namespace qualify in the case where you do make the variable type explicit. E.g., you can write this:

DateTime start = startStamp.ToDateTime();

Here, the explicit type name in the variable declaration looks identical to what appears after To on the right. Developers who routinely fully-namespace-qualify DateTime might get upset, but there are at least valid (and, apparently common) coding styles in which you don’t include the System. prefix.

Compare that with a List<T> example:

List<int> ns = Enumerable.Range(1, 10).ToList()

There’s a very clear difference here: the type name on the right (List<int>) does not appear in the initializer expression. Only part of it (List) does.

And unlike your DateTime example, this isn’t simply a matter of choice. Whereas with DateTime I have the option to leave off the System., I do not have any equivalent option to leave off the <int>. I cannot write this:

// Will not compile
List ns = Enumerable.Range(1, 10).ToList()

So the comparison seems weak. Although it’s true that in both cases, the text following To is not the full typename, the critical difference is that with DateTime, it is at least a permissible representation of the type name, whereas you don’t get to use List as a shorthand version of List<int>.

In any case, the whole argument seems to miss the point. Isn’t the heart of the matter this: why do people want to use an explicit type in in certain cases? Without surveying developers it’s impossible to say for sure, so I can only add my 2c, but as someone who wants var if and only if the type is apparent, it’s because when reviewing pull requests in a web browser (which is how I normally review them) I can’t mouse-over something to find out its real type.

Here’s the crux for me: I don’t like var because it just means “thing”, and I want to know what kind of thing; exactly the same logic applies if I have a collection—if all I have is var and ToList, that just means “things” and I want to know what kind of things.

So when the analyzer replaces List<int> xs = q.ToList(); with var xs = q.ToList();, I have gone from knowing that I have some ints, to knowing only that I have some things. This feels exactly equivalent from going from int x = Foo(); to var x = Foo(); in that I’ve gone from knowing that I have an int to knowing only that I have a thing.

To narrow it down further: I want to know what sort of thing the code is dealing with. So when it comes to List<int> the int part of that is actually more important than the List part. So saying that ToList makes the type explicit ignores the more important of the two types.

I actually find it hard to imagine why anyone would want int x but var xs in these examples. I understand reasonable people can disagree over the merits of var vs explicit typing, but I find it hard to think of any consistent position in which you’d want explicit typing most of the time, but wouldn’t want to know the types of your things if they happen to be in collections that were returned by methods, and even then only if the method was called ToList. I’m genuinely curious to know: is there any data to suggest that there are people who (like me) want int x in the singular case, but who (unlike me) want var xs in the plural case.

It feels like the “var only when type is apparent” behaviour was implemented by someone who prefers var everywhere and doesn’t really understand why some people want the behaviour in question.

I’d love to see a coherent argument for why this:

int x = GetNumber();
List<int> xs = GetNumbers();
var zs = ys.ToList();

is in any way internally consistent.

The type is nowhere on this line:

var listOfStrings = myString.Split(',').ToList()

It as, as per rule ‘6’ on the list.

I could just as easily create my own poorly named extension method called ToCollection() that returned List<>:

In this case, the type would not be apparent. Because ‘Collection’ and ‘List’ are not the same types. If your method did return a Collection though, then it would be. ToBlah is a very common pattern in the .Net ecosystem to say: i’m converting what i have to an instance of ‘Blah’.

Yes, the type is List, but it is obscured in the method name.

Apparent is in the eye of the behold. You think it is not apparent, i think it is. When i see IConvertible.ToDateTime, it’s totally clear to me that that’s returning a DateTime and i don’t need the type name there.

Similarly, someone may think an as Blah at the end of a long, complex, multi-line expression is ‘obscured’. But others may not.

I am requesting that unless there is a type on the line, var not be used, so it would stay as:

Would it be potentially better to just disable this Roslyn rule and use whatever other tools you want here to control this scenario? It would keep Roslyn from having to reverse engineer whatever rules other tools have, and from having to keep those in sync as other tools change over time.

Since the proposal for csharp_style_var_when_type_is_explicit was rejected, i’d like to take another option to teh IDE design team on how we can address this space. Specifically, we will continue to have csharp_style_var_when_type_is_apparent, however, we will augment that option with sub-options like so:

csharp_style_var_when_type_is_apparent_for_default_expression

In other words, we would have csharp_style_var_when_type_is_apparent_for_XXX where XXX would be a knob for each sub option that could be controlled. We could then doc these (and their logic) and allow users to opt out of cases they do not like. This would also give us a mechanism to add in new apparent cases in the future without it being very problematic for users. For example, with the new lambda work we’re doing for C# 10, we could say that lambdas with explicit parameter and return types have an apparent type. e.g. string (int) => ... has the apparent type Func<int, string> since the int and string are explicitly spelled out. However, if someone doesn’t like that (because Func was not explicitly stated), then they could disable this.

This would also address the problem we have when we’ve been trying to come up with a set of rules that people can agree on as even in this thread (and other linked threads) there have been disagreements from the community on what tehy think is apparent.

If we went this route, the set of options would be like so (remember that this is the XXX in csharp_style_var_when_type_is_apparent_for_XXX):

name	example	notes
default_expression	default(string)
literal_expression	0	We could also have an option for each literal type (bool/numeric/string/etc.)
object_creation_expression	new List<int>	not for implicit creation new()
array_creation_expression	new int[0]	not sure about implicit creation new[] {...}. If all elements were apparent would the array type be apparent?
cast_expression	(int)...
as_expression	... as int
is_expression	... is int	always apparent this is bool
tuple_expression	(new List<int> ..., (int)...)	only apparent if all element types are apparent
static_factory_method	XElement.Parse	Only allowed for static method that returns exact same type as containing type, and containing type is specified
conversion_method	.ToDateTime()	name_only or name_and_type_arguments. name_only means just the name of the type being returned needs to match the name after To. name_and_type_arguments means you’d need something like .ToList<int> to match a return type of List<int>.

As above, with this approach, we could also add more items in the future depending on how we and the community feel. For example, we’ve had requests to match things like var v = componentModel.GetService<IFileWatcher>(). Here we could add a new option like instance_factory_method (or whatever) and then add opt-out/in support for these cases.

For a lot of the complaints we have we could then either have those users switch the setting to false. Or, for cases like conversion_method they could specify name_and_type_arguments to deal with the generics case.

I’m hoping this is a best of all worlds approach. It does not introduce a totally new option with a disparate overlap with the existing option. It doesn’t change the existing option for codebases that use it. It provides flexibility for codebases out there that don’t like our defaults. And it gives us an upgrade path to allow us to add stuff in the future safely with an out for customers who may not like those decisions.

The problem was: there is a group of people that do not always want ‘var’.

Indeed, and as a member of that group, I feel you have misunderstood what we wanted. I might be wrong of course: if it turns out that I’m the one out on a limb here, and that the majority of my fellow slightly-var-averse (or more accurately, type-concealment-averse) types are actually happy with this, then fine. What I was hoping is that you would say that you have canvassed opinion on this and found that the majority of people nagging you for this var-only-when-non-information-hiding behavior do actually prefer the behaviour that we ended up with. (And TBH, at that point it’s largely a matter of intellectual curiosity for me as to why other people have a different view. It’s easier to be philosophical about ending up on the losing side of a decision if you can at least comprehend the other point of view. But right now I don’t even know what the other point of view is. And maybe I could learn from it—perhaps if I understood why anyone would want the current behaviour, I might come round to it. I reserve the right to get smarter.)

But based on the information I currently have, it looks like this feature has failed to understand the goals of the very group it was supposed to support. Certainly, I’ve not yet heard from anyone who both a) wants “var if and only if the type is evident without it” and b) feels that the current behaviour does what they want. Maybe this table will help explain why I’m sceptical:

Developer Type	a)	b)
var-happy	No	Don’t care
Fully-var-averse	No	Don’t care
Likes var only when it avoids duplication	Yes	No
???	Yes	Yes

You address the needs of developers in the first two rows perfectly: people who want var everywhere are fully catered for, as are people who will only tolerate var in scenarios where it is completely unavoidable (anonymous types). I’m in the 3rd row, as is everyone I know who prefers the setting we’re discussing. But you have designed for row 4. I am as yet unconvinced that there is anyone in row 4.

By the way, a common feature of the people I know to be in row 3 is that they do a lot of code reviews. So another way to describe the requirement is “Don’t force me to do type inference in my head during code reviews.”

I believe this feature itself has been around about 3 years now and this is the first piece of feedback about this.

I thought this came in with code style enforcement, which was a VS 2017 feature wasn’t it? So I think it’s more like one and a half years. And for whatever reason, I only discovered this rather surprising ToList behaviour recently—I complained the moment I became aware of it. So apparently it’s possible to use VS for a year and a half without coming across this problem. And at my current company, there has been a relatively recent process of migrating away from Resharper because all the new analyzers that have gradually been appearing in VS (e.g. “Convert implicit type to explicit type (C#)” which only appeared in 15.7) have meant that there’s less and less incentive to use Resharper. So not everyone leapt onto this feature the minute it appeared last year.

Indeed, IIRC, the feedback we’ve gotten in the past is about ‘apparent’ not showing up in more places.

I am also a member of this group. This is not mutually exclusive with thinking that the behaviour being discussed in this issue is undesirable. (In fact, it’s presumably only those of us who have reasons to be selective about use of var who are making the complaints you describe: nobody in rows 1 or 2 of the table above will be having these issues.)

E.g., with something like var logger = serviceProvider.GetRequiredService<ILogger>(); I consider the type ILogger to be apparent, and I think that’s a case you currently don’t handle as ‘apparent’?

So it would be a mistake to interpret “We are hearing feedback asking for us to report more cases as apparent” as “There are no scenarios in which we overestimate the apparentness”. In an ideal world,

One possible outcome of this discussion is: there is no evidence that row 4 in that table is a non-empty set, so maybe the current design is wrong, but most people don’t care about this, so it’s not a priority. (Or, more pithily: Wrong, but won’t fix.) In which case, can I ask: is there a way to plug your own analyzers and fixers into VS in a way that code generation uses their rules? I did actually write my own analyzer and fixer back in 2016, because as far I was able to tell, this problem didn’t have a solution 3 years ago. (It is available at https://www.nuget.org/packages/Flyntax.AvoidVar/ and it not only gets this right, for my definition of right, but it also identifies a wide range of additional cases as ‘apparent’.) But the problem with it is that it didn’t participate in any code generation. (Although this was more of a problem with https://www.nuget.org/packages/Flyntax.StoreCtorArg/ tbh.)

The big benefit of the VS Code Style Enforcement is that all these settings get honoured by code gen (unlike anything you attempt to enforce through analyzers). Is there a way for us to write analyzers and code fixes that get involved during code gen? If there is, then I’ll happily update my existing analyzer which has the benefit of enabling me to define “right” however I please.

(Ideally, I’d also like some good way of managing settings for code style analyzers. I’d love to resurrect https://resharper-plugins.jetbrains.com/packages/OrderUsings/ as a Roslyn-based tool, but it currently relies on Resharper’s settings management system, something for which there is apparently no corollary for VS analyzers and code fixes. And it too needs to get hooked into code gen scenarios to be usable.)

I tend to agree with the original complaint; I’ve never been able to make sense of the rules for “type is apparent”.

The rules are that the type ‘Foo’ is apparent if we see:

1. new Foo
2. new Foo[]
3. (Foo)...
4. ... as Foo
5. Foo.StaticMethodThatReturnsAFoo(...). i.e. XElement.Parse
6. ....ToFoo(...). i.e. Convert.ToDateTime, blah.ToList(),

Those are considered ‘apparent’. Having there be a doc somewhere that mentions this would probably be good.

This:

the vast majority of cases, people felt it was apparent

is not as conclusive as you might think. You might have fallen into a statistical trap. Let me make up some data that fits your description, and yet which points in the exact opposite direction from your conclusion.

When you asked those questions, did you record which of the answers came from people who prefer var in the first place, and which came from people who think var obscures things? E.g. might it have looked like this?

preference	Apparent	Not apparent
var	1000	20
explicit type	1	100

In this (completely made up) poll, 1001 of the 1121 people asked said they thought the type was apparent—that’s 89%, “the vast majority” as you put it.

However, of the people who said they thought that it was apparent, 99.9% of them have absolutely no interest in using this feature. It seems unhelpful to allow their opinion to sway the design of a feature they’re not going to use.

If you just look at the people who would actually want to use this feature in this (completely made up) poll, you find that 99% of those people (the ones who will actually use this feature, and care about how it works) think it’s not obvious.

So in summary both of these (seemingly contradicting) points are true:

• the vast majority of those surveyed thought the type was apparent
• the vast majority of those surveyed who will actually use this feature thought the type was not apparent

Again, made up data. But not implausible. If you’re in the category of people who prefer the nominal type to be explicit, then you obviously have a different point of view on how much type names tell you from someone who’s perfectly happy using var everywhere, and so you are highly likely to have a different view on whether the type is apparent in these cases. My point is that your observation, that you examined this “by actually bringing up these cases to people and asking if they thought the type was apparent from context” could well have pointed you in entirely the wrong direction. If you don’t have a breakdown of the kind above, then you can’t really conclude anything about how the feature should work from the fact that most of the people you asked felt the type was apparent. You need to know what the majority of people who might use the feature think.

And the fact that people who like var everywhere substantially outnumber those who don’t means that looking at the answer to this question in aggregate is going to drown out the views of the explicit-type-preferers with the views of the var-preferers, which in the case of a feature designed for var-preferers, is exactly the wrong thing to do.

(If you do have that breakdown and can demonstrate that even amongst just those of us who prefer the full type name to be visible somewhere, the “vast majority” think the type is apparent, then I’ll be utterly amazed but will have to submit to the data, although I’ll find it hard to shake off the suspicion that you might not actually have shown anyone an example that really illustrates the problem when asking the question.)

BTW, the code that drove me here today was this:

var successResponses = operation
    .Responses
    .Where(r => r.Key == "default" || (r.Key.Length == 3 && r.Key[0] == '2'))
    .ToList();

While it may be apparent that successResponses is a List<...something...>, I don’t believe you will be able to tell me what that ...something... is from looking at this code alone. I don’t believe it is apparent. (And even if you scan up the code to discover that operation is an OpenApiOperation, and you can thank me for making that possible by not using var where that variable was declared, I submit that unless you happen to be extremely familiar with the library I happen to be using, it’s still not remotely apparent what the item type is in this list.)

I fully understand that you can’t really change the existing behaviour. But please please please I would love some sort of way to switch on a feature that prevents var from being recommended in cases where there are non-apparent generic type arguments.

The rule #6, …ToFoo(…). i.e. Convert.ToDateTime, blah.ToList(), seems to me to be in direct violation of https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions “Don’t assume the type is clear from a method name.”.

It seems strange to me to have a style analyzer with a default, non-configurable rule that is in direct violation of the documented conventions.

Also, regardless of the previous user poll - it just doesn’t make sense with generics to assume that the type is clear from

var foo = MyMethodOrWhatEverExpression.ToList(); // We’re assuming the type is clear from the method name and that it’s non-generic.

In actual fact, foo is of type List<Tuple<string, int>> for example. How is that clear!?

In view of this, either update your documented conventions to state “DO assume the type is clear from a method name” AND limit it to non-generic types, or scratch rule #6.

Honestly if you just fix ToList, ToDictionary and ToHashSet I will shut up about this forever 😃

There is a difference between these three cases:

var foo1 = bar.ToString();
var foo2 = bar.ToList();
var foo3 = bar.ToNullable();

In the first case, the type is apparent in the conversion method name. In the latter two cases, the conversion method only identifies the container type. It’s interesting to know what type the container is, but I would argue that it’s of secondary importance. The greater half of the type information is hidden. The fact that the hidden type happens to be a generic type parameter is an implementation detail. If a dedicated method like ToStringList where used, everyone would be happy. This whole argument seems to revolve around the concept that the container type is the only relevant type information, as if the distinction between generic types and non-generic types is irrelevant. This distinction seems to be fundamental in the way types are treated in much of the rest of the compiler and tooling, why not here? It’s clearly a problem for a number of people. A feature flag (e.g., in .editorconfig or as an analyzer rule) seems like an obvious solution.

Wouldn’t it be easier to simply make the 6 rules configurable in .editorconfig?

This entire discussion would have been over months ago.

Without getting into the individual rules, I can definitely see why some people disagree that unknownType.ToList() is the same as unknownType.ToList<Int32>()

It was mentioned above that:

If the current approach is not satisfactory, then it’s definitely encouraged and suitable to go and write your own alternative that has ‘var/types’ operating as you would like them to behave.

And yet,

In which case, can I ask: is there a way to plug your own analyzers and fixers into VS in a way that code generation uses their rules?

Not currently. But you could definitely open an issue asking for that 😃

Based on that, I’d say roslyn has failed in its mission if we’re telling people to go write their own alternatives for a system that hasn’t been put in place, instead of adding a few config flags to an .editorconfig.

I’d personally support more vars around tuples.

But i assume that wouldn’t apply to:

... x = (florb(), bippity.boppity.Boo());

Right?

In this case, the type of the tuple would be completely unclear and not at all explicit from the RHS. So this would have to have a spelled out type right?

We’re only discussing the times when at least the type of the tuple seems moderately clear from it’s constructor, right?

(Just trying to make sure i’m on the same page as you. It’s been made totally clear to me that my gut feelings on what is ‘explicit/apparent’ doesn’t necessarily align with others. so i’m trying hard to not assume and to make sure i’m hearing things properly). Thanks!

Your comment about existing code is totally valid, this is why another mode for this option may be a good idea as suggested.

As for the MessageProcessor/XElement.Parse example which I consider separate and much less impactful issue that I wouldn’t bother to report if it wasn’t for the similar but different ToList issue here is my logic: I think of the apparent var option like this - if I am looking at the code in notepad do I 100% know the type of the var? If yes - suggest var, if not - suggest explicit type. I now know how the tool works but when I encountered it for the first time I thought it might be a bug. I still feel the need to hover with my cursor over the method to check if the type is right when I see a code like this. Who knows what editorconfig options the code uses. Also what about when reading code on GitHub? This opinion of mine is probably too extreme but it advocates for very consistent behavior with no exceptions. As I said it doesn’t bother me nearly as much as the ToList issue which literally made the option unusable for me. While I thought about building an analyzer as you suggested I decided that it is not worth it because what I really want is to share this option via editorconfig.

I have always been strong proponent of this type of var usage. It seems like there are very few of us and I have yet to meet someone who thinks the current behavior is preferred. Everyone who says the current behavior is preferred seems to be in the “always var” camp to begin with. This particular behavior was the reason I setup my editor config to not report these although I want it badly but this is deal breaker. I’d rather scream at my team when I see it than have lists of invisible types all over my code. If a lot of people who use the option (i.e. they are not “always var” people) disagree then as @MarioAtTrimble suggest there is need for another option.

Also this hurts “I believe this feature itself has been around about 3 years now and this is the first piece of feedback about this”… Yeah my user voice feedback from 20 months ago which even had a reasonable number of upvotes was ignored. The very same migrated user voice item linked above was submitted by me.

Assuming the list above is the complete set of rules for determining if something is apparent, it seems the only problem I have is with generics. List and List<T> are not the same type, and there’s no question that T is not apparent under the current rules/examples above.

I believe this was discussed at hte time and the feeling was that the type was apparent. i.e. the critical bit was knowing you had a List, not the complete instantiation of that type. It’s the same reason why “ToDateTime” is clear enough, even though i technically don’t know which namespace “DateTime” is referring to. I don’t need the function to be “ToSystemDateTime”.

but we should consider updating the current rules if they fail to meet this.

I’m definitely worried about us going that route. Because that would make different versions of Roslyn incompatible with itself. i.e. a team could have different versions of VS and now have them fighting each other. Similarly if you had this be an error, and then upgraded, we might now take a whole bunch of code and say: this is wrong, you must use an explicit type. It’s unclear to me if that would upset more people than the amount of people it makes happy.

If you really are going to argue this, please give us another option, something like ‘Use var only when type is present on line’