Skip to content

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Sep 1, 2025

Fix some Ql4Ql violations based on the following checks

  • ql/field-only-used-in-charpred
  • ql/could-be-cast
  • ql/counting-to-zero
  • ql/dataflow-module-naming-convention
  • ql/if-with-none
  • ql/missing-parameter-qldoc
  • ql/misspelling

@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Sep 2, 2025
@michaelnebel michaelnebel marked this pull request as ready for review September 2, 2025 08:02
@michaelnebel michaelnebel requested a review from a team as a code owner September 2, 2025 08:02
@Copilot Copilot AI review requested due to automatic review settings September 2, 2025 08:02
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 fixes several Ql4Ql violations identified by static analysis checks, focusing on code quality improvements in QL test files and source code. The changes address issues related to field usage, casting patterns, dataflow naming conventions, and misspellings.

  • Adds field accessor methods to test classes to address "field-only-used-in-charpred" violations
  • Refactors redundant cast logic by moving variable declarations into appropriate scopes
  • Updates expected test outputs to reflect line number changes from added methods

Reviewed Changes

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

Show a summary per file
File Description
ql/ql/test/queries/style/UseSetLiteral/test.qll Adds getS() method to access field s
ql/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected Updates line numbers in expected test results
ql/ql/test/queries/style/UseInstanceofExtension/UseInstanceofExtension.expected Updates line numbers in expected test results
ql/ql/test/queries/style/UseInstanceofExtension/Foo.qll Adds getRange() method to access field range
ql/ql/test/queries/style/Misspelling/Test.qll Adds getNum() method to access field numOccurences
ql/ql/test/queries/style/Misspelling/Misspelling.expected Updates line numbers in expected test results
ql/ql/test/queries/performance/VarUnusedInDisjunct/Test.qll Adds getField() method to access field field
ql/ql/src/codeql_ql/style/RedundantCastQuery.qll Refactors variable scoping in RedundantInlineCast and RedundantAnyCast classes

@@ -81,6 +81,8 @@ class MyTest8Class extends int {
predicate is(int x) { x = this }

int get() { result = this }

string getS() { result = s }
Copy link
Preview

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The method references field 's' but this field is not defined in the MyTest8Class. This will cause a compilation error.

Copilot uses AI. Check for mistakes.

@@ -8,6 +8,8 @@ class PublicallyAccessible extends string {

// should be argument
predicate hasAgrument() { none() }

int getNum() { result = numOccurences }
Copy link
Preview

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The field name 'numOccurences' contains a misspelling - it should be 'numOccurrences' with double 'r'.

Note: See the diff below for a potential fix:

@@ -2,14 +2,14 @@
  * A string that's deliberately mispelled (and so is that last word).
  */
 class PublicallyAccessible extends string {
-  int numOccurences; // should be 'occurrences'
+  int numOccurrences; // should be 'occurrences'
 
-  PublicallyAccessible() { this = "publically" and numOccurences = 123 }
+  PublicallyAccessible() { this = "publically" and numOccurrences = 123 }
 
   // should be argument
   predicate hasAgrument() { none() }
 
-  int getNum() { result = numOccurences }
+  int getNum() { result = numOccurrences }
 }
 
 /**

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelnebel michaelnebel merged commit 88ee20f into github:main Sep 2, 2025
13 checks passed
@michaelnebel michaelnebel deleted the ql/ql4ql branch September 2, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note QL-for-QL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants