Update the simplifier to respect and apply the 'use var' user preference. by CyrusNajmabadi · Pull Request #25865 · dotnet/roslyn

CyrusNajmabadi · 2018-03-31T23:55:01Z

Note: this PR looks large, but that's primarily due to moving some "use var" functionality down from teh CSharpFeatures layer to the CSharpWorkspace layer. This was done so that the C# simplification service could appropriately simplify code like Dictionary<int, string> d = ... to var d = ... if that matched the user's style preference.

Unfortunately, the classes used for this functionality were not in their own files. They were sibling classes to the "use var/explicit-type' analyzers", and thus show up as huge deletes/adds even though they were barely touched.

CyrusNajmabadi · 2018-03-31T23:57:05Z

Tagging @kuhlenh @jinujoseph @dotnet/roslyn-ide

CyrusNajmabadi · 2018-03-31T23:57:28Z

Taggign @heejaechang @mavasani as i used IOperation primarily for this.

alrz · 2018-04-01T02:03:58Z

would be nice to also recognize

if (c) M(1);
else M(2);

M(c ? 1 : 2);

CyrusNajmabadi · 2018-04-01T04:10:26Z

@alrz Not sure how i feel about that one. If it were to be done, it would be a separate feature IMO.

CyrusNajmabadi · 2018-04-01T05:33:54Z

...eatures/CSharp/Portable/Diagnostics/Analyzers/CSharpTypeStyleDiagnosticAnalyzerBase.State.cs

-{
-    internal abstract partial class CSharpTypeStyleDiagnosticAnalyzerBase
-    {
-        internal class State


This type was moved down to the CSharpWorkspace layer and renamed to CSharpTypeStyleContext

CyrusNajmabadi · 2018-04-01T05:34:17Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpTypeStyleDiagnosticAnalyzerBase.cs


 namespace Microsoft.CodeAnalysis.CSharp.Diagnostics.TypeStyle
 {
-    internal abstract partial class CSharpTypeStyleHelper


This type moved down to CSharpWorkspace

CyrusNajmabadi · 2018-04-01T05:34:30Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseExplicitTypeDiagnosticAnalyzer.cs


 namespace Microsoft.CodeAnalysis.CSharp.Diagnostics.TypeStyle
 {
-    internal sealed class CSharpUseExplicitTypeHelper : CSharpTypeStyleHelper


This type moved down to CSharpWorkspace

CyrusNajmabadi · 2018-04-01T05:34:42Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseImplicitTypeDiagnosticAnalyzer.cs


 namespace Microsoft.CodeAnalysis.CSharp.Diagnostics.TypeStyle
 {
-    internal sealed class CSharpUseImplicitTypeHelper : CSharpTypeStyleHelper


This type moved down to CSharpWorkspace

CyrusNajmabadi · 2018-04-01T05:36:08Z

src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs

                return false;
            }

-            // If the simpleName is the type of the Variable Declaration Syntax belonging to LocalDeclaration, For Statement or Using statement


all this code was removed. We now defer to CSharpTypeStypeHelper to do the logic. This way we're entirely in sync with the analyzer/refactoring.

CyrusNajmabadi · 2018-04-01T05:36:41Z

src/Workspaces/CSharp/Portable/Utilities/CSharpTypeStyleDiagnosticAnalyzerBase.State.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.Utilities
+{
+    internal class CSharpTypeStyleContext


This was moved down from CSharpFeatures. Other than renaming it, it was not changed.

CyrusNajmabadi · 2018-04-01T05:36:57Z

src/Workspaces/CSharp/Portable/Utilities/CSharpTypeStyleHelper.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.Utilities
+{
+    internal abstract partial class CSharpTypeStyleHelper


This was moved down from CSharpFeatures. Other than renaming it, and making it a singleton, it was not changed.

CyrusNajmabadi · 2018-04-01T05:37:07Z

src/Workspaces/CSharp/Portable/Utilities/CSharpUseExplicitTypeHelper.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.Utilities
+{
+    internal sealed class CSharpUseExplicitTypeHelper : CSharpTypeStyleHelper


This was moved down from CSharpFeatures. Other than renaming it, and making it a singleton, it was not changed.

CyrusNajmabadi · 2018-04-01T05:37:45Z

src/Workspaces/CSharp/Portable/Utilities/CSharpUseImplicitTypeHelper.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.Utilities
+{
+    internal sealed class CSharpUseImplicitTypeHelper : CSharpTypeStyleHelper


This was moved down from CSharpFeatures. Other than renaming it, and making it a singleton, it was not changed.

feature.

CyrusNajmabadi · 2018-04-18T19:00:18Z

Tagging @dotnet/roslyn-ide @jcouv @jmarolf . I've now split this PR out from #26236

This PR is now only the change to move the "use var" logic down to the workspace layer so it can be part of hte simplifier logic. This is highly beneficial and it means that features do not have to always figure out "should i use 'var' or not". Instead, as long as they either generate a type with the Simplifier annotation on it (which the core Workspace code generation helpers do), then the Simplifier will convert to 'var' if that's what the user wants and it's legal to do so.

CyrusNajmabadi · 2018-04-18T19:00:26Z

Also tagging @DustinCampbell

CyrusNajmabadi · 2018-04-18T19:01:09Z

src/EditorFeatures/Test2/Simplification/TypeNameSimplifierTest.vb

+{
+    void Main()
+    {
+        {|Simplify:int|} i = 0;


these cases were already being tested in this file, but as part of one large test. I kept hte large test, but also broke out individual cases to make things easier to debug when things go wrong.

CyrusNajmabadi · 2018-04-18T19:01:44Z

src/Features/CSharp/Portable/CSharpFeaturesResources.Designer.cs

        }

-         /// <summary>
+        /// <summary>


no idea why the code was bad before. but my VS fixed it.

CyrusNajmabadi · 2018-04-18T19:03:38Z

src/Features/Core/Portable/FeaturesResources.Designer.cs

+            get {
                return ResourceManager.GetString("Alias_ambiguous_type_0", resourceCulture);
            }
        }


someone manually edited this file in teh past. my VS keeps fixing it, so i'm just going to merge this in since it's a PITA to have to undo this file every time.

CyrusNajmabadi · 2018-04-18T19:04:33Z

src/Workspaces/CSharp/Portable/CodeStyle/TypeStyle/TypeStyleHelper.cs

@@ -306,7 +304,7 @@ public static TypeSyntax GetTypeExpression(
            }


this whole helper can get removed in the future. features should just ask "type.GenerateTypeSyntax()" and they should get the right behavior.

CyrusNajmabadi · 2018-04-18T19:05:23Z

src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs

-            }
-
-            // If the simpleName is the type of the Variable Declaration Syntax belonging to LocalDeclaration, For Statement or Using statement
-            if (simpleName.IsParentKind(SyntaxKind.VariableDeclaration) &&


we had duplicated a bunch of "can i use var" logic here. It was both broken and incomplete. Now, we just defer to the CSharpUseImplicitTypeHelper which contains all the correct logic and is the place we should always be updating.

CyrusNajmabadi · 2018-04-18T19:06:52Z

src/Workspaces/Core/Portable/Simplification/SimplificationOptions.cs

        /// </summary>
-        public static Option<bool> PreferImplicitTypeInLocalDeclaration { get; } = new Option<bool>(nameof(SimplificationOptions), nameof(PreferImplicitTypeInLocalDeclaration), false,
+        public static Option<bool> PreferImplicitTypeInLocalDeclaration { get; } = new Option<bool>(nameof(SimplificationOptions), nameof(PreferImplicitTypeInLocalDeclaration), true,
            storageLocations: new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.PreferImplicitTypeInLocalDeclaration"));


this is an option that is not actually exposed to the user in any way. I'm not sure why we actually have it.

…ressoin

mavasani · 2018-04-19T17:55:59Z

src/Workspaces/CSharp/Portable/Utilities/CSharpTypeStyleHelper.cs

+        /// things quickly, even if it's going against their stated style.
+        /// </summary>
+        public readonly bool IsStylePreferred;
+        public readonly DiagnosticSeverity Severity;


This code moved down. I didn't want to change it. I also don't see the value in an auto-prop over a readonly field.

mavasani · 2018-04-19T18:07:49Z

src/Workspaces/CSharp/Portable/Utilities/CSharpTypeStyleHelper.cs

+        /// one exception is the refactoring, which is explicitly there to still let people convert
+        /// things quickly, even if it's going against their stated style.
+        /// </summary>
+        public readonly bool IsStylePreferred;


The doc comment is very helpful, but the naming IsStylePreferred still seems a bit confusing. I would have assumed that AnalyzeTypeName(typeSyntax).IsStylePreferred would tell me if the current type style of typeSyntax is preferred, but the current implementation of the flag is opposite. Should this be renamed to IsConvertedStylePreferred with current semantics or to IsExistingTypeStylePreferred with negated semantics?

This code moved down. I didn't want to change it. :) I can change the name if you really want though.

mavasani · 2018-04-19T18:14:18Z

src/Workspaces/CSharp/Portable/Utilities/CSharpUseExplicitTypeHelper.cs

+        {
+        }
+
+        protected override bool IsStylePreferred(


Seems like the result CSharpUseExplicitTypeHelper.IsStylePreferred is the negation of CSharpUseImplicitTypeHelper.IsStylePreferred. Can we share code?

All this code moved down. I did not want to change it. But i can if you think that's important. let me know.

Ah, I didn't realize that. In that case, I wouldn't do any rename or functionality changes as part of this PR. We can do it later. Thanks.

CyrusNajmabadi · 2018-04-19T18:47:34Z

Can this be merged in? Thanks!

heejaechang · 2018-04-19T18:50:52Z

retest windows_debug_vs-integration_prtest please

CyrusNajmabadi · 2018-04-19T23:08:40Z

@heejaechang @mavasani All tests passed. Can this be merged in? Thanks!

CyrusNajmabadi · 2018-04-19T23:17:28Z

Thanks!

DustinCampbell

Thanks @CyrusNajmabadi!

CyrusNajmabadi added 5 commits March 31, 2018 13:52

Initial UseConditional work.

514c920

Initial UseConditional work.

2a38b93

Working replacement.

7f82f3a

Extract common code.

96cd0ab

Make 'var' work properly in the simplifier service.

1561214

CyrusNajmabadi requested a review from a team as a code owner March 31, 2018 23:55

Move types to their own files.

78a5872

CyrusNajmabadi added 4 commits March 31, 2018 21:33

Provide a way to create types that are not simplified to 'var'.

ec650b9

Update test options.

05b6fb0

Update test.

11371db

Move CSharp code style code down to the Workspace layer.

919ab7a

CyrusNajmabadi commented Apr 1, 2018

View reviewed changes

CyrusNajmabadi added 6 commits March 31, 2018 22:38

Rename config value.

3f294cd

Add ui options.

40437e7

Add VB ui options.

3a6ce03

Add VB support

f5e476d

Strip trivia

4076b69

Initial stubs for 'use conditional for returns'

bfd91b2

CyrusNajmabadi added 2 commits April 18, 2018 11:11

Merge branch 'master' into useConditionalExpressoin

72df410

Remove 'Use Conditional Expressoin'

ac19888

CyrusNajmabadi changed the title ~~Add features to offer using conditional expressions ?: over explicit if-statement flows.~~ Update the simplifier to respect and apply the 'use var' user preference. Apr 18, 2018

CyrusNajmabadi added 3 commits April 18, 2018 11:49

Remove more vestiges of

ccc0406

feature.

Remove code.

386c43d

Add back newline.

a6e1cf9

CyrusNajmabadi mentioned this pull request Apr 18, 2018

Add features to offer using conditional expressions ?: over explicit if-statement flows. #26236

Merged

4 tasks

CyrusNajmabadi commented Apr 18, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into useConditionalExp…

7d86930

…ressoin

mavasani reviewed Apr 19, 2018

View reviewed changes

mavasani approved these changes Apr 19, 2018

View reviewed changes

heejaechang approved these changes Apr 19, 2018

View reviewed changes

heejaechang merged commit 06393e4 into dotnet:master Apr 19, 2018

CyrusNajmabadi deleted the useConditionalExpressoin branch April 19, 2018 23:17

DustinCampbell reviewed Apr 20, 2018

View reviewed changes

CyrusNajmabadi mentioned this pull request May 2, 2018

Add features to offer using conditional expressions ?: over explicit if-statement flows. (squashed) #26572

Closed

4 tasks

CyrusNajmabadi mentioned this pull request May 17, 2018

Incorrect "use var" suggestion in a foreach loop #26923

Closed

		@@ -306,7 +304,7 @@ public static TypeSyntax GetTypeExpression(
		}

Conversation

CyrusNajmabadi commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Mar 31, 2018

Uh oh!

CyrusNajmabadi commented Mar 31, 2018

Uh oh!

alrz commented Apr 1, 2018

Uh oh!

CyrusNajmabadi commented Apr 1, 2018

Uh oh!

CyrusNajmabadi Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 18, 2018

Uh oh!

CyrusNajmabadi commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 19, 2018

Uh oh!

heejaechang commented Apr 19, 2018

Uh oh!

CyrusNajmabadi commented Apr 19, 2018

Uh oh!

CyrusNajmabadi commented Apr 19, 2018

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

CyrusNajmabadi commented Mar 31, 2018 •

edited

Loading

CyrusNajmabadi Apr 1, 2018 •

edited

Loading

CyrusNajmabadi Apr 1, 2018 •

edited

Loading

CyrusNajmabadi Apr 1, 2018 •

edited

Loading

mavasani Apr 19, 2018 •

edited

Loading