QL4QL: Improvements to RedundantImport query

Author: hvitved

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 =

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()
   )
 }

ql/ql/test/queries/style/RedundantImport/D.qll

@@ -0,0 +1,2 @@
+import folder.A
+import folder.B

ql/ql/test/queries/style/RedundantImport/E.qll

@@ -0,0 +1,2 @@
+import folder.A
+import folder.C

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 |

ql/ql/test/queries/style/RedundantImport/RedundantImport.qlref

@@ -0,0 +1 @@
+queries/style/RedundantImport.ql

ql/ql/test/queries/style/RedundantImport/folder/A.qll

@@ -0,0 +1 @@
+predicate p() { any() }

ql/ql/test/queries/style/RedundantImport/folder/B.qll

@@ -0,0 +1 @@
+import A

ql/ql/test/queries/style/RedundantImport/folder/C.qll

@@ -0,0 +1 @@
+deprecated import A