Skip to content
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

Implement BL0007: Component parameters should be auto property #30102

Merged
merged 18 commits into from
Feb 3, 2022
Merged

Implement BL0007: Component parameters should be auto property #30102

merged 18 commits into from
Feb 3, 2022

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Feb 11, 2021 •
edited
Loading

Fixes #26230

TODOs:

Wanted to get an initial review before proceeding.

@Youssef1313 Youssef1313 requested a review from a team as a code owner February 11, 2021 15:31
@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Feb 11, 2021
Youssef1313
Youssef1313 commented Feb 11, 2021
View reviewed changes
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated
});
}

private async Task<bool> IsSameSemanticAsAutoPropertyAsync(IPropertySymbol symbol, CancellationToken cancellationToken)
Copy link
Member Author

Choose a reason for hiding this comment

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

This method attempts to handle #26230 (comment).

Feel free if you want me to revert and not handle it.

pranavkm
pranavkm reviewed Feb 12, 2021
View reviewed changes
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/Helpers/SymbolExtensions.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/Helpers/SymbolExtensions.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/Helpers/SymbolExtensions.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/Helpers/SymbolExtensions.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/Helpers/WellKnownTypeNames.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated Show resolved Hide resolved
pranavkm
pranavkm reviewed Feb 12, 2021
View reviewed changes
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated
var setterAccessor = syntax.AccessorList.Accessors[1];
if (getterAccessor.IsKind(SyntaxKind.SetAccessorDeclaration))
{
// Swap if necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

This deserves a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pranavkm Did you mean a more explanatory comment?

src/Analyzers/Analyzers/src/UseParameterAttributeCorrectlyAnalyzer.cs Outdated
(getterAccessor, setterAccessor) = (setterAccessor, getterAccessor);
}

if (!getterAccessor.IsKind(SyntaxKind.GetAccessorDeclaration) || !setterAccessor.IsKind(SyntaxKind.SetAccessorDeclaration))
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't read the rest of this method, but is there a way to detect this by inspecting if the getters / setters are compiled generated?

@sharwell in case you have suggestions. We're basically trying to determine that a property is an auto-property. public string Prop1 { get; set; }

Copy link
Member Author

Choose a reason for hiding this comment

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

@pranavkm This method isn't for detecting auto property, but full-properties with the same semantics. See the comments in the linked issue.

@Youssef1313 Youssef1313 requested a review from a team as a code owner February 12, 2021 04:18
@Youssef1313 Youssef1313 changed the title Implement ASP0002: Use 'ParameterAttribute' correctly Implement BL0007: Use 'ParameterAttribute' correctly Feb 12, 2021
@Youssef1313 Youssef1313 force-pushed the parameter-attribute-analyzer branch 3 times, most recently from 41206a8 to 7c830ae Compare February 12, 2021 05:03
@Youssef1313 Youssef1313 changed the title Implement BL0007: Use 'ParameterAttribute' correctly Implement BL0007: Component parameters should be auto property Feb 12, 2021
@mkArtakMSFT mkArtakMSFT added the community-contribution Indicates that the PR has been added by a community member label Apr 27, 2021
dougbu
dougbu previously requested changes Jul 23, 2021
View reviewed changes
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

New files need MIT license headers.

@pranavkm
Copy link
Contributor

pranavkm commented Aug 5, 2021

@Youssef1313 sorry this PR has been sitting in the backburner so far. While we would like to take this change, there's some concern about adding it to the default set of analyzers and producing warnings in 3.1 or 5.0 Blazor apps. There's a couple of options we could approach this (including shipping analyzers in the runtime pack) which might mitigate this. Unfortunately I haven't been able to follow up on this. Given how close we're to calling 6.0 done, I'm hoping to get back to this early in 7.0 so we give any choice we make enough time to bake.

@mkArtakMSFT
Copy link
Member

@pranavkm looks like this is still relevant and you want to get back to this soon, don't you?

@pranavkm pranavkm mentioned this pull request Jan 26, 2022
Merged
pranavkm added a commit to pranavkm/aspnetcore that referenced this pull request Jan 27, 2022
@pranavkm
Fork component analyzers
d9edcb9 
This is an attempt to resolve the open dotnet#30102 PR. Microsoft.AspNetCore.Components.Analyzers ships as a part
of the .NET SDK and the analysis applies to all apps starting in .NET 3.1 or newer. Adding new analyzers to this assembly was problematic since it
would result in new warnings in existing apps when users updated the SDK.

This PR forks the Component.Analyzers to produce a new Components.SDKAnalyzers analyzer assembly. Components.Analyzers is also added to the targeting
pack so it allows us to produce TFM specific analysis.
pranavkm added a commit that referenced this pull request Jan 29, 2022
@pranavkm @TanayParikh
Fork component analyzers (#39796)
112a0de 
* Fork component analyzers

This is an attempt to resolve the open #30102 PR. Microsoft.AspNetCore.Components.Analyzers ships as a part
of the .NET SDK and the analysis applies to all apps starting in .NET 3.1 or newer. Adding new analyzers to this assembly was problematic since it
would result in new warnings in existing apps when users updated the SDK.

This PR forks the Component.Analyzers to produce a new Components.SDKAnalyzers analyzer assembly. Components.Analyzers is also added to the targeting
pack so it allows us to produce TFM specific analysis.

* Apply suggestions from code review

Co-authored-by: Tanay Parikh <TanayParikh@users.noreply.github.com>

Co-authored-by: Tanay Parikh <TanayParikh@users.noreply.github.com>
@pranavkm pranavkm requested a review from dougbu February 3, 2022 01:41
@pranavkm pranavkm enabled auto-merge (squash) February 3, 2022 02:29
dougbu
dougbu reviewed Feb 3, 2022
View reviewed changes
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Other than the lovely MIT license and trailing whitespace, anything you need me to look at @pranavkm❔

@pranavkm
Copy link
Contributor

pranavkm commented Feb 3, 2022

anything you need me to look at @pranavkm❔

Nope. Thanks for fixing up the whitespace.

@pranavkm pranavkm dismissed dougbu’s stale review February 3, 2022 20:19

Looks like the license was updated.

@pranavkm pranavkm merged commit 417f950 into dotnet:main Feb 3, 2022
@ghost ghost added this to the 7.0-preview2 milestone Feb 3, 2022
@Youssef1313 Youssef1313 deleted the parameter-attribute-analyzer branch February 3, 2022 22:37
@Youssef1313
Copy link
Member Author

Thanks @pranavkm and @dougbu for jumping in and completing this!

@ghost
Copy link

ghost commented Feb 3, 2022

Hi @Youssef1313. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS left review comments

@pranavkm pranavkm pranavkm approved these changes

Assignees

@pranavkm pranavkm

Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0-preview2
Development

Successfully merging this pull request may close these issues.

Consider an analyzer to warn if a [Parameter] property isn't a simple auto property
6 participants
@Youssef1313 @pranavkm @mkArtakMSFT @SteveSandersonMS @dougbu @Pilchie