JS: Refactor

Author: asgerf

javascript/ql/lib/semmle/javascript/endpoints/EndpointNaming.qll

@@ -167,24 +167,6 @@ predicate sinkHasPrimaryName(API::Node sink, string package, string name) {
   sinkHasPrimaryName(sink, package, name, _)
 }
 
-/**
- * Holds if `(package, name)` is an alias for `node`.
- *
- * This means it is a valid name for it, but was not chosen as the primary name.
- */
-private predicate sinkHasAlias(API::Node sink, string package, string name) {
-  not sinkHasPrimaryName(sink, package, name) and
-  (
-    exists(string baseName, string step |
-      sinkHasPrimaryName(getAPredecessor(sink, step, _), package, baseName) and
-      name = join(baseName, step)
-    )
-    or
-    sink = API::moduleExport(package) and
-    name = ""
-  )
-}
-
 /** Gets a source node that can flow to `sink` without using a return step. */
 private DataFlow::SourceNode nodeReachingSink(API::Node sink, DataFlow::TypeBackTracker t) {
   t.start() and
@@ -243,49 +225,8 @@ private predicate classObjectHasNameCandidate(
   )
   or
   nameFromExterns(cls, package, name, badness)
-}
-
-private predicate classObjectHasPrimaryName(
-  DataFlow::ClassNode cls, string package, string name, int badness
-) {
-  badness = min(int b | classObjectHasNameCandidate(cls, _, _, b) | b) and
-  package = min(string p | classObjectHasNameCandidate(cls, p, _, badness) | p) and
-  name = min(string n | classObjectHasNameCandidate(cls, package, n, badness) | n)
-}
-
-/** Holds if `(package, name)` is the primary name for the class object of `cls`. */
-predicate classObjectHasPrimaryName(DataFlow::ClassNode cls, string package, string name) {
-  classObjectHasPrimaryName(cls, package, name, _)
-}
-
-/** Holds if an instance of `cls` can be exposed to client code. */
-private predicate hasEscapingInstance(DataFlow::ClassNode cls) {
-  cls.getAnInstanceReference().flowsTo(any(API::Node n).asSink())
-}
-
-/**
- * Holds if `(package, name)` is a potential name to use for instances of `cls`, with the given `badness`.
- */
-private predicate classInstanceHasNameCandidate(
-  DataFlow::ClassNode cls, string package, string name, int badness
-) {
-  exists(string baseName |
-    classObjectHasPrimaryName(cls, package, baseName, badness) and
-    name = join(baseName, "prototype")
-  )
   or
-  // In case the class itself is unaccessible, but an instance is exposed via an access path,
-  // consider using that access path. For example:
-  //
-  //   class InternalClass {}
-  //   module.exports.foo = new InternalClass();
-  //
-  exists(int baseBadness |
-    sinkHasPrimaryName(getASinkNode(cls.getAnInstanceReference()), package, name, baseBadness) and
-    badness = baseBadness + 30 // add penalty, as we prefer to base this on the class name
-  )
-  or
-  // If neither the class nor its instances are accessible via an access path, but instances of the
+  // If the class is not accessible via an access path, but instances of the
   // class can still escape via more complex access patterns, resort to a synthesized name.
   // For example:
   //
@@ -297,62 +238,21 @@ private predicate classInstanceHasNameCandidate(
   hasEscapingInstance(cls) and
   exists(string baseName |
     InternalModuleNaming::fallbackModuleName(cls.getTopLevel(), package, baseName, badness - 100) and
-    name = join(baseName, cls.getName()) + ".prototype"
+    name = join(baseName, cls.getName())
   )
 }
 
-private predicate classInstanceHasPrimaryName(
-  DataFlow::ClassNode cls, string package, string name, int badness
-) {
-  badness = min(int b | classInstanceHasNameCandidate(cls, _, _, b) | b) and
-  package = min(string p | classInstanceHasNameCandidate(cls, p, _, badness) | p) and
-  name =
-    min(string n |
-      classInstanceHasNameCandidate(cls, package, n, badness)
-    |
-      n order by n.length(), n
-    )
-}
-
-/** Holds if `(package, name)` is the primary name to use for instances of `cls`. */
-predicate classInstanceHasPrimaryName(DataFlow::ClassNode cls, string package, string name) {
-  classInstanceHasPrimaryName(cls, package, name, _)
-}
-
-/** Holds if `(package, name)` is an alias referring to some instance of `cls`. */
-predicate classInstanceHasAlias(DataFlow::ClassNode cls, string package, string name) {
-  not classInstanceHasPrimaryName(cls, package, name) and
-  exists(int badness |
-    classInstanceHasNameCandidate(cls, package, name, badness) and
-    badness < 100 // Badness 100 is when we start to synthesize names. Do not suggest these as aliases.
-  )
-}
-
-private predicate functionHasNameCandidate(
-  DataFlow::FunctionNode function, string package, string name, int badness
-) {
-  sinkHasPrimaryName(getASinkNode(function), package, name, badness)
-  or
-  exists(DataFlow::ClassNode cls |
-    function = cls.getConstructor() and
-    classObjectHasPrimaryName(cls, package, name, badness)
-    or
-    exists(string baseName, string memberName |
-      function = cls.getStaticMethod(memberName) and
-      classObjectHasPrimaryName(cls, package, baseName, badness) and
-      name = join(baseName, memberName)
-    )
-  )
-  or
-  nameFromExterns(function, package, name, badness)
+/** Holds if an instance of `cls` can be exposed to client code. */
+private predicate hasEscapingInstance(DataFlow::ClassNode cls) {
+  cls.getAnInstanceReference().flowsTo(any(API::Node n).asSink())
 }
 
 private predicate sourceNodeHasNameCandidate(
   DataFlow::SourceNode node, string package, string name, int badness
 ) {
   sinkHasPrimaryName(getASinkNode(node), package, name, badness)
   or
-  functionHasNameCandidate(node, package, name, badness)
+  nameFromExterns(node, package, name, badness)
   or
   classObjectHasNameCandidate(node, package, name, badness)
 }
@@ -371,13 +271,18 @@ private predicate sourceNodeHasPrimaryName(
  * Holds if `node` is a function or a call that returns a function.
  */
 private predicate isFunctionSource(DataFlow::SourceNode node) {
-  node instanceof DataFlow::FunctionNode
-  or
-  node instanceof DataFlow::InvokeNode and
-  exists(node.getABoundFunctionValue(_)) and
-  // `getASinkNode` steps through imports (but not other calls) so exclude calls that are imports (i.e. require calls)
-  // as we want to get as close to the source as possible.
-  not node instanceof DataFlow::ModuleImportNode
+  exists(getASinkNode(node)) and
+  (
+    node instanceof DataFlow::FunctionNode
+    or
+    node instanceof DataFlow::ClassNode
+    or
+    node instanceof DataFlow::InvokeNode and
+    exists(node.getABoundFunctionValue(_)) and
+    // `getASinkNode` steps through imports (but not other calls) so exclude calls that are imports (i.e. require calls)
+    // as we want to get as close to the source as possible.
+    not node instanceof DataFlow::ModuleImportNode
+  )
 }
 
 /**
@@ -528,28 +433,6 @@ module Debug {
       )
   }
 
-  /** Holds if the given `node` has multiple primary names. */
-  query string ambiguousClassObjectName(DataFlow::ClassNode node) {
-    strictcount(string package, string name | classObjectHasPrimaryName(node, package, name)) > 1 and
-    result =
-      concat(string package, string name |
-        classObjectHasPrimaryName(node, package, name)
-      |
-        renderName(package, name), ", "
-      )
-  }
-
-  /** Holds if the given `node` has multiple primary names. */
-  query string ambiguousClassInstanceName(DataFlow::ClassNode node) {
-    strictcount(string package, string name | classInstanceHasPrimaryName(node, package, name)) > 1 and
-    result =
-      concat(string package, string name |
-        classInstanceHasPrimaryName(node, package, name)
-      |
-        renderName(package, name), ", "
-      )
-  }
-
   /** Holds if the given `node` has multiple primary names. */
   query string ambiguousFunctionName(DataFlow::FunctionNode node) {
     strictcount(string package, string name | functionHasPrimaryName(node, package, name)) > 1 and

javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected

@@ -1,13 +1,8 @@
 testFailures
-| pack11/index.ts:33:1:33:16 |  | Unexpected result: method=(pack11).C3.privateField |
-| pack11/index.ts:33:18:33:69 | // $ me ... ng.name | Missing result:method=(pack11).C3.publicField.really.long.name |
-| pack11/index.ts:41:23:41:24 |  | Unexpected result: alias=(pack11).C3.publicField.really.long.name==(pack11).C3.privateField |
-failures
 ambiguousPreferredPredecessor
 | pack2/lib.js:1:1:3:1 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getInstance() |
 | pack2/lib.js:8:22:8:34 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getMember("foo") |
 | pack2/main.js:1:1:3:1 | def moduleImport("pack2").getMember("exports").getMember("MainClass").getInstance() |
 ambiguousSinkName
-ambiguousClassObjectName
-ambiguousClassInstanceName
 ambiguousFunctionName
+failures

javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.ql

@@ -10,28 +10,16 @@ private predicate isIgnored(DataFlow::FunctionNode function) {
 }
 
 module TestConfig implements TestSig {
-  string getARelevantTag() { result = ["instance", "class", "method", "alias"] }
+  string getARelevantTag() { result = ["name", "alias"] }
 
   predicate hasActualResult(Location location, string element, string tag, string value) {
-    exists(string package, string name |
-      element = "" and
+    element = "" and
+    tag = "name" and
+    exists(DataFlow::SourceNode function, string package, string name |
+      EndpointNaming::functionHasPrimaryName(function, package, name) and
+      not isIgnored(function) and
+      location = function.getAstNode().getLocation() and
       value = EndpointNaming::renderName(package, name)
-    |
-      exists(DataFlow::ClassNode cls | location = cls.getAstNode().getLocation() |
-        tag = "class" and
-        EndpointNaming::classObjectHasPrimaryName(cls, package, name)
-        or
-        tag = "instance" and
-        EndpointNaming::classInstanceHasPrimaryName(cls, package, name)
-      )
-      or
-      exists(DataFlow::SourceNode function |
-        not isIgnored(function) and
-        location = function.getAstNode().getLocation() and
-        tag = "method" and
-        EndpointNaming::functionHasPrimaryName(function, package, name) and
-        not function instanceof DataFlow::ClassNode // reported with class tag
-      )
     )
     or
     element = "" and

javascript/ql/test/library-tests/EndpointNaming/pack1/main.js

@@ -1,13 +1,13 @@
-export class PublicClass {} // $ class=(pack1).PublicClass instance=(pack1).PublicClass.prototype
+export class PublicClass {} // $ name=(pack1).PublicClass
 
 class PrivateClass {}
 
-export const ExportedConst = class ExportedConstClass {} // $ class=(pack1).ExportedConst instance=(pack1).ExportedConst.prototype
+export const ExportedConst = class ExportedConstClass {} // $ name=(pack1).ExportedConst
 
-class ClassWithEscapingInstance {} // $ instance=(pack1).ClassWithEscapingInstance.prototype
+class ClassWithEscapingInstance {}
 
 export function getEscapingInstance() {
     return new ClassWithEscapingInstance();
-} // $ method=(pack1).getEscapingInstance
+} // $ name=(pack1).getEscapingInstance
 
-export function publicFunction() {} // $ method=(pack1).publicFunction
+export function publicFunction() {} // $ name=(pack1).publicFunction

javascript/ql/test/library-tests/EndpointNaming/pack10/foo.js

@@ -1 +1 @@
-export default class FooClass {} // $ class=(pack10).Foo instance=(pack10).Foo.prototype
+export default class FooClass {} // $ name=(pack10).Foo

javascript/ql/test/library-tests/EndpointNaming/pack11/index.ts

@@ -1,5 +1,5 @@
 const f1 = {
-    m() {} // $ method=(pack11).C1.publicField.really.long.name.m
+    m() {} // $ name=(pack11).C1.publicField.really.long.name.m
 };
 
 export class C1 {
@@ -12,10 +12,10 @@ export class C1 {
             }
         }
     }
-} // $ class=(pack11).C1 instance=(pack11).C1.prototype
+} // $ name=(pack11).C1
 
 const f2 = {
-    m() {} // $ method=(pack11).C2.publicField.really.long.name.m
+    m() {} // $ name=(pack11).C2.publicField.really.long.name.m
 }
 
 export class C2 {
@@ -28,9 +28,9 @@ export class C2 {
             }
         }
     }
-} // $ class=(pack11).C2 instance=(pack11).C2.prototype
+} // $ name=(pack11).C2
 
-function f3() {} // $ method=(pack11).C3.publicField.really.long.name
+function f3() {} // $ name=(pack11).C3.publicField.really.long.name
 
 export class C3 {
     private static privateField = f3;
@@ -42,11 +42,11 @@ export class C3 {
             }
         }
     }
-} // $ class=(pack11).C3 instance=(pack11).C3.prototype
+} // $ name=(pack11).C3
 
 
 const f4 = {
-    m() {} // $ method=(pack11).C4.really.long.name.m
+    m() {} // $ name=(pack11).C4.really.long.name.m
 };
 
 export const C4 = {

javascript/ql/test/library-tests/EndpointNaming/pack12/index.js

@@ -3,8 +3,8 @@ function wrap(fn) {
 }
 
 function f() {}
-export const f1 = wrap(f); // $ method=(pack12).f1
-export const f2 = wrap(f); // $ method=(pack12).f2
+export const f1 = wrap(f); // $ name=(pack12).f1
+export const f2 = wrap(f); // $ name=(pack12).f2
 
 function g() {}
-export const g1 = wrap(g); // $ method=(pack12).g1
+export const g1 = wrap(g); // $ name=(pack12).g1

javascript/ql/test/library-tests/EndpointNaming/pack2/lib.js

@@ -1,8 +1,8 @@
 class AmbiguousClass {
-    instanceMethod(foo) {} // $ method=(pack2).lib.LibClass.prototype.instanceMethod
-} // $ class=(pack2).lib.LibClass instance=(pack2).lib.LibClass.prototype
+    instanceMethod(foo) {} // $ name=(pack2).lib.LibClass.prototype.instanceMethod
+} // $ name=(pack2).lib.LibClass
 
 export default AmbiguousClass; // $ alias=(pack2).lib.default==(pack2).lib.LibClass
 export { AmbiguousClass as LibClass }
 
-AmbiguousClass.foo = function() {} // $ method=(pack2).lib.LibClass.foo
+AmbiguousClass.foo = function() {} // $ name=(pack2).lib.LibClass.foo

javascript/ql/test/library-tests/EndpointNaming/pack2/main.js

@@ -1,6 +1,6 @@
 class AmbiguousClass {
-    instanceMethod() {} // $ method=(pack2).MainClass.prototype.instanceMethod
-} // $ class=(pack2).MainClass instance=(pack2).MainClass.prototype
+    instanceMethod() {} // $ name=(pack2).MainClass.prototype.instanceMethod
+} // $ name=(pack2).MainClass
 
 export default AmbiguousClass; // $ alias=(pack2).default==(pack2).MainClass
 export { AmbiguousClass as MainClass }

javascript/ql/test/library-tests/EndpointNaming/pack3/lib.js

@@ -1 +1 @@
-export default function(x,y,z) {} // $ method=(pack3).libFunction alias=(pack3).libFunction.default==(pack3).libFunction
+export default function(x,y,z) {} // $ name=(pack3).libFunction alias=(pack3).libFunction.default==(pack3).libFunction