Skip to content

Add analyzer for detecting mismatched endpoint parameter optionality #36154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Sep 10, 2021
Merged

Add analyzer for detecting mismatched endpoint parameter optionality #36154

merged 5 commits into from
Sep 10, 2021
+776 −38

Conversation

captainsafia
Copy link
Member

Fixes #34553.

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 3, 2021
@captainsafia captainsafia reopened this Sep 3, 2021
@captainsafia captainsafia marked this pull request as ready for review September 7, 2021 18:34
@captainsafia captainsafia requested review from dougbu, Pilchie and a team as code owners September 7, 2021 18:34
@captainsafia captainsafia requested review from pranavkm and removed request for a team, Pilchie and dougbu September 7, 2021 18:35
@captainsafia captainsafia added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Sep 7, 2021
pranavkm
pranavkm reviewed Sep 8, 2021
View reviewed changes
src/Framework/Analyzer/src/DelegateEndpoints/DetectMismatchedParameterOptionality.Fixer.cs Outdated

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public partial class DelegateEndpointFixer : CodeFixProvider
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to split this across two different files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the pattern we currently use for analyzers because I think it makes sense. One main file where all the definitions are registered and then each codefix in its own file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's far less likely we would re-use a single codefix for all of these scenarios. The analyzers were useful to share because they were detecting the same thing - viz calls to MapGet. It's less common to want to do that with the codefix. In addition, code fixes aren't on a hot path

src/Framework/Analyzer/test/MinimalActions/DetectMismatchedParameterOptionalityTest.cs Outdated

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public partial class DetectMismatchedParameterOptionalityTest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add unit tests for the tokenizer e.g. : https://github.com/dotnet/aspnetcore/pull/35732/files#diff-742e27fb1eae81bcb0bfd8113ccc306c0add53e7ee7c13b85d9c1917c974b9b5R14-R34?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah forgot we talked about this! I coped over the test cases here and included a few interesting ones from https://github.com/dotnet/aspnetcore/blob/b46c5d5e15bdc6e4babd5194e0a1723d3e9cfc46/src/Http/Routing/test/UnitTests/Template/TemplateParserTests.cs

@captainsafia captainsafia force-pushed the safia/optionality-analyzer branch from 58aea48 to d93e1c1 Compare September 9, 2021 16:20
@captainsafia captainsafia force-pushed the safia/optionality-analyzer branch from d93e1c1 to 1ab519c Compare September 9, 2021 16:23
pranavkm
pranavkm approved these changes Sep 9, 2021
View reviewed changes
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Could we make the one change to separate out analyzers and codefixes to separate assembly before getting this merged?

src/Framework/Analyzer/src/DelegateEndpoints/DetectMismatchedParameterOptionality.Fixer.cs Outdated

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public partial class DelegateEndpointFixer : CodeFixProvider
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's far less likely we would re-use a single codefix for all of these scenarios. The analyzers were useful to share because they were detecting the same thing - viz calls to MapGet. It's less common to want to do that with the codefix. In addition, code fixes aren't on a hot path

src/Framework/Analyzer/src/DelegateEndpoints/DelegateEndpointFixer.cs Outdated

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public partial class DelegateEndpointFixer : CodeFixProvider
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sam's suggestion was to put code fixes in a different assembly since it depends on the workspaces assembly. Could we do that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep -- I recall this now.

@sharwell Can you sanity check that the packaging was done directly here?

Also, do they still need to be split out even if the CodeFixes don't take an explicit dependency on the Microsoft.CodeAnalysis.Workspaces assembly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do they still need to be split out even if the CodeFixes don't take an explicit dependency on the

CodeFixProvider is defined in Microsoft.CodeAnalysis.Workspaces, so it's impossible to avoid.

@captainsafia captainsafia force-pushed the safia/optionality-analyzer branch from 3927aae to a208204 Compare September 9, 2021 21:48
@captainsafia captainsafia force-pushed the safia/optionality-analyzer branch from a208204 to 6d083a0 Compare September 9, 2021 23:11
@captainsafia
Copy link
Member Author

@dotnet/aspnet-build Can I get help merging?

@dougbu dougbu merged commit cd424e6 into release/6.0 Sep 10, 2021
@dougbu dougbu deleted the safia/optionality-analyzer branch September 10, 2021 16:17
@ghost ghost added this to the 6.0-rc2 milestone Sep 10, 2021
@dougbu
Copy link
Member

dougbu commented Sep 10, 2021

Can I get help merging?

No 😀

captainsafia added a commit that referenced this pull request Sep 10, 2021
@captainsafia
Add analyzer for detecting mismatched endpoint parameter optionality (#…
c279156 
…36154)

* Add analyzer for detecting mismatched endpoint parameter optionality

* Address feedback from code review

* Factor out CodeFixes and Analyzers to separate assemblies

* Address more feedback from review

* Address code checks
@captainsafia captainsafia mentioned this pull request Sep 10, 2021
Merged
captainsafia added a commit that referenced this pull request Sep 14, 2021
@captainsafia @dougbu
Cherry-pick mismatched parameter optionality analyzer and clean up di…
b156057 
…rectories (#36379)

* Add analyzer for detecting mismatched endpoint parameter optionality (#36154)

* Add analyzer for detecting mismatched endpoint parameter optionality

* Address feedback from code review

* Factor out CodeFixes and Analyzers to separate assemblies

* Address more feedback from review

* Address code checks

* Clean up folder structure

* Update solution file and address feedback

* Update src/Framework/Framework.slnf

Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>

* Add test project to solution file

Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharwell sharwell

@pranavkm pranavkm

Assignees
No one assigned
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Milestone
6.0-rc2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@captainsafia @dougbu @pranavkm @sharwell