Move C# editorconfig diagnostic support API to CompilationOptions, instead of SyntaxTree #44331

agocke · 2020-05-17T19:53:48Z

No description provided.

jasonmalinowski

Was looking to help @agocke understand what to do next for the test failures, and generally reviewed the IDE code carefully just to understand if there was any easy mistake that would explain it. Comments are what I noticed but the code looks solid.

jasonmalinowski · 2020-05-19T18:52:49Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCompiler.cs

            var tree = SyntaxFactory.ParseSyntaxTree(
                content,
                file.IsScript ? scriptParseOptions : parseOptions,
-                file.Path,
-                diagnosticOptions,


So any existing analyzers out there will just break if they were using the obsolete properties? I'm not terribly worried about it as the users are likely rare (or non-existent) but just wanted to confirm the intent since this will go through API review.

The intended breaking changes are:

If you added options to an analyzer config, they will not get propagated to the syntax trees. You can still access the property on the syntax tree but it is obsolete, and none of the options from the analyzer config files will be present.

If you added options to the trees directly, those will be preserved.

Eventually, we can consider removing the obsolete APIs all together.

src/Compilers/Core/Portable/Compilation/SyntaxTreeOptionsProvider.cs

jasonmalinowski · 2020-05-19T22:10:14Z

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

+            public override bool? IsGenerated(SyntaxTree tree)
+            {
+                var options = _lazyAnalyzerConfigSet
+                    .GetValue(CancellationToken.None).GetOptionsForSourcePath(tree.FilePath);


Any reason we shouldn't thread cancellation through the SyntaxTreeOptionsProvider?

Oversight. I'll fix.

Sorry, didn't fully understand the implications here -- now that I think about it, I think it's better not to do so, and the reason is because once you've supplied the provider to the compiler, there are a variety of operations which are simply uncancellable that will request the provider. I think it's best to leave it out because it incorrectly implies that the cancellation token is respected in the compiler when it won't be.

there are a variety of operations which are simply uncancellable that will request the provider

What operations would be uncancellable?

I think it's best to leave it out because it incorrectly implies that the cancellation token is respected in the compiler when it won't be.

Cancellation is always advisory so the compiler is allowed to do so. My concern is if/when this becomes a perf issue then we will have to do more API evilness to deal with that.

I'll address the cancellation concern in a separate pull request.

src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs

src/Compilers/Core/Portable/Compilation/SyntaxTreeOptionsProvider.cs

The diagnostic configurations produced by AnalyzerConfig processing currently go into the SyntaxTree.DiagnosticOptions. This unfortunately has performance problems because any change to the diagnostic options may force all syntax trees to be replaced. This change moves the diagnostic options produced from AnalyzerConfigs to the CompilationOptions, which can be replaced in a Compilation without replacing SyntaxTrees.

… value

…reeOptionsProvider

…t changing options If a Workspace has changes to .editorconfig files, that'll also cause changes to CompilationOptions. This excludes the changing SyntaxTreeOptionsProvider from something that the host has to deal with which maintains existing compatibility and also keeps things a bit simpler for callers.

…roviders around Even if we replace the CompilationOptions, we want to replace the SyntaxTreeOptionProvider so things are still consistent.

Testing for both recoverable and non-recoverables doesn't make sense anymore, and also removes a test that was asserting we carried data along if you replaced the root, but that's now entirely moot.

Reloading a project is now triggering work to happen because the CompilationOptions won't be identical after the reload: we'll have a new SyntaxTreeOptionsProvider which although would answer with the same values, won't have object identity in the way the tests would expect. These APIs aren't used in Visual Studio so I don't think it's a big deal.

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.ParsedSyntaxTree.cs

src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs

src/Compilers/CSharp/Test/Symbol/Compilation/CompilationAPITests.cs

333fred · 2020-07-24T17:51:06Z

Done review pass (commit 23)

…arning

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.cs

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.ParsedSyntaxTree.cs

chsienki · 2020-08-04T17:55:10Z

Any further movement on this?

333fred · 2020-08-05T16:53:19Z

@chsienki yes, I have unresolved comments.

jasonmalinowski · 2020-08-05T17:35:17Z

@chsienki Yep I'll be picking this back up today. Had some other fun issues that took priority last week.

333fred

LGTM (commit 31)

For reasons that aren't entirely clear, when we create a preview of a code fix, we remove and re-add documents that are being changed when we produce the PreviewWorkspace. This process had an bug which meant that the file path of an .editorconfig got dropped, so the resultant .editorconfig document path was null. This later caused a crash if the diagnostic engine, when processing the PreviewWorkspace's new solution, tried asking for a Compilation, since the null path would get passed to AnalyzerConfig.Parse(), and it would throw. This was exposed by dotnet#45076; until then if you had a preview of just an .editorconfig file, nothing would actually be analyzed and so nothing asked for the Compilation. The crash is "fixed" by dotnet#44331 in that asking for a Compilation no longer results in a call to AnalyzerConfig.Parse(); you have to ask for a Compilation and then do something with a semantic model. By luck, that doesn't happen in the common repro, but more obscure things absolutely could still fail. This is being tested by the integration test being added in dotnet#46639. We don't really have a good unit tests here as far as I can tell, but even then a unit test really doesn't validate any of the end-to-end here. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1162464

agocke force-pushed the compilationoptions-analyzerconfig branch 4 times, most recently from fcf3b64 to 314275e Compare May 17, 2020 22:31

jasonmalinowski reviewed May 19, 2020

View reviewed changes

agocke force-pushed the compilationoptions-analyzerconfig branch 2 times, most recently from 1eadb4a to c73239f Compare May 20, 2020 00:14

agocke marked this pull request as ready for review May 20, 2020 00:15

agocke requested review from a team as code owners May 20, 2020 00:15

agocke marked this pull request as draft May 20, 2020 00:15

agocke closed this May 20, 2020

agocke reopened this May 20, 2020

agocke marked this pull request as ready for review May 20, 2020 00:26

agocke marked this pull request as draft May 20, 2020 00:26

agocke force-pushed the compilationoptions-analyzerconfig branch from c73239f to 0d1b7bb Compare May 20, 2020 00:27

jaredpar reviewed May 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Compilation/SyntaxTreeOptionsProvider.cs Outdated Show resolved Hide resolved

jasonmalinowski mentioned this pull request Jun 8, 2020

Design issues for speeding up .editorconfig processing #44936

Closed

agocke changed the title ~~WIP~~ Move C# editorconfig diagnostic support API to CompilationOptions, instead of SyntaxTree Jun 16, 2020

agocke force-pushed the compilationoptions-analyzerconfig branch from 0d1b7bb to a9899b1 Compare June 16, 2020 22:17

jasonmalinowski added 4 commits June 16, 2020 19:00

Allow CompilationOptions.WithSyntaxTreeOptionsProvider to take a null…

7e5edc5

… value

Ensure that calling CompilationOptions.With* doesn't lose the SyntaxT…

9da4451

…reeOptionsProvider

Ensure Solution.WithProjectCompilationOptions keeps SyntaxTreeOptionP…

118aace

…roviders around Even if we replace the CompilationOptions, we want to replace the SyntaxTreeOptionProvider so things are still consistent.

jasonmalinowski assigned agocke and jasonmalinowski Jun 17, 2020

jasonmalinowski added 3 commits June 17, 2020 11:51

Adjust tests for .editorconfig no longer being on trees

d8993a3

Testing for both recoverable and non-recoverables doesn't make sense anymore, and also removes a test that was asserting we carried data along if you replaced the root, but that's now entirely moot.

Fix the TestFieldsForEqualsAndGetHashCode test

cdf22e5

333fred reviewed Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.ParsedSyntaxTree.cs Outdated Show resolved Hide resolved

333fred reviewed Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs Outdated Show resolved Hide resolved

333fred reviewed Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs Show resolved Hide resolved

333fred reviewed Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Compilation/CompilationAPITests.cs Outdated Show resolved Hide resolved

jasonmalinowski added 5 commits July 24, 2020 12:19

Fix whitespace

f5f3e1d

Fix up the PublicAPI.Unshipped.txt files and also one more analyzer w…

6dc3748

…arning

Include obsoletion message in the Obsolete attributes

17bdda0

Make all the obsoletions be explicitly error-causing

d2616ff

Simplify and inline local function

0c2abce

333fred reviewed Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.cs Show resolved Hide resolved

333fred reviewed Jul 24, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.ParsedSyntaxTree.cs Outdated Show resolved Hide resolved

jasonmalinowski added 3 commits August 5, 2020 17:38

Merge branch 'dotnet/master' into compilationoptions-analyzerconfig

f227479

Switch the obsoletions back to warnings

d83cd94

Move back compat overloads to the end of the file

3423b25

333fred approved these changes Aug 6, 2020

View reviewed changes

jasonmalinowski mentioned this pull request Aug 7, 2020

Fix crash in the preview of a code action that modified an .editorconfig #46644

Merged

jasonmalinowski merged commit 45f2aed into dotnet:master Aug 8, 2020

ghost added this to the Next milestone Aug 8, 2020

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

mavasani mentioned this pull request Aug 13, 2020

Editorconfig severity changes not respected #46784

Closed

This was referenced Aug 14, 2020

Add a cache for AnalyzerConfigSets #46740

Merged

Ensure the Compilation's options are changed on .editorconfig changes #46801

Merged

jasonmalinowski mentioned this pull request Aug 25, 2020

Typing extremely slow when editing .editorconfig file #41837

Closed

agocke deleted the compilationoptions-analyzerconfig branch August 25, 2020 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move C# editorconfig diagnostic support API to CompilationOptions, instead of SyntaxTree #44331

Move C# editorconfig diagnostic support API to CompilationOptions, instead of SyntaxTree #44331

agocke commented May 17, 2020

jasonmalinowski left a comment

jasonmalinowski May 19, 2020

agocke May 19, 2020 •

edited

Loading

jasonmalinowski May 19, 2020

agocke May 19, 2020

agocke Jul 1, 2020

jasonmalinowski Jul 1, 2020

jasonmalinowski Aug 8, 2020

333fred commented Jul 24, 2020

chsienki commented Aug 4, 2020

333fred commented Aug 5, 2020

jasonmalinowski commented Aug 5, 2020

333fred left a comment

Move C# editorconfig diagnostic support API to CompilationOptions, instead of SyntaxTree #44331

Move C# editorconfig diagnostic support API to CompilationOptions, instead of SyntaxTree #44331

Conversation

agocke commented May 17, 2020

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski May 19, 2020

Choose a reason for hiding this comment

agocke May 19, 2020 • edited Loading

Choose a reason for hiding this comment

jasonmalinowski May 19, 2020

Choose a reason for hiding this comment

agocke May 19, 2020

Choose a reason for hiding this comment

agocke Jul 1, 2020

Choose a reason for hiding this comment

jasonmalinowski Jul 1, 2020

Choose a reason for hiding this comment

jasonmalinowski Aug 8, 2020

Choose a reason for hiding this comment

333fred commented Jul 24, 2020

chsienki commented Aug 4, 2020

333fred commented Aug 5, 2020

jasonmalinowski commented Aug 5, 2020

333fred left a comment

Choose a reason for hiding this comment

agocke May 19, 2020 •

edited

Loading