Make MVC API analyzers detect undocumented status codes in conditional returns#66775
Open
KitKeen wants to merge 4 commits into
Open
Make MVC API analyzers detect undocumented status codes in conditional returns#66775KitKeen wants to merge 4 commits into
KitKeen wants to merge 4 commits into
Conversation
Contributor
|
Thanks for your PR, @KitKeen. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #33105.
Background
API1000/API1001already trigger on plain returns:…but the equivalent ternary form silently passes:
@pranavkm previously confirmed in the issue thread that the IOperation rewrite in #34020 did not add conditional handling and that a PR extending it would be welcome.
Change
ActualApiResponseMetadataFactory.InspectReturnOperationis taught to recurse into both branches of anIConditionalOperation(the operation that represents the C# ternarycond ? a : b). Each branch produces its ownActualApiResponseMetadataentry, mirroring the existing handling ofISwitchExpressionOperation.To support recursion without piggy-backing on the
ISwitchExpressionArmOperationparameter that previously did this job,InspectReturnOperationgains anIOperation? overrideReturnedValueparameter. It isnullfor the existing call sites, so their behavior is unchanged.The
WhenFalse: { } whenFalseguard makes the new branch a no-op when the conditional is anifstatement (whereWhenFalsecan be null) — only ternary expressions used as a return value reach this code.Nested conditionals fall out of the recursion naturally — a
x ? a : (y ? b : c)return produces three entries, one per leaf.Tests
ActualApiResponseMetadataFactoryTest: two new unit tests —InspectReturnExpression_ReadsBothBranchesOfConditionalandInspectReturnExpression_ReadsNestedConditional— backed by a new fixture fileTestFiles/.../InspectReturnExpressionTestsForConditionalExpression.csthat mirrors the existing switch fixture.ApiConventionAnalyzerIntegrationTest: new end-to-end testDiagnosticsAreReturned_ForActionsReturnedFromConditionalExpressionthat provesAPI1000now fires on the ternary form shown in the issue.Out of scope
Issue #33091 (expression-bodied methods) is referenced by the issue body but tracked separately. Expression-bodied methods already flow through
GetReturnStatementsvia the implicitIReturnOperationRoslyn emits, so the new conditional handling will also kick in for=> cond ? a : bbodies as a side effect, but adding a dedicated test for that case would require a different syntax-lookup helper and is left for #33091.