Suggestions on implicitly typed local variables

[shankyjain7243]

Fixes the following issue

Suggestions are shown on comparing the types from the left and the right hand variable types by using string comparison. The string comparison approach was taken because Type.GetType("int"); returns null and hence comparing CLR types did not seem possible.

[ironcev]

Hi @shankyjain7243,

Thanks a lot for implementing the feature! This is the first C# suggestion that wasn't developed by myself so I can't tell you how excited I am about this pull request! It's a milestone in the project history for sure, and actually, the second milestone done by you :-) Also, I am happy to see that you've added unit tests. So far I've hardly found time to properly cover the analysis with unit tests.

The pull request can be accepted as it is, but there are a few details that are worth changing. It will take me very less time to change them on my own after accepting the pull request but I think in that case I will be taking a good learning opportunity from you, especially if you are eager to continue working on Sharpen.

Also, since this is the first substantial pull request I would like to use it to extend the guidelines and to improve the issue definition format so that future contributions can be made more easy for the contributors.

Please let me know your thoughts about it. If you have time to polish the details I would gladly describe them. If not I will merge the pull request immediately and do the changes on my own.

I am looking forward to having this suggestion in the next Sharpen release. Let's have it out quickly :-)

Thanks a lot again!

Regards, Igor

[shankyjain7243]

Hi @ironcev ,

Please do let me know the proposed improvements to the pull request so that I can make the changes on my own. This is a good opportunity to actually know which all areas I missed so that I can polish my coding skills further. We can also identify more features in order to prepare for a release.

Best, Shanky

[ironcev]

Great @shankyjain7243 , let's do it that way.

Below are the remarks. Meanwhile I'll prepare another one C# suggestion related to the var keyword. Together with two features added from my side, this would be a very good content for the next release. Looking forward to it :-)

Comparing types

To answer your direct question on comparing types. As you've noticed by yourself, string comparison has its limitations and in a general case it will not work properly. Roslyn semantic API offer a method for getting type symbol for syntax nodes:

semanticModel.GetTypeInfo(node).Type

For existing usage example in Sharpen, search for semanticModel.GetTypeInfo.

Missing smoke tests

The smoke tests are missing and they are one of the requirements for C# suggestions. When I catch some time I'll explain in detail in the Wiki why smoke tests are so important and why we insist having them. I've also extended the issue description with the Acceptance Criteria which emphasize this. Please take a look.

Analysis algorithm (performance and consistency)

The current algorithm creates internally two lists, one for the variable declarations and one to keep the result. In general, we want to avoid creating additional objects, especially lists if it is not necessary. Please see the Implementation Considerations chapter that I've just added. If you take a look at all the oder analyzers you will see that they are implemented as LINQ expressions, means no additional lists are created to hold the result or intermediate nodes. To stay consistent with the other analyzers and to avoid creating unnecessary objects, the algorithm should be slightly refactored to remove the unnecessary two lists.

FYI I plan to add performance optimizations to all of the existing analyzers in the future. It will be much easier to roll out those optimizations to all the analyzers including this one that you wrote if they all follow the same structure.

Semantic of LanguageVersions

Unfortunately the code is not that throughly documented which sometimes can lead to the confusion. The LanguageVersions property denotes the language version in which a feature was implemented and not in which it is available. This is in 99% of the cases a single language version. Some very rare features are implemented across few language version, that's why we have this property as plural.

So

LanguageVersions { get; } = ImmutableArray.CreateRange(new[] { CSharp30 ,CSharp50, CSharp60, CSharp70 , CSharp71});

should actually be:

LanguageVersions { get; } = ImmutableArray.CreateRange(new[] { CSharp30 });

These are the semantical and performance oriented points worth to be done. There are some other slight points that are related for following the conventions. Here I can also comment, but I would suggest that you work on these topics first and then on your own take a look at your solution and compare it with existing equivalents. If you ask yourself "Does my contribution looks the same as the existing code? Does it anywhere sticks out?" you will most likely notice some convention that are not followed. (A hint :-) - take a look at commit messages, folder and file naming and structure).

Thanks a lot once again for you contribution :-) and please let me know if you have any additional questions.