From 0361b2e6e8e8038a9e0347fee7fd9f49603c8c3d Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Fri, 8 Dec 2023 10:05:53 +0100 Subject: [PATCH] QL4QL: Improvements to `RedundantImport` query --- ql/ql/src/codeql_ql/ast/Ast.qll | 15 +++++++ .../codeql_ql/style/RedundantImportQuery.qll | 40 +++++++++---------- .../test/queries/style/RedundantImport/D.qll | 2 + .../test/queries/style/RedundantImport/E.qll | 2 + .../RedundantImport/RedundantImport.expected | 1 + .../RedundantImport/RedundantImport.qlref | 1 + .../style/RedundantImport/folder/A.qll | 1 + .../style/RedundantImport/folder/B.qll | 1 + .../style/RedundantImport/folder/C.qll | 1 + 9 files changed, 42 insertions(+), 22 deletions(-) create mode 100644 ql/ql/test/queries/style/RedundantImport/D.qll create mode 100644 ql/ql/test/queries/style/RedundantImport/E.qll create mode 100644 ql/ql/test/queries/style/RedundantImport/RedundantImport.expected create mode 100644 ql/ql/test/queries/style/RedundantImport/RedundantImport.qlref create mode 100644 ql/ql/test/queries/style/RedundantImport/folder/A.qll create mode 100644 ql/ql/test/queries/style/RedundantImport/folder/B.qll create mode 100644 ql/ql/test/queries/style/RedundantImport/folder/C.qll diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index acc36be15eee..937c7bc61010 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -889,6 +889,9 @@ class ModuleMember extends TModuleMember, AstNode { /** Holds if this member is declared as `final`. */ predicate isFinal() { this.hasAnnotation("final") } + + /** Holds if this member is declared as `deprecated`. */ + predicate isDeprecated() { this.hasAnnotation("deprecated") } } private newtype TDeclarationKind = @@ -2738,6 +2741,18 @@ module YAML { ) } + /** + * Gets the language library file for this QLPack, if any. For example, the + * language library file for `codeql/cpp-all` is `cpp.qll`. + */ + File getLanguageLib() { + exists(string name | + name = this.getExtractor() and + result.getParentContainer() = this.getFile().getParentContainer() and + result.getBaseName() = name + ".qll" + ) + } + Location getLocation() { // hacky, just pick the first node in the file. result = diff --git a/ql/ql/src/codeql_ql/style/RedundantImportQuery.qll b/ql/ql/src/codeql_ql/style/RedundantImportQuery.qll index 4cb007aced18..1ed31d6c43b8 100644 --- a/ql/ql/src/codeql_ql/style/RedundantImportQuery.qll +++ b/ql/ql/src/codeql_ql/style/RedundantImportQuery.qll @@ -1,68 +1,64 @@ import ql +private import YAML +private import codeql_ql.ast.internal.Module -Import imports(Import imp) { +private FileOrModule getResolvedModule(Import imp) { + result = imp.getResolvedModule() and + // skip the top-level language files + not result.asFile() = any(QLPack p).getLanguageLib() +} + +private Import imports(Import imp) { ( exists(File file, TopLevel top | - imp.getResolvedModule().asFile() = file and + getResolvedModule(imp).asFile() = file and top.getLocation().getFile() = file and result = top.getAMember() ) or exists(Module mod | - imp.getResolvedModule().asModule() = mod and + getResolvedModule(imp).asModule() = mod and result = mod.getAMember() ) ) } -Import getAnImport(AstNode parent) { +private Import getAnImport(AstNode parent) { result = parent.(TopLevel).getAMember() or result = parent.(Module).getAMember() } -pragma[inline] -predicate importsFromSameFolder(Import a, Import b) { - exists(string base | - a.getImportString().regexpCapture("(.*)\\.[^\\.]*", 1) = base and - b.getImportString().regexpCapture("(.*)\\.[^\\.]*", 1) = base - ) - or - not a.getImportString().matches("%.%") and - not b.getImportString().matches("%.%") -} - predicate problem(Import imp, Import redundant, string message) { not exists(imp.importedAs()) and not exists(redundant.importedAs()) and not exists(imp.getModuleExpr().getQualifier*().getArgument(_)) and // any type-arguments, and we ignore, they might be different. // skip the top-level language files, they have redundant imports, and that's fine. - not exists(imp.getLocation().getFile().getParentContainer().getFile("qlpack.yml")) and + not imp.getLocation().getFile() = any(QLPack p).getLanguageLib() and // skip the DataFlowImpl.qll and similar, they have redundant imports in some copies. not imp.getLocation() .getFile() .getBaseName() .regexpMatch([".*Impl\\d?\\.qll", "DataFlowImpl.*\\.qll"]) and - // skip two imports that imports different things from the same folder. - not importsFromSameFolder(imp, redundant) and // if the redundant is public, and the imp is private, then the redundant might add things that are exported. not (imp.isPrivate() and not redundant.isPrivate()) and // Actually checking if the import is redundant: exists(AstNode parent | imp = getAnImport(parent) and - redundant = getAnImport(parent) and - redundant.getLocation().getStartLine() > imp.getLocation().getStartLine() + redundant = getAnImport(parent) | message = "Redundant import, the module is already imported inside $@." and // only looking for things directly imported one level down. Otherwise things gets complicated (lots of cycles). exists(Import inner | inner = imports(imp) | - redundant.getResolvedModule() = inner.getResolvedModule() and + getResolvedModule(redundant) = getResolvedModule(inner) and not inner.isPrivate() and // if the inner is private, then it's not propagated out. + not inner.isDeprecated() and not exists(inner.importedAs()) ) or message = "Duplicate import, the module is already imported by $@." and // two different import statements, that import the same thing - imp.getResolvedModule() = redundant.getResolvedModule() + getResolvedModule(imp) = getResolvedModule(redundant) and + redundant.getLocation().getStartLine() > imp.getLocation().getStartLine() ) } diff --git a/ql/ql/test/queries/style/RedundantImport/D.qll b/ql/ql/test/queries/style/RedundantImport/D.qll new file mode 100644 index 000000000000..1badf0ebbc54 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantImport/D.qll @@ -0,0 +1,2 @@ +import folder.A +import folder.B diff --git a/ql/ql/test/queries/style/RedundantImport/E.qll b/ql/ql/test/queries/style/RedundantImport/E.qll new file mode 100644 index 000000000000..4435151db00b --- /dev/null +++ b/ql/ql/test/queries/style/RedundantImport/E.qll @@ -0,0 +1,2 @@ +import folder.A +import folder.C diff --git a/ql/ql/test/queries/style/RedundantImport/RedundantImport.expected b/ql/ql/test/queries/style/RedundantImport/RedundantImport.expected new file mode 100644 index 000000000000..e2e5921942f0 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantImport/RedundantImport.expected @@ -0,0 +1 @@ +| D.qll:1:1:1:15 | Import | Redundant import, the module is already imported inside $@. | D.qll:2:1:2:15 | Import | folder.B | diff --git a/ql/ql/test/queries/style/RedundantImport/RedundantImport.qlref b/ql/ql/test/queries/style/RedundantImport/RedundantImport.qlref new file mode 100644 index 000000000000..a2ff992e5cd8 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantImport/RedundantImport.qlref @@ -0,0 +1 @@ +queries/style/RedundantImport.ql \ No newline at end of file diff --git a/ql/ql/test/queries/style/RedundantImport/folder/A.qll b/ql/ql/test/queries/style/RedundantImport/folder/A.qll new file mode 100644 index 000000000000..daca10250d8e --- /dev/null +++ b/ql/ql/test/queries/style/RedundantImport/folder/A.qll @@ -0,0 +1 @@ +predicate p() { any() } diff --git a/ql/ql/test/queries/style/RedundantImport/folder/B.qll b/ql/ql/test/queries/style/RedundantImport/folder/B.qll new file mode 100644 index 000000000000..9da260a695f5 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantImport/folder/B.qll @@ -0,0 +1 @@ +import A diff --git a/ql/ql/test/queries/style/RedundantImport/folder/C.qll b/ql/ql/test/queries/style/RedundantImport/folder/C.qll new file mode 100644 index 000000000000..c64cc7ebc293 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantImport/folder/C.qll @@ -0,0 +1 @@ +deprecated import A