Fix DetectMisplacedLambdaAttribute analyzer and tests #44061

halter73 · 2022-09-19T16:33:36Z

Description

Before this change, we had a whole suite of analyzers that were not being tested on the CI.

I just happened to be looking at these analyzers because we started working on new out-of-band analyzers for the hackathon. When running the tests locally, I discovered failing tests because of a false positive in our DetectMisplacedLambdaAttribute logic. That's when I realized these tests must not be running on the CI.

Customer Impact

Customers haven't noticed this. Below, the [Authorize] attribute is in the correct location, but we improperly warn anyway.

But we've noticed that the false positive warning does not always show up in the error list. It appears to be due to the race condition fixed by dotnet/roslyn#6322. We think this might be why no customers have reported this issue. The false positive happens to be in a fairly specific case where an attribute is added to a lambda that then immediately invokes another method.

Regression?

Yes
No

Tests haven't been running on the CI, so the regression likely happened sometime between preview1 when the analyzer was first released and now.

Risk

High
Medium
Low

I think it would be higher risk to continue running no tests for these analyzers in our release/7.0 branch.

Verification

Manual (required)
Automated

Packaging changes reviewed?

Yes
No
N/A

captainsafia · 2022-09-19T17:02:52Z

src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/DetectMisplacedLambdaAttribute.cs

+
+        static IMethodSymbol? GetReturnedInvocation(IBlockOperation blockOperation)
+        {
+            foreach (var op in blockOperation.ChildOperations.Reverse())


Why do we search the child operations in reverse here?

I figured it made sense to search from the end of the method for return operations instead of the start because I'd expect most returns to be nearer to the end. Multiple return operations are an edge case this analyzer never really accounted for anyway.

eng/Build.props

halter73 · 2022-09-20T00:05:06Z

I want to manually see if the analyzer is broken in our rc2 nightlies before undrafting this considering this PR targets release/7.0. If the analyzers just don't run rather than emit false positives, it might not be worth fixing. If we are emitting false positives like it was in the tests, we should probably send this through tactics.

halter73 · 2022-09-20T18:48:25Z

There appears to be a real issue with analyzer creating some false positives, but we think it has not been reported because of dotnet/roslyn#63225. Below, the [Authorize] attribute is in the correct location, but we improperly warn anyway.

We've noticed that the false positive warning does not always show up in the error list:

I've also noticed that our tests run with an outdated version of Microsoft.CodeAnalysis.CSharp compared to VS 17.4.0 Preview 2.0. Our tests run against 4.200.22.12801 and VS 17.4.0 Preview 2.0 loads 4.400.22.43014. Is this expected because it needs to work with 17.3 or something? Are we worried about incompatibilities? @dotnet/aspnet-build @jaredpar

@dotnet/minimal-apis What do we think about bringing this to tactics for rc2 or GA?

ghost · 2022-09-20T18:49:08Z

Hi @halter73. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

jaredpar · 2022-09-20T19:33:05Z

Are we worried about incompatibilities?

There shouldn't be any incompatibilities I'm aware of.

halter73 · 2022-09-21T18:08:36Z

@dotnet/aspnet-admins This has been approved by tactics.

dougbu

Undo changes to Build.props please

halter73 · 2022-09-21T23:36:10Z

@dougbu The point of the Build.props change was to run the tests that were previously not being run on the CI. This is the most important part of the fix, so I cannot just undo it.

I've updated it to only add Microsoft.AspNetCore.App.Analyzers.Test.csproj to Build.props rather than adding all the projects in the AspNetCoreAnalyzers directory since the non-test projects are already included by Microsoft.AspNetCore.App.Ref.csproj. Does this address your concern?

dougbu · 2022-09-22T00:41:21Z

Does this address your concern?

Yes. I'll enable auto-merge (squash).

@SteveSandersonMS CircuitGracefulTerminationTests.ClosingTheBrowserWindow_GracefullyDisconnects_TheCurrentCircuit is failing in this attempt and I'm sure (a) that's not related to @halter73's change and (b) I've seen the same aspnet-components-e2e failure in other PRs and maybe rolling builds. Could you please quarantine this test❔

halter73 · 2022-09-22T20:06:21Z

/azp run

azure-pipelines · 2022-09-22T20:06:39Z

Azure Pipelines successfully started running 3 pipeline(s).

* Fix DetectMisplacedLambdaAttribute analyzer and tests * Add AspNetCoreAnalyzers to Build.props * Update eng/Build.props

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 19, 2022

halter73 added 2 commits September 19, 2022 09:50

Fix DetectMisplacedLambdaAttribute analyzer and tests

53b5952

Add AspNetCoreAnalyzers to Build.props

ccfd9ab

halter73 force-pushed the halter73/7.0/fix-analyzers branch from c9f7a9b to ccfd9ab Compare September 19, 2022 16:51

captainsafia approved these changes Sep 19, 2022

View reviewed changes

halter73 marked this pull request as ready for review September 19, 2022 23:58

halter73 requested review from a team, Pilchie, dougbu and wtgodbe as code owners September 19, 2022 23:58

halter73 marked this pull request as draft September 20, 2022 00:00

halter73 marked this pull request as ready for review September 20, 2022 18:48

halter73 added the Servicing-consider Shiproom approval is required for the issue label Sep 20, 2022

halter73 added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 21, 2022

dougbu suggested changes Sep 21, 2022

View reviewed changes

Update eng/Build.props

0b4c616

dougbu approved these changes Sep 22, 2022

View reviewed changes

dougbu enabled auto-merge (squash) September 22, 2022 00:41

dougbu merged commit 5195e68 into release/7.0 Sep 23, 2022

dougbu deleted the halter73/7.0/fix-analyzers branch September 23, 2022 01:04

JamesNK pushed a commit that referenced this pull request Sep 23, 2022

Fix DetectMisplacedLambdaAttribute analyzer and tests (#44061)

f512aa5

* Fix DetectMisplacedLambdaAttribute analyzer and tests * Add AspNetCoreAnalyzers to Build.props * Update eng/Build.props

JamesNK mentioned this pull request Sep 23, 2022

Add header dictionary analyzer and fixer #44123

Merged

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Fix DetectMisplacedLambdaAttribute analyzer and tests #44061

Fix DetectMisplacedLambdaAttribute analyzer and tests #44061

Uh oh!

Conversation

halter73 commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Customer Impact

Regression?

Risk

Verification

Packaging changes reviewed?

Uh oh!

captainsafia Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

halter73 Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halter73 commented Sep 20, 2022

Uh oh!

halter73 commented Sep 20, 2022

Uh oh!

ghost commented Sep 20, 2022

Uh oh!

jaredpar commented Sep 20, 2022

Uh oh!

halter73 commented Sep 21, 2022

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

halter73 commented Sep 21, 2022

Uh oh!

dougbu commented Sep 22, 2022

Uh oh!

halter73 commented Sep 22, 2022

Uh oh!

azure-pipelines bot commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

halter73 commented Sep 19, 2022 •

edited

Loading