Add correctness check that only runs analyzers #62035

dibarbet · 2022-06-20T23:45:21Z

It isn't uncommon for me to push changes to a PR and then an hour later notice that the correctness build fails due to formatting errors. This splits out the analyzers run from correctness so that we can more quickly get analysis results without having to wait for the longest CI leg to complete.

I know @CyrusNajmabadi also hits this regularly

Youssef1313

Should -runAnalyzers:$true be removed from

roslyn/eng/test-build-correctness.ps1

Line 46 in 29a29d5

    
           Exec-Block { & (Join-Path $PSScriptRoot "build.ps1") -restore -build -bootstrap -bootstrapConfiguration:Debug -ci:$ci -runAnalyzers:$true -configuration:$configuration -pack -binaryLog -useGlobalNuGetCache:$false -warnAsError:$true -properties "/p:RoslynEnforceCodeStyle=true"}

?

Youssef1313 · 2022-06-21T05:28:44Z

azure-pipelines.yml

+      displayName: Build with analyzers
+      inputs:
+        filePath: eng/build.ps1
+        arguments: -restore -build -configuration Release -prepareMachine -ci -binaryLog -runAnalyzers:$true -warnAsError:$true -properties "/p:RoslynEnforceCodeStyle=true"


Do we need to build using both Debug and Release? I can imagine an analyzer reporting a diagnostic inside #if DEBUG that won't be caught if we're building in Release.

I don't think its worth running both to be honest, we've already cut down our CI a fair amount to reduce chances for flakiness. However I do see that we have wayyy more #if DEBUG than release ones, so I'm definitely open to running this in Debug instead.

The compiler uses debug pragmas a lot. On the IDE side we use significantly less (about 10% of what the compiler does) but we still use them. However, all of our Debug pragmas are additive so if we run the analyzer leg in Debug I think that should be all we need.

updated to use debug mode instead

dibarbet · 2022-06-21T18:28:41Z

Should -runAnalyzers:$true be removed from

roslyn/eng/test-build-correctness.ps1

Line 46 in 29a29d5

Exec-Block { & (Join-Path $PSScriptRoot "build.ps1") -restore -build -bootstrap -bootstrapConfiguration:Debug -ci:$ci -runAnalyzers:$true -configuration:$configuration -pack -binaryLog -useGlobalNuGetCache:$false -warnAsError:$true -properties "/p:RoslynEnforceCodeStyle=true"}

This is a question I also wanted to ask @jmarolf - I'm not sure if we want to execute analyzers in the bootstrap compiler to make sure there aren't analyzer bugs, so I left it in

jmarolf · 2022-06-21T18:30:53Z

This is a question I also wanted to ask @jmarolf - I'm not sure if we want to execute analyzers in the bootstrap compiler to make sure there aren't analyzer bugs, so I left it in

@dibarbet yeah, there are compiler assertions that get hit by running analyzers. The intent of this was to be a stress-test of the compiler so I think we should leave it in.

jaredpar · 2022-06-21T18:54:59Z

yeah, there are compiler assertions that get hit by running analyzers. The intent of this was to be a stress-test of the compiler so I think we should leave it in.

Yep this is the reason. Correctness is meant as a full stress test on "can we keep compiling Roslyn with these changes to the compiler".

RikkiGibson · 2022-06-21T20:51:11Z

azure-pipelines.yml

+      displayName: Build with analyzers
+      inputs:
+        filePath: eng/build.ps1
+        arguments: -restore -build -configuration Debug -prepareMachine -ci -binaryLog -runAnalyzers:$true -warnAsError:$true -properties "/p:RoslynEnforceCodeStyle=true"


So getting all analyzer diagnostics in the solution on the command line requires emitting, etc.? is that correct? Just wondered if there was an msbuild target or something equivalent to "run code analysis on solution."

someday when we are allowed to used github actions we could use: https://github.com/dotnet/code-analysis but for now I think this is the simplest approach.

RikkiGibson · 2022-06-21T23:15:41Z

It's slightly disconcerting that the build_analyzers leg is failing but the correctness_build leg is passing :)

dibarbet · 2022-06-21T23:23:46Z

It's slightly disconcerting that the build_analyzers leg is failing but the correctness_build leg is passing :)

It is, I am investigating. It appears as though the analyzers leg is reporting formatting errors on raw string literals. These formatting errors are not reported in the IDE and seem to not be reported in the correctness leg. I wonder if potentially the build with analyzers is using an outdated analyzer/compiler that doesn't support them? As best as I can tell the formatting seems ok (for example an error is reported here - https://github.com/dotnet/roslyn/blob/main/src/Compilers/Test/Core/BaseCompilerFeatureRequiredTests.cs#L29).

…nalyzers

RikkiGibson · 2022-06-22T00:08:41Z

We use the same version of the analyzers in both bootstrap and non bootstrap builds right? Is there possibly an API behavior difference in bootstrap that accounts for the formatting issue?

dibarbet · 2022-06-22T20:37:57Z

We use the same version of the analyzers in both bootstrap and non bootstrap builds right? Is there possibly an API behavior difference in bootstrap that accounts for the formatting issue?

I think this is what was happening - now that I've upgraded to newer versions of the analyzers / formatter, I see a new set of warnings that are the same in both the correctness and analyzers leg. I'm going to add fixes for those to the PR

azure-pipelines.yml

dibarbet · 2022-06-23T19:41:13Z

@RikkiGibson @dotnet/roslyn-compiler for compiler review - had to update to a new version of the analyzer / formatter which added new warnings.

RikkiGibson

Happy to take this, though would suggest we wait another week or two for the compiler's outstanding feature branches to merge back in. Don't want any formatting issues added in there to impede progress right now.

dibarbet · 2022-06-23T20:54:28Z

Happy to take this, though would suggest we wait another week or two for the compiler's outstanding feature branches to merge back in. Don't want any formatting issues added in there to impede progress right now.

Fine with me, just lmk when they are merged in so I can update this PR.

JoeRobich · 2022-07-25T21:59:39Z

/azp run roslyn-CI

azure-pipelines · 2022-07-25T21:59:50Z

Azure Pipelines failed to run 1 pipeline(s).

JoeRobich · 2022-07-25T22:28:30Z

/azp run roslyn-CI

azure-pipelines · 2022-07-25T22:28:41Z

Azure Pipelines failed to run 1 pipeline(s).

…nalyzers

dibarbet · 2022-07-28T23:04:14Z

@RikkiGibson is now a good time to merge this?

dibarbet · 2022-07-29T00:50:18Z

talked to @333fred offline since Rikki is OOF - going ahead with merge.

@jmarolf you should be able to pull this into yours now

ghost added the Area-IDE label Jun 20, 2022

dibarbet force-pushed the split_correctness_analyzers branch from 4937bf3 to 21e9027 Compare June 21, 2022 00:25

Youssef1313 reviewed Jun 21, 2022

View reviewed changes

runfoapp bot mentioned this pull request Jun 21, 2022

roslyn.visualstudio.newintegrationtests.csharp.csharperrorlistnetcore.errorsafterclosingfile is flaky #61368

Open

Add correctness check that only runs analyzers

68d1c05

dibarbet force-pushed the split_correctness_analyzers branch from 21e9027 to 68d1c05 Compare June 21, 2022 18:30

dibarbet marked this pull request as ready for review June 21, 2022 18:32

dibarbet requested a review from a team as a code owner June 21, 2022 18:32

Increase timeout

77ac388

dibarbet requested a review from RikkiGibson June 21, 2022 18:39

jmarolf approved these changes Jun 21, 2022

View reviewed changes

RikkiGibson reviewed Jun 21, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into split_correctness_a…

52c69bb

…nalyzers

Update analyzers / format version

eb39b1a

Fix new formatting warnings

b0dd4fa

dibarbet requested review from a team as code owners June 22, 2022 22:44

sharwell reviewed Jun 22, 2022

View reviewed changes

azure-pipelines.yml Outdated Show resolved Hide resolved

dibarbet and others added 3 commits June 22, 2022 17:07

Fix more warnings

9ce3240

Fix another batch of warnings

5bf9771

Bump timeout in case restore takes a while

8c8ae70

RikkiGibson approved these changes Jun 23, 2022

View reviewed changes

dibarbet added 2 commits July 25, 2022 15:33

Merge remote-tracking branch 'upstream/main' into split_correctness_a…

55957ae

…nalyzers

Fix windows checkout task name

b688a61

JoeRobich approved these changes Jul 26, 2022

View reviewed changes

dibarbet merged commit 0146540 into dotnet:main Jul 29, 2022

dibarbet deleted the split_correctness_analyzers branch July 29, 2022 00:50

ghost added this to the Next milestone Jul 29, 2022

allisonchou mentioned this pull request Aug 3, 2022

[Automated] PRs inserted in VS build main-32802.320 #63156

Closed

dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022

Add correctness check that only runs analyzers #62035

Add correctness check that only runs analyzers #62035

Uh oh!

Conversation

dibarbet commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

dibarbet Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

jmarolf Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

dibarbet Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

dibarbet commented Jun 21, 2022

Uh oh!

jmarolf commented Jun 21, 2022

Uh oh!

jaredpar commented Jun 21, 2022

Uh oh!

RikkiGibson Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

jmarolf Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Jun 21, 2022

Uh oh!

dibarbet commented Jun 21, 2022

Uh oh!

RikkiGibson commented Jun 22, 2022

Uh oh!

dibarbet commented Jun 22, 2022

Uh oh!

Uh oh!

dibarbet commented Jun 23, 2022

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

dibarbet commented Jun 23, 2022

Uh oh!

JoeRobich commented Jul 25, 2022

Uh oh!

azure-pipelines bot commented Jul 25, 2022

Uh oh!

JoeRobich commented Jul 25, 2022

Uh oh!

azure-pipelines bot commented Jul 25, 2022

Uh oh!

dibarbet commented Jul 28, 2022

Uh oh!

dibarbet commented Jul 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dibarbet commented Jun 20, 2022 •

edited

Loading