Skip to content

Add cs/string-concatenation-in-loop to the quality suite #19650

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

Merged
merged 1 commit into from
Jun 10, 2025

Conversation

tamasvajk
Copy link
Contributor

@tamasvajk tamasvajk commented Jun 3, 2025

This PR is adding the cs/string-concatenation-in-loop query to the code-quality suite. I've ran the query on MRVA and then manually checked autofix suggestions. The results and the fixes are sensible.

  • MRVA looks good
  • need to check autofix (might need more compliant/non-compliant samples)

@github-actions github-actions bot added the C# label Jun 3, 2025
@tamasvajk tamasvajk marked this pull request as ready for review June 10, 2025 07:09
@Copilot Copilot AI review requested due to automatic review settings June 10, 2025 07:09
@tamasvajk tamasvajk requested a review from a team as a code owner June 10, 2025 07:09
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR integrates the new cs/string-concatenation-in-loop performance rule into the C# code-quality suite.

  • Adds the quality tag to the rule’s metadata.
  • Includes the new query in the csharp-code-quality.qls.expected suite file.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
csharp/ql/src/Performance/StringConcatenationInLoop.ql Added quality to the @tags list in the rule header
csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected Registered the new query in the expected suite output
Comments suppressed due to low confidence (1)

csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected:14

  • Integration tests now include the new query in the suite, but there are no corresponding positive and negative test fixtures (e.g., sample C# files) to verify that the rule correctly flags string-concatenation-in-loop cases and accepts compliant code. Consider adding .qltest scenarios under integration-tests to cover both compliant and non-compliant patterns.
ql/csharp/ql/src/Performance/StringConcatenationInLoop.ql

Copy link
Contributor

@michaelnebel michaelnebel 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!
Is there a corresponding PR in the codeql-autofix repo that adds it to the CCR suite (maybe that is not needed either?).

@tamasvajk
Copy link
Contributor Author

Looks good! Is there a corresponding PR in the codeql-autofix repo that adds it to the CCR suite (maybe that is not needed either?).

I didn't update that suite. Should I?

@tamasvajk
Copy link
Contributor Author

@michaelnebel What do you think, do we need a change note? (I think we don't)

@michaelnebel
Copy link
Contributor

@michaelnebel What do you think, do we need a change note? (I think we don't)

We shouldn't add a change note when adding a query to the code-quality suite.

@michaelnebel
Copy link
Contributor

michaelnebel commented Jun 10, 2025

Looks good! Is there a corresponding PR in the codeql-autofix repo that adds it to the CCR suite (maybe that is not needed either?).

I didn't update that suite. Should I?

Not sure, whether this suite was only consumed by the suite that was used by the now disabled feature flag - I have been updating it, but maybe it is not needed. Will ask in Slack.

@tamasvajk tamasvajk added the no-change-note-required This PR does not need a change note label Jun 10, 2025
@tamasvajk tamasvajk merged commit 7a632e8 into github:main Jun 10, 2025
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants