From f20c18627713ba1d3578ec2aafbbbcb39d451710 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 14 Jul 2022 12:20:30 +0200 Subject: [PATCH 1/6] add ql/repeated-word query --- ql/ql/src/codeql_ql/ast/Ast.qll | 10 +++++++ ql/ql/src/codeql_ql/ast/internal/AstNodes.qll | 3 ++ ql/ql/src/queries/style/RepeatedWord.ql | 30 +++++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100644 ql/ql/src/queries/style/RepeatedWord.ql diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index e399128c09a..4b3c00df10d 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -178,6 +178,16 @@ class BlockComment extends TBlockComment, AstNode { override string getAPrimaryQlClass() { result = "BlockComment" } } +class LineComment extends TLineComment, AstNode { + QL::LineComment comment; + + LineComment() { this = TLineComment(comment) } + + string getContents() { result = comment.getValue() } + + override string getAPrimaryQlClass() { result = "LineComment" } +} + /** * The `from, where, select` part of a QL query. */ diff --git a/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll b/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll index 9448fe71240..312f7f90c60 100644 --- a/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll +++ b/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll @@ -7,6 +7,7 @@ newtype TAstNode = TTopLevel(QL::Ql file) or TQLDoc(QL::Qldoc qldoc) or TBlockComment(QL::BlockComment comment) or + TLineComment(QL::LineComment comment) or TClasslessPredicate(QL::ClasslessPredicate pred) or TVarDecl(QL::VarDecl decl) or TFieldDecl(QL::Field field) or @@ -154,6 +155,8 @@ QL::AstNode toQL(AST::AstNode n) { or n = TBlockComment(result) or + n = TLineComment(result) + or n = TClasslessPredicate(result) or n = TVarDecl(result) diff --git a/ql/ql/src/queries/style/RepeatedWord.ql b/ql/ql/src/queries/style/RepeatedWord.ql new file mode 100644 index 00000000000..90ce45344dd --- /dev/null +++ b/ql/ql/src/queries/style/RepeatedWord.ql @@ -0,0 +1,30 @@ +/** + * @name Comment has repeated word + * @description Comment has repeated word. + * @kind problem + * @problem.severity warning + * @id ql/repeated-word + * @precision very-high + */ + +import ql + +string getComment(AstNode node) { + result = node.(QLDoc).getContents() + or + result = node.(BlockComment).getContents() + or + result = node.(LineComment).getContents() +} + +/** Gets a word used in `node` */ +string getWord(AstNode node) { result = getComment(node).regexpFind("\\b[A-Za-z]+\\b", _, _) } + +AstNode hasRepeatedWord(string word) { + word = getWord(result) and + getComment(result).regexpMatch(".*\\b" + word + "\\s+" + word + "\\b.*") +} + +from AstNode comment, string word +where comment = hasRepeatedWord(word) +select comment, "The comment repeats " + word + "." From cb3a0fb5deecb99e4bc7809d0a19b61fe6ee74b6 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 14 Jul 2022 12:25:01 +0200 Subject: [PATCH 2/6] make a Comment superclass --- ql/ql/src/codeql_ql/ast/Ast.qll | 16 ++++++++++------ ql/ql/src/codeql_ql/ast/internal/AstNodes.qll | 2 ++ ql/ql/src/queries/style/RepeatedWord.ql | 16 ++++------------ 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index 4b3c00df10d..03a26868657 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -156,34 +156,38 @@ class TopLevel extends TTopLevel, AstNode { override QLDoc getQLDoc() { result = this.getMember(0) } } -class QLDoc extends TQLDoc, AstNode { +abstract class Comment extends AstNode, TComment { + abstract string getContents(); +} + +class QLDoc extends TQLDoc, Comment { QL::Qldoc qldoc; QLDoc() { this = TQLDoc(qldoc) } - string getContents() { result = qldoc.getValue() } + override string getContents() { result = qldoc.getValue() } override string getAPrimaryQlClass() { result = "QLDoc" } override AstNode getParent() { result.getQLDoc() = this } } -class BlockComment extends TBlockComment, AstNode { +class BlockComment extends TBlockComment, Comment { QL::BlockComment comment; BlockComment() { this = TBlockComment(comment) } - string getContents() { result = comment.getValue() } + override string getContents() { result = comment.getValue() } override string getAPrimaryQlClass() { result = "BlockComment" } } -class LineComment extends TLineComment, AstNode { +class LineComment extends TLineComment, Comment { QL::LineComment comment; LineComment() { this = TLineComment(comment) } - string getContents() { result = comment.getValue() } + override string getContents() { result = comment.getValue() } override string getAPrimaryQlClass() { result = "LineComment" } } diff --git a/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll b/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll index 312f7f90c60..ce5cd352dd8 100644 --- a/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll +++ b/ql/ql/src/codeql_ql/ast/internal/AstNodes.qll @@ -90,6 +90,8 @@ class TYamlNode = TYamlCommemt or TYamlEntry or TYamlKey or TYamlListitem or TYa class TSignatureExpr = TPredicateExpr or TType; +class TComment = TQLDoc or TBlockComment or TLineComment; + /** DEPRECATED: Alias for TYamlNode */ deprecated class TYAMLNode = TYamlNode; diff --git a/ql/ql/src/queries/style/RepeatedWord.ql b/ql/ql/src/queries/style/RepeatedWord.ql index 90ce45344dd..f8e0ddd1018 100644 --- a/ql/ql/src/queries/style/RepeatedWord.ql +++ b/ql/ql/src/queries/style/RepeatedWord.ql @@ -9,22 +9,14 @@ import ql -string getComment(AstNode node) { - result = node.(QLDoc).getContents() - or - result = node.(BlockComment).getContents() - or - result = node.(LineComment).getContents() -} - /** Gets a word used in `node` */ -string getWord(AstNode node) { result = getComment(node).regexpFind("\\b[A-Za-z]+\\b", _, _) } +string getWord(Comment node) { result = node.getContents().regexpFind("\\b[A-Za-z]+\\b", _, _) } -AstNode hasRepeatedWord(string word) { +Comment hasRepeatedWord(string word) { word = getWord(result) and - getComment(result).regexpMatch(".*\\b" + word + "\\s+" + word + "\\b.*") + result.getContents().regexpMatch(".*\\b" + word + "\\s+" + word + "\\b.*") } -from AstNode comment, string word +from Comment comment, string word where comment = hasRepeatedWord(word) select comment, "The comment repeats " + word + "." From 2ea2bd89663f5a3c82778fba475ea2fa57f757a1 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 14 Jul 2022 12:35:09 +0200 Subject: [PATCH 3/6] refine the repeated-word query --- ql/ql/src/queries/style/RepeatedWord.ql | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ql/ql/src/queries/style/RepeatedWord.ql b/ql/ql/src/queries/style/RepeatedWord.ql index f8e0ddd1018..cbbed4668bd 100644 --- a/ql/ql/src/queries/style/RepeatedWord.ql +++ b/ql/ql/src/queries/style/RepeatedWord.ql @@ -14,9 +14,12 @@ string getWord(Comment node) { result = node.getContents().regexpFind("\\b[A-Za- Comment hasRepeatedWord(string word) { word = getWord(result) and - result.getContents().regexpMatch(".*\\b" + word + "\\s+" + word + "\\b.*") + result.getContents().regexpMatch(".*[\\s]" + word + "\\s+" + word + "[\\s.,].*") } from Comment comment, string word -where comment = hasRepeatedWord(word) +where + comment = hasRepeatedWord(word) and + // lots of these, and I can't just change old dbschemes. + not (word = "type" and comment.getLocation().getFile().getExtension() = "dbscheme") select comment, "The comment repeats " + word + "." From 85a652f3d104ad10a6fbaaa43731e48363b8e5d6 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 14 Jul 2022 12:35:23 +0200 Subject: [PATCH 4/6] remove a bunch of repeated words --- cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll | 2 +- .../lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll | 2 +- .../code/csharp/controlflow/internal/ControlFlowGraphImpl.qll | 2 +- csharp/ql/src/Telemetry/ExternalApi.qll | 2 +- go/ql/src/RedundantCode/DuplicateSwitchCase.ql | 2 +- java/ql/src/Telemetry/ExternalApi.qll | 2 +- python/ql/lib/semmle/python/Frameworks.qll | 2 +- python/ql/lib/semmle/python/dataflow/old/TaintTracking.qll | 2 +- ql/ql/src/codeql_ql/ast/internal/Module.qll | 2 +- ql/ql/src/queries/performance/MissingNomagic.ql | 2 +- ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll | 2 +- ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll | 2 +- ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll | 2 +- 13 files changed, 13 insertions(+), 13 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll index da8e7564b3e..74ba3569d20 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll @@ -699,7 +699,7 @@ private predicate exprToExprStep_nocfg(Expr fromExpr, Expr toExpr) { call.getTarget() = f and // AST dataflow treats a reference as if it were the referred-to object, while the dataflow // models treat references as pointers. If the return type of the call is a reference, then - // look for data flow the the referred-to object, rather than the reference itself. + // look for data flow the referred-to object, rather than the reference itself. if call.getType().getUnspecifiedType() instanceof ReferenceType then outModel.isReturnValueDeref() else outModel.isReturnValue() diff --git a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll index c9e790d9077..0f47770dc82 100644 --- a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll +++ b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll @@ -1573,7 +1573,7 @@ private module SimpleRangeAnalysisCached { result = min([max(getTruncatedUpperBounds(expr)), getGuardedUpperBound(expr)]) } - /** Holds if the upper bound of `expr` may have been widened. This means the the upper bound is in practice likely to be overly wide. */ + /** Holds if the upper bound of `expr` may have been widened. This means the upper bound is in practice likely to be overly wide. */ cached predicate upperBoundMayBeWidened(Expr e) { isRecursiveExpr(e) and diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll index d9bebb9bf17..b63d804216b 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll @@ -335,7 +335,7 @@ module Expressions { // ```csharp // new Dictionary() { [0] = "Zero", [1] = "One", [2] = "Two" } // ``` - // need special treatment, because the the accesses `[0]`, `[1]`, and `[2]` + // need special treatment, because the accesses `[0]`, `[1]`, and `[2]` // have no qualifier. this = any(MemberInitializer mi).getLValue() } diff --git a/csharp/ql/src/Telemetry/ExternalApi.qll b/csharp/ql/src/Telemetry/ExternalApi.qll index 9679cd1061a..685eda64e50 100644 --- a/csharp/ql/src/Telemetry/ExternalApi.qll +++ b/csharp/ql/src/Telemetry/ExternalApi.qll @@ -85,7 +85,7 @@ class ExternalApi extends DotNet::Callable { defaultAdditionalTaintStep(this.getAnInput(), _) } - /** Holds if this API is is a constructor without parameters. */ + /** Holds if this API is a constructor without parameters. */ private predicate isParameterlessConstructor() { this instanceof Constructor and this.getNumberOfParameters() = 0 } diff --git a/go/ql/src/RedundantCode/DuplicateSwitchCase.ql b/go/ql/src/RedundantCode/DuplicateSwitchCase.ql index 46956347785..7af97b76875 100644 --- a/go/ql/src/RedundantCode/DuplicateSwitchCase.ql +++ b/go/ql/src/RedundantCode/DuplicateSwitchCase.ql @@ -13,7 +13,7 @@ import go -/** Gets the global value number of of `e`, which is the `i`th case label of `switch`. */ +/** Gets the global value number of `e`, which is the `i`th case label of `switch`. */ GVN switchCaseGVN(SwitchStmt switch, int i, Expr e) { e = switch.getCase(i).getExpr(0) and result = e.getGlobalValueNumber() } diff --git a/java/ql/src/Telemetry/ExternalApi.qll b/java/ql/src/Telemetry/ExternalApi.qll index b839ca15c6d..eaf88555444 100644 --- a/java/ql/src/Telemetry/ExternalApi.qll +++ b/java/ql/src/Telemetry/ExternalApi.qll @@ -73,7 +73,7 @@ class ExternalApi extends Callable { TaintTracking::localAdditionalTaintStep(this.getAnInput(), _) } - /** Holds if this API is is a constructor without parameters. */ + /** Holds if this API is a constructor without parameters. */ private predicate isParameterlessConstructor() { this instanceof Constructor and this.getNumberOfParameters() = 0 } diff --git a/python/ql/lib/semmle/python/Frameworks.qll b/python/ql/lib/semmle/python/Frameworks.qll index daa67ee4231..6cf4044157b 100644 --- a/python/ql/lib/semmle/python/Frameworks.qll +++ b/python/ql/lib/semmle/python/Frameworks.qll @@ -2,7 +2,7 @@ * Helper file that imports all framework modeling. */ -// If you add modeling of a new framework/library, remember to add it it to the docs in +// If you add modeling of a new framework/library, remember to add it to the docs in // `docs/codeql/support/reusables/frameworks.rst` private import semmle.python.frameworks.Aioch private import semmle.python.frameworks.Aiohttp diff --git a/python/ql/lib/semmle/python/dataflow/old/TaintTracking.qll b/python/ql/lib/semmle/python/dataflow/old/TaintTracking.qll index 22832b46c6c..d0293522404 100755 --- a/python/ql/lib/semmle/python/dataflow/old/TaintTracking.qll +++ b/python/ql/lib/semmle/python/dataflow/old/TaintTracking.qll @@ -333,7 +333,7 @@ abstract class Sanitizer extends string { /** Holds if `taint` cannot flow through `node`. */ predicate sanitizingNode(TaintKind taint, ControlFlowNode node) { none() } - /** Holds if `call` removes removes the `taint` */ + /** Holds if `call` removes the `taint` */ predicate sanitizingCall(TaintKind taint, FunctionObject callee) { none() } /** Holds if `test` shows value to be untainted with `taint` */ diff --git a/ql/ql/src/codeql_ql/ast/internal/Module.qll b/ql/ql/src/codeql_ql/ast/internal/Module.qll index 91a5888ac12..8c1e6189e0a 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Module.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Module.qll @@ -78,7 +78,7 @@ private class Folder_ extends ContainerOrModule, TFolder { override ContainerOrModule getEnclosing() { result = TFolder(f.getParentContainer()) and - // if this the the root, then we stop. + // if this the root, then we stop. not exists(f.getFile("qlpack.yml")) } diff --git a/ql/ql/src/queries/performance/MissingNomagic.ql b/ql/ql/src/queries/performance/MissingNomagic.ql index 13084bf6ad6..ebab0e7b7e5 100644 --- a/ql/ql/src/queries/performance/MissingNomagic.ql +++ b/ql/ql/src/queries/performance/MissingNomagic.ql @@ -57,7 +57,7 @@ class CandidatePredicate extends Predicate { this.getName() .regexpCapture("(.+)" + ["0", "helper", "aux", "cand", "Helper", "Aux", "Cand"], 1) or - // Or this this predicate is named "foo02" and `pred` is named "foo01". + // Or this predicate is named "foo02" and `pred` is named "foo01". exists(int n, string name | hasNameWithNumberSuffix(pred, name, n) and hasNameWithNumberSuffix(this, name, n - 1) diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll index efb69af39eb..95a0514320e 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll @@ -348,7 +348,7 @@ module ExprNodes { /** Gets an argument of this call. */ final ExprCfgNode getAnArgument() { result = this.getArgument(_) } - /** Gets the the keyword argument whose key is `keyword` of this call. */ + /** Gets the keyword argument whose key is `keyword` of this call. */ final ExprCfgNode getKeywordArgument(string keyword) { exists(PairCfgNode n | e.hasCfgChild(e.getAnArgument(), this, n) and diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index 730445d99ac..11aa33b7551 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -71,7 +71,7 @@ class CallNode extends LocalSourceNode, ExprNode { /** Gets the data-flow node corresponding to the named argument of the call corresponding to this data-flow node */ ExprNode getKeywordArgument(string name) { result.getExprNode() = node.getKeywordArgument(name) } - /** Gets the name of the the method called by the method call (if any) corresponding to this data-flow node */ + /** Gets the name of the method called by the method call (if any) corresponding to this data-flow node */ string getMethodName() { result = node.getExpr().(MethodCall).getMethodName() } /** Gets the number of arguments of this call. */ diff --git a/ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll b/ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll index 3dacd89b25e..ede99069213 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll @@ -379,7 +379,7 @@ class GraphqlFieldResolutionMethod extends Method, HTTP::Server::RequestHandler: result.(KeywordParameter).hasName(argDefn.getArgumentName()) or // TODO this will cause false positives because now *anything* in the **args - // param will be flagged as as RoutedParameter/RemoteFlowSource, but really + // param will be flagged as RoutedParameter/RemoteFlowSource, but really // only the hash keys corresponding to the defined arguments are user input // others could be things defined in the `:extras` keyword argument to the `argument` result instanceof HashSplatParameter // often you see `def field(**args)` From 1037c2b18213ed1efaf4113c2314417c8401b3ac Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 14 Jul 2022 13:30:12 +0200 Subject: [PATCH 5/6] all comments are alive --- ql/ql/src/codeql_ql/style/DeadCodeQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/ql/src/codeql_ql/style/DeadCodeQuery.qll b/ql/ql/src/codeql_ql/style/DeadCodeQuery.qll index 8974560fd59..e2eaff9c5ca 100644 --- a/ql/ql/src/codeql_ql/style/DeadCodeQuery.qll +++ b/ql/ql/src/codeql_ql/style/DeadCodeQuery.qll @@ -203,7 +203,7 @@ private AstNode classUnion() { private AstNode benign() { not result.getLocation().getFile().getExtension() = ["ql", "qll"] or // ignore dbscheme files - result instanceof BlockComment or + result instanceof Comment or not exists(result.toString()) or // <- invalid code // cached-stages pattern result.(Module).getAMember().(ClasslessPredicate).getName() = "forceStage" or From 625e37a0da4bfb057cd432ff3c31eadbee7bea48 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 14 Jul 2022 21:53:21 +0200 Subject: [PATCH 6/6] fix typo Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll index 74ba3569d20..79d9f85464e 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll @@ -699,7 +699,7 @@ private predicate exprToExprStep_nocfg(Expr fromExpr, Expr toExpr) { call.getTarget() = f and // AST dataflow treats a reference as if it were the referred-to object, while the dataflow // models treat references as pointers. If the return type of the call is a reference, then - // look for data flow the referred-to object, rather than the reference itself. + // look for data flow to the referred-to object, rather than the reference itself. if call.getType().getUnspecifiedType() instanceof ReferenceType then outModel.isReturnValueDeref() else outModel.isReturnValue()