Shared and Sync: Fix some Ql4Ql violations. #20335
Conversation
michaelnebel
added
the
no-change-note-required
There was a problem hiding this comment.
Pull Request Overview
This PR fixes several Ql4Ql violations by addressing code quality issues identified by static analysis checks including field usage, naming conventions, control flow, and documentation.
Key changes include:
- Improved control flow structure by replacing
if-else-nonepattern with more explicit conditional logic - Enhanced module naming by giving descriptive names to generic module variables
- Added missing parameter documentation for better code clarity
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| shared/rangeanalysis/codeql/rangeanalysis/RangeAnalysis.qll | Refactored nested if-else-none pattern to use explicit conditional logic |
| shared/quantum/codeql/quantum/experimental/Model.qll | Fixed spelling error and removed unused field variable |
| shared/dataflow/codeql/dataflow/TaintTracking.qll | Renamed generic module names to be more descriptive |
| shared/dataflow/codeql/dataflow/DataFlow.qll | Renamed generic module variable C to descriptive StateConfig |
| ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsExtensions.qll | Added missing parameter documentation for madId |
| python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll | Added missing parameter documentation for madId |
| javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExtensions.qll | Added missing parameter documentation for madId |
| abstract class ArtifactConsumer extends ConsumerElement { | ||
| /** | ||
| * Use `getAKnownArtifactSource() instead. The behaviour of these two predicates is equivalent. | ||
| * Use `getAKnownArtifactSource() instead. The behavior of these two predicates is equivalent. |
There was a problem hiding this comment.
Good correction of 'behaviour' to 'behavior' for American English consistency.
| */ | ||
| module Global<ConfigSig Config> implements GlobalFlowSig { | ||
| private module C implements FullStateConfigSig { | ||
| private module StateConfig implements FullStateConfigSig { |
There was a problem hiding this comment.
Please revert. It's a short-scoped private name, and making it longer may excessively bloat the length of DIL/RA names.
There was a problem hiding this comment.
Hmm, I was not aware that this was a concern! Thx!
| */ | ||
| module GlobalWithState<StateConfigSig Config> implements GlobalFlowSig { | ||
| private module C implements FullStateConfigSig { | ||
| private module StateConfig implements FullStateConfigSig { |
| private import MakeImplStage1<Location, DataFlowLang> as DataFlowInternalStage1 | ||
|
|
||
| private module AddTaintDefaults<DataFlowInternal::FullStateConfigSig Config> implements | ||
| private module MakeAddTaintDefaultsConfig<DataFlowInternal::FullStateConfigSig Config> implements |
There was a problem hiding this comment.
Revert? I don't see a good reason for this change. Also, the double verb prefix becomes awkward. Whatever QL4QL rule that demands all modules implementing a data flow config to end in Config should instead be more lenient.
There was a problem hiding this comment.
Fair enough, I will revert it. However, I do think that the existing name is slightly confusing. The parameterized module is used to "make" a config module (maybe a better name is MakeTaintDefaultsConfig). In any case, I will just revert the change.
|
|
||
| private module C implements DataFlowInternal::FullStateConfigSig { | ||
| import AddTaintDefaults<Config0> | ||
| private module AddTaintDefaultsConfig implements DataFlowInternal::FullStateConfigSig { |
|
|
||
| private module C implements DataFlowInternal::FullStateConfigSig { | ||
| import AddTaintDefaults<Config0> | ||
| private module AddTaintDefaultsConfig implements DataFlowInternal::FullStateConfigSig { |
| else ( | ||
| semNegative(x) and | ||
| upper = false and | ||
| delta = D::fromInt(0) |
There was a problem hiding this comment.
Revert. This change breaks the uniformity of the code pattern above.
aschackmull
left a comment
aschackmull
left a comment
There was a problem hiding this comment.
I'd like most of these changes reverted, please.
michaelnebel
force-pushed
the
shared/ql4ql
branch
from
7ec6444 to
4caae6b
Compare
michaelnebel
force-pushed
the
shared/ql4ql
branch
from
4caae6b to
7490d8d
Compare
2 participants
Fix some Ql4Ql violations based on the following checks
ql/field-only-used-in-charpredql/could-be-castql/counting-to-zeroql/dataflow-module-naming-conventionql/if-with-noneql/missing-parameter-qldocql/misspelling