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.

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

Fix S1144 FP: types used through reflection #9406

Merged
merged 13 commits into from
Jun 13, 2024

Conversation

zsolt-kolbay-sonarsource
Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource commented Jun 5, 2024

Fixes #9379

RSpec change: SonarSource/rspec#3992

Copy link
Contributor

Choose a reason for hiding this comment

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

It generally looks fine. It misses some UTs that might be worth adding. Feel free not to add them if you think it's not worth it.

@CristianAmbrosini CristianAmbrosini marked this pull request as ready for review June 6, 2024 15:07
@CristianAmbrosini CristianAmbrosini enabled auto-merge (squash) June 6, 2024 15:52
@CristianAmbrosini CristianAmbrosini marked this pull request as draft June 7, 2024 09:59
auto-merge was automatically disabled June 7, 2024 09:59

Pull request was converted to draft

@zsolt-kolbay-sonarsource
Copy link
Contributor Author

I removed the check through type parameters, because it nearly doubled the analysis time.
We'll need to accept it as FP.

@martin-strecker-sonarsource
Copy link
Contributor

I removed the check through type parameters, because it nearly doubled the analysis time.
We'll need to accept it as FP.

How did you measure? If the rule was very, very fast, duplication is not a problem. Can you post some more info about the perf validation?

@zsolt-kolbay-sonarsource
Copy link
Contributor Author

I removed the check through type parameters, because it nearly doubled the analysis time.
We'll need to accept it as FP.

How did you measure? If the rule was very, very fast, duplication is not a problem. Can you post some more info about the perf validation?

I used the AnalyzerRunner from the Roslyn repo to compare the performance with and without the check for type parameters. I ran it on AutoMapper, Akka.Net, and an earlier version of the Roslyn repo. They all showed a degradation in performance ranging between 40% and 90%. UnusedPrivateMember is one of the slowest rules we have (excluding SE rules), so I don't think such a change is acceptable.

WITHOUT the check for type arguments (AutoMapper):

Statistics for UnusedPrivateMember:
Concurrent:                     False
Execution time (ms):            3644.8062
Code Block Actions:             0
Code Block Start Actions:       0
Code Block End Actions:         0
Compilation Actions:            0
Compilation Start Actions:      6
Compilation End Actions:        6
Operation Actions:              0
Operation Block Actions:        0
Operation Block Start Actions:  0
Operation Block End Actions:    0
Semantic Model Actions:         0
Symbol Actions:                 6
Symbol Start Actions:           0
Symbol End Actions:             0
Syntax Node Actions:            0
Syntax Tree Actions:            0
Additional File Actions:        0
Suppression Actions:            0
Execution times (ms):
UnusedPrivateMember:    3645
  S1144: 1364 instances
  S4487: 9 instances

--------------------------------------------------------
--------------------------------------------------------

INCLUDING the check for type arguments (AutoMapper):

Statistics for UnusedPrivateMember:
Concurrent:                     False
Execution time (ms):            5731.9556
Code Block Actions:             0
Code Block Start Actions:       0
Code Block End Actions:         0
Compilation Actions:            0
Compilation Start Actions:      6
Compilation End Actions:        6
Operation Actions:              0
Operation Block Actions:        0
Operation Block Start Actions:  0
Operation Block End Actions:    0
Semantic Model Actions:         0
Symbol Actions:                 6
Symbol Start Actions:           0
Symbol End Actions:             0
Syntax Node Actions:            0
Syntax Tree Actions:            0
Additional File Actions:        0
Suppression Actions:            0
Execution times (ms):
UnusedPrivateMember:    5732
  S1144: 1364 instances
  S4487: 9 instances

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also update the RSpec? It needs the DynamicallyAccessedMembersAttribute in the exception section. Consider only checking only the name of the attribute instead of namespace/name. This would allow users to create this attribute in unsupported platforms (which is every platform except Net5 upwards). The name is long enough to not be confused with anything else. If you go that route, tests are needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. I think a comment is helpful in this case.

Copy link

sonarcloud bot commented Jun 13, 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

Copy link

sonarcloud bot commented Jun 13, 2024

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource merged commit c485d21 into master Jun 13, 2024
26 checks passed
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource deleted the Zsolt/Fix-S1144-FP-reflection branch June 13, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix S1144 FP: Diagnostic doesn't respect reflection with DynamicallyAccessedMembers attribute
3 participants