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

Add Argument tracker and tests #8950

Open
wants to merge 4 commits into
base: feature/New_S6932
Choose a base branch
from
Open

Add Argument tracker and tests #8950

wants to merge 4 commits into from
+1,959 −50

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Adds an ArgumentDescriptor and logic to match an ArgumentSyntax against it.
Based on #8947

@github-actions github-actions bot added this to In progress in Best Kanban Mar 18, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 New issue
83.3% Coverage on New Code (required ≥ 95%)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

This was referenced Mar 18, 2024
Draft
Closed
@martin-strecker-sonarsource martin-strecker-sonarsource mentioned this pull request Apr 18, 2024
Merged
Base automatically changed from Martin/RoslynExtensions to feature/New_S6932 April 24, 2024 09:56
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Apr 24, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review April 24, 2024 12:50
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Apr 24, 2024
costin-zaharia-sonarsource
costin-zaharia-sonarsource requested changes Apr 25, 2024
View reviewed changes
analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpTrackerFacade.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpAttributeParameterLookup.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodParameterLookup.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpArgumentTracker.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Facade/ITrackerFacade.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Show resolved Hide resolved
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Apr 25, 2024
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Apr 29, 2024
costin-zaharia-sonarsource
costin-zaharia-sonarsource reviewed Apr 30, 2024
View reviewed changes
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpArgumentTracker.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Show resolved Hide resolved
@costin-zaharia-sonarsource costin-zaharia-sonarsource moved this from In progress to Review in progress in Best Kanban Apr 30, 2024
costin-zaharia-sonarsource
costin-zaharia-sonarsource reviewed Apr 30, 2024
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

My last round on the ArgumentTrackerTest.cs. I will continue with impl review.

analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
costin-zaharia-sonarsource
costin-zaharia-sonarsource reviewed Apr 30, 2024
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

Another round, this time for ArgumentDescriptor. I'm sorry for the slow progress but it takes me longer than expected to review the code.

analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpArgumentTracker.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpArgumentTracker.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpArgumentTracker.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/ArgumentDescriptor.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Trackers/ArgumentTrackerTest.cs Outdated Show resolved Hide resolved
costin-zaharia-sonarsource
costin-zaharia-sonarsource reviewed May 2, 2024
View reviewed changes
analyzers/src/SonarAnalyzer.Common/Trackers/ArgumentTracker.cs Outdated
&& Language.MethodParameterLookup(invoked, methodSymbol).TryGetSymbol(argumentNode, out var parameter)
&& ParameterFits(parameter, descriptor.ParameterConstraint, descriptor.InvokedMemberConstraint))
{
context.Parameter = parameter;
Copy link
Member

Choose a reason for hiding this comment

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

I find it very confusing that this method has side effects. Will this used only in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this used only in tests?

Unfortunately not. The rule PR uses this call.

this method has side effects

Yes. This is very unfortunate. There should be a distinction between input and output, but the context is both at the moment. This is how the tracker is designed now: It creates a pipeline of matchers and passes the context from one to the others. Each matcher can manipulate parts of the context. (It's a bit like the request pipeline in asp.net core).

@costin-zaharia-sonarsource costin-zaharia-sonarsource moved this from Review in progress to In progress in Best Kanban May 3, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Passed Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Failed Quality Gate failed for 'SonarAnalyzer for .NET'

Failed conditions
18 New issues
84.5% Coverage on New Code (required ≥ 95%)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

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

@costin-zaharia-sonarsource costin-zaharia-sonarsource costin-zaharia-sonarsource requested changes

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource Awaiting requested review from zsolt-kolbay-sonarsource

Requested changes must be addressed to merge this pull request.

Assignees

@martin-strecker-sonarsource martin-strecker-sonarsource

Labels
Sprint: ASP.NET MVC ASP.NET rules
Projects
Best Kanban
  
In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@martin-strecker-sonarsource @costin-zaharia-sonarsource @zsolt-kolbay-sonarsource