[WIP] Port ConvertSwitchStatementToExpression analyzer/fixer to shared layer by mavasani · Pull Request #41815 · dotnet/roslyn

mavasani · 2020-02-20T15:18:04Z

Extracted out of #41363 so it only contains the following changes:

Port analyzer/fixer/test files for ConvertSwitchStatementToExpression to shared layer
Options related namespace changes (see comment Port few IDE analyzers/fixers to shared layer to be enabled in CodeStyle NuGet package #41363 (comment) for details)
MEF based language service discovery in CodeStyle layer. Follow up items after PR 41363 #41462 tracks replacing this with a non-MEF based approach.
Minimal resx/xlf file changes - few were needed as the resources used by analyzer/fixer were moved to shared resx file.
Enable .editorconfig based unit test support for CodeStyle layer. NOTE: First commit b006324 just ports changes from Always try to register the EditorConfigDocumentOptionsProvider #40513 which are required to enable this support. This commit should become redundant once that PR is merged in.

mavasani · 2020-02-20T15:19:53Z

src/Analyzers/Core/Analyzers/Helpers/AnalyzerHelper.cs

Moving existing code into shared layer

mavasani · 2020-02-20T15:21:42Z

src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs

Code moved to partial declaration which was moved to shared layer.

Extracted out of dotnet#41363 so it only contains the following changes: 1. Port analyzer/fixer/test files for ConvertSwitchStatementToExpression to shared layer 2. Options related namespace changes (see comment dotnet#41363 (comment) for details) 3. MEF based language service discovery in CodeStyle layer. dotnet#41462 tracks replacing this with a non-MEF based approach. 4. Minimal resx/xlf file changes - few were needed as the resources used by analyzer/fixer were moved to shared resx file. 5. Enable .editorconfig based unit test support for CodeStyle layer. NOTE: First commit dotnet@b006324 just ports changes from dotnet#40513 which are required to enable this support. This commit should become redundant once that PR is merged in.

mavasani · 2020-02-20T19:18:35Z

Unit test failures are due to #41823

CyrusNajmabadi · 2020-02-23T17:16:00Z

Compilers.sln

+		{E7BC93F8-51F0-45A8-872D-86C387243D38} = {B20208C3-D3A6-4020-A274-6BE3786D29FB}
 	EndGlobalSection
 	GlobalSection(ExtensibilityGlobals) = postSolution
 		SolutionGuid = {6F599E08-A9EA-4FAA-897F-5D824B0210E6}


why is all this changing?

CyrusNajmabadi · 2020-02-23T17:16:48Z

src/Analyzers/Core/Analyzers/AbstractCodeQualityDiagnosticAnalyzer.cs

+    // Consider moving all the CodeQuality diagnostic analyzers into analyzer repo as CA rules.
    internal abstract class AbstractCodeQualityDiagnosticAnalyzer : DiagnosticAnalyzer, IBuiltInAnalyzer
    {
+        // Diagnostics in CodeStyle layer should be warnings by default.


this feels very hacky.

CyrusNajmabadi · 2020-02-23T17:17:59Z

src/CodeStyle/Core/Analyzers/Formatting/Formatter.cs

+    /// This utility class is only to be used by the formatting analyzer and formatting code fixer in Code Style layer.
+    /// Other components should use the public "Formatter" type defined in "Microsoft.CodeAnalysis.Formatting" namespace.
+    /// </remarks>
    internal static class Formatter


i have to admit this is getting very confusing.

i also don't like all the duplication in the code below... is this all realy necessary?

CyrusNajmabadi · 2020-02-23T17:19:08Z

src/CodeStyle/Core/CodeFixes/Host/Mef/CodeStyleHostLanguageServices.cs

+
+        public static CodeStyleHostLanguageServices? GetMappedCodeStyleLanguageServices(HostLanguageServices? hostLanguageServices)
+            => hostLanguageServices != null ? s_mappedLanguageServices.GetOrAdd(hostLanguageServices, Create) : null;
+        public static CodeStyleHostLanguageServices GetRequiredMappedCodeStyleLanguageServices(HostLanguageServices hostLanguageServices)


Suggested change

public static CodeStyleHostLanguageServices GetRequiredMappedCodeStyleLanguageServices(HostLanguageServices hostLanguageServices)

public static CodeStyleHostLanguageServices GetRequiredMappedCodeStyleLanguageServices(HostLanguageServices hostLanguageServices)

CyrusNajmabadi · 2020-02-23T17:19:16Z

src/CodeStyle/Core/CodeFixes/Host/Mef/CodeStyleHostLanguageServices.cs

+            => hostLanguageServices != null ? s_mappedLanguageServices.GetOrAdd(hostLanguageServices, Create) : null;
+        public static CodeStyleHostLanguageServices GetRequiredMappedCodeStyleLanguageServices(HostLanguageServices hostLanguageServices)
+            => s_mappedLanguageServices.GetOrAdd(hostLanguageServices, Create);
+        private static CodeStyleHostLanguageServices Create(HostLanguageServices hostLanguageServices)


Suggested change

private static CodeStyleHostLanguageServices Create(HostLanguageServices hostLanguageServices)

private static CodeStyleHostLanguageServices Create(HostLanguageServices hostLanguageServices)

CyrusNajmabadi · 2020-02-23T17:20:12Z

src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2+Test.cs

+            {
+                get
+                {
+#if CODE_STYLE


i really don't like how infectious this seems to be. it feels like we're becoming a c++ codebase with ifdefs everywhere to support conditional compilation.

CyrusNajmabadi · 2020-02-23T17:21:03Z

src/EditorFeatures/TestUtilities/CodeActions/CodeFixVerifierHelper.cs

+            return solution;
+        }
+
+#if CODE_STYLE


like... this literally feels like entirely separate approaches here. having us use ifdefs instead of other mechanisms (i.e. interfaces/DI/etc.) just feels really hacky and hard to maintain.

CyrusNajmabadi · 2020-02-23T17:21:43Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

+                return;
+            }
+
+            _optionService.RegisterDocumentOptionsProvider(documentOptionsProvider);


what's going on in this file?

CyrusNajmabadi · 2020-02-23T17:22:34Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs

+#if CODE_STYLE
+using Microsoft.CodeAnalysis.Internal.Options;
+
+namespace Microsoft.CodeAnalysis.CSharp.Internal.CodeStyle


this is super confusing to read and understand. i also just am hating the ifdef shenanigans.

mavasani · 2020-02-23T18:08:07Z

@CyrusNajmabadi Please hold off reviewing this PR for now. @sharwell is changing options infrastructure to avoid requiring any conditional preprocessor directives in shared layer.

CyrusNajmabadi · 2020-02-23T18:11:58Z

@CyrusNajmabadi Please hold off reviewing this PR for now. @sharwell is changing options infrastructure to avoid requiring any conditional preprocessor directives in shared layer.

Hooray! :)

mavasani · 2020-03-04T22:34:02Z

Superceded by #42171

Port changes from dotnet#40513

b006324

mavasani requested review from a team, CyrusNajmabadi, sharwell and tmat February 20, 2020 15:18

mavasani mentioned this pull request Feb 20, 2020

Port few IDE analyzers/fixers to shared layer to be enabled in CodeStyle NuGet package #41363

Closed

mavasani commented Feb 20, 2020

View reviewed changes

src/Analyzers/Core/Analyzers/Helpers/AnalyzerHelper.cs Outdated

Copy link

Contributor Author

mavasani Feb 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving existing code into shared layer

mavasani commented Feb 20, 2020

View reviewed changes

mavasani force-pushed the PortConvertSwitch branch from e20527f to d545499 Compare February 20, 2020 16:22

mavasani added 4 commits February 20, 2020 12:15

Port additional change from dotnet#40513

8200921

Attempt a workaround for integration test failures

9dd6665

Fix determinism build

a9dea72

Merge remote-tracking branch 'upstream/master' into PortConvertSwitch

221c57c

CyrusNajmabadi reviewed Feb 23, 2020

View reviewed changes

mavasani changed the title ~~Port ConvertSwitchStatementToExpression analyzer/fixer to shared layer~~ [WIP] Port ConvertSwitchStatementToExpression analyzer/fixer to shared layer Feb 23, 2020

mavasani added the Blocked label Feb 23, 2020

mavasani closed this Mar 4, 2020

	public static CodeStyleHostLanguageServices GetRequiredMappedCodeStyleLanguageServices(HostLanguageServices hostLanguageServices)

	public static CodeStyleHostLanguageServices GetRequiredMappedCodeStyleLanguageServices(HostLanguageServices hostLanguageServices)

	private static CodeStyleHostLanguageServices Create(HostLanguageServices hostLanguageServices)

	private static CodeStyleHostLanguageServices Create(HostLanguageServices hostLanguageServices)

Conversation

mavasani commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani commented Feb 20, 2020

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 commented Feb 23, 2020

Uh oh!

CyrusNajmabadi commented Feb 23, 2020

Uh oh!

mavasani commented Mar 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants