Formatting analyzer #28873

sharwell · 2018-07-27T17:15:32Z

No description provided.

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

src/Features/Core/Portable/Formatting/AbstractFormattingDiagnosticAnalyzer.cs

src/Compilers/CSharp/CSharpAnalyzerDriver/CSharpDeclarationComputer.cs

src/Features/Core/Portable/Formatting/AbstractFormattingDiagnosticAnalyzer.cs

CyrusNajmabadi · 2018-07-27T18:32:06Z

Right now, it's hard to tell the effect this would have due to the noisiness of the "trailing whitespace" diagnostics. would it be possible to not have those, and then see what sort of impact this would normally be expected to have?

src/Features/Core/Portable/Formatting/AbstractFormattingDiagnosticAnalyzer.cs

CyrusNajmabadi · 2018-07-27T18:58:18Z

Suppress IDE0051 in Microsoft.CodeAnalysis.VisualBasic

why are we not just suppressing at the project level... having a million line-suppressions just means we have so much noise that interferes with seeing what actually benefits here.

sharwell · 2018-07-27T19:04:44Z

why are we not just suppressing at the project level... having a million line-suppressions just means we have so much noise that interferes with seeing what actually benefits here.

I'm on a bug hunt, looking for bugs in the formatter and bugs in the Suppress Message feature. Turns out this analyzer is quite good at revealing both.

heejaechang · 2018-07-27T19:07:15Z

if a fix for this analyzer will fix one diagnostic, then I think reporting diagnostic per change seems right. if a fix for this analyzer will fix whole document, then I think reporting diagnostic per document seems right.

CyrusNajmabadi · 2018-07-27T19:09:07Z

if a fix for this analyzer will fix one diagnostic, then I think reporting diagnostic per change seems right. if a fix for this analyzer will fix whole document, then I think reporting diagnostic per document seems right.

Agreed. That's a good way to think about it.

sharwell · 2018-07-27T20:06:06Z

if a fix for this analyzer will fix one diagnostic, then I think reporting diagnostic per change seems right. if a fix for this analyzer will fix whole document, then I think reporting diagnostic per document seems right.

I'm leaning towards implementing the simple fix as fixing all squiggles on the line where invoked, and the Fix All in Document runs FormatAsync.

src/EditorFeatures/CSharpTest/Formatting/FormattingAnalyzerTests.cs

CyrusNajmabadi · 2018-07-29T21:28:03Z

@CyrusNajmabadi Is there a way to validate diagnostic spans? The current test is passing if I change the line the following:

There may be. I'm not certain. Look at the base classes and their public/protected helpers.

I would have expected a test failure because the markup span does not match the reported diagnostic span.

I agree. The original test harness validated this. However, when the rewrite of all the implementations of these analyzers changed, these were changed to be less strict in what they checked (i think to make the porting over easier). But it means they're less effective in terms of validating the experience is working properly.

If you wanted to tighten the check, that would be fine with me. I expect you'd have to go update a whole swath of tests. But i think that would be totally fine to do (and i'd be willing to review for you).

I'm also interested in how I can test multiple diagnostics reported in a single input, such as the following:

You'd have to write a helper for that.

sharwell · 2018-08-07T17:45:39Z

src/Workspaces/Core/Portable/Formatting/Formatter.cs

+            options = options.WithChangedOption(FormattingOptions.AllowConcurrent, false);
+            var resultTask = GetFormattedTextChangesAsync(node, workspace, options, cancellationToken);
+            Debug.Assert(resultTask.IsCompleted);
+            return resultTask.WaitAndGetResult_CanCallOnBackground(cancellationToken);


@jasonmalinowski Please make sure to review the new pattern in this method.

This seems like this deserves a comment. Is there a concern here about blocking a thread on a parallelized algorithm?

the one who calls this API should set the option, not the API itself set the option.

Is there a concern here about blocking a thread on a parallelized algorithm?

Yes, the analyzer runs in parallel on the thread pool. It would be easy for it to starve the system.

the one who calls this API should set the option

The asynchronous form of this method provides opportunistic parallelization, if desired.

So, i'm somewhat ok with this change. But it would def need to be applied a bit uniformly. We could also consider deprecating this guy. It always worries me when we expose sync entrypoints that immediately block on the async version.

It should also be a warning to us for future API development. It's likely we'll need to default things to being async so we don't end up hurting ourselves here later. A good example of that is around the discussions of Options. Options present a synchronous API, but need to block on async work. If we then make Options async, it virally hits entrypoints like this (which are currently fine being synchronous).

If we had settled on these being async from teh start, we wouldn't have these problems cropping up over time as more core parts of our system become async.

But it would def need to be applied a bit uniformly.

Implementing this for the rest of the synchronous Formatter methods. It's so ugly it makes me sad but implementing the synchronous form all-the-way-down was a huge amount of work and mostly duplicated code.

so the thing i'm missing is: how do you know the returned task iscomplete?

oh, i see. because you pass AllowConcurrent: false. gotcha.

Note: my preference wouldjust be deprecate these methods as we do this.

The diagnostic analyzer code paths are synchronous, and rely on the new synchronous behavior of these methods to avoid thread pool starvation in projects with very large files.

src/Features/Core/Portable/FeaturesResources.resx

src/Features/Core/Portable/Formatting/FormattingCodeFixProvider.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs

src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/FixAllContext.DiagnosticProvider.cs

mavasani

The core analyzer/fixer changes LGTM.

I would still request adding a unit test to guard the reflection based code and also let someone from @dotnet/roslyn-infrastructure review the changes to Packages.props.

src/CodeStyle/CSharp/Tests/FormattingAnalyzerTests.cs

mavasani · 2018-10-11T20:09:28Z

src/CodeStyle/Core/CodeFixes/FormattingCodeFixProvider.cs

+        {
+            OptionSet options = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);
+
+            // The in-IDE workspace supports .editorconfig without special handling. However, the AdhocWorkspace used


Is this a test-only code? If so, can it be refactored into a test-only method?

It's not strictly test-only code. It's addressing a limitation of the workspace which would naturally go away once we switch the code to using the new .editorconfig support in the compiler.

It could be considered test-only code from the perspective that removing the code would not produce a failure inside the IDE, but it could still produce failures in tools like AnalyzerRunner.

jaredpar · 2018-10-12T14:56:42Z

src/CodeStyle/Core/CodeFixes/AbstractFormattingAnalyzer.cs

+            analyzer.InitializeWorker(context);
+        }
+
+        private static Assembly HandleAssemblyResolve(object sender, ResolveEventArgs args)


Where is the bug link tracking the removal of this? Can't imagine this is part of the long term solution here.

➡️ #30472

jaredpar · 2018-10-12T14:59:59Z

src/CodeStyle/CSharp/Analyzers/Microsoft.CodeAnalysis.CSharp.CodeStyle.csproj

@@ -8,16 +8,17 @@
    <RootNamespace>Microsoft.CodeAnalysis.CSharp</RootNamespace>
    <TargetFramework>netstandard1.3</TargetFramework>
  </PropertyGroup>
+  <ItemGroup>
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisCSharpFixedVersion)" />


Why are we referencing a package here instead of the corresponding source project? Feel like we need a good explanation here. This is extremely odd and I'm not going to be the last person confused by this.

I'm interested in understanding this too. If there's a good reason, it's not obvious.

All the hacks for this package exist so we can get feedback from users about the behavior of this analyzer in developer and CI scenarios. To accomplish that, it is installable in and works with 15.7 plus.

jaredpar

Signing off on the infrastructure side of the change. The core followup item is #30472

jinujoseph · 2018-10-15T08:09:33Z

test windows_release_vs-integration_prtest

jinujoseph · 2018-10-15T08:10:17Z

Approved to merge when ready

jinujoseph · 2018-10-15T23:57:26Z

test windows_release_vs-integration_prtest

MarcoRossignoli · 2018-10-16T17:13:31Z

Hi guys!
This analyzer will be published as a nuget package?
If so when?

sharwell · 2018-10-16T18:44:51Z

@MarcoRossignoli I moved your comment to #30541 👍

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 27, 2018

sharwell requested review from a team as code owners July 27, 2018 17:15

sharwell commented Jul 27, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Outdated Show resolved Hide resolved

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Outdated Show resolved Hide resolved

jcouv added Area-IDE Area-Compilers labels Jul 27, 2018

heejaechang reviewed Jul 27, 2018

View reviewed changes