Migrate Java code to separate QL repo.

java/ql/src/Language Abuse/CastThisToTypeParameter.java

@@ -0,0 +1,14 @@
+public class CastThisToTypeParameter {
+	private abstract static class BadBaseNode<T extends BadBaseNode<T>> {
+		public abstract T getParent();
+
+		public T getRoot() {
+			// BAD: relies on derived types to use the right pattern
+			T cur = (T)this;
+			while(cur.getParent() != null) {
+				cur = cur.getParent();
+			}
+			return cur;
+		}
+	}
+}

java/ql/src/Language Abuse/CastThisToTypeParameter.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Casting <code>this</code> to a type parameter usually suggests that there is an implicit type constraint - the programmer probably wanted to express
+the notion that <code>this</code> could be converted to the type parameter (when using the enclosing method from derived types). 
+However, casting to the desired type, relies on derived types to ensure that the cast will succeed without the compiler forcing them
+to do so.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+The solution is to enforce the constraint by adding an abstract method on the base type (see example below). Each derived
+type must then implement this method, which makes the constraint checkable by the compiler and removes the need for a cast.
+</p>
+
+</recommendation>
+<example>
+<p>In this example <code>BadBaseNode</code> relies on derived types to use the right pattern.</p>
+<sample src="CastThisToTypeParameter.java" />
+<p>This constraint is better enforced by adding an abstract method on the base type. Implementing 
+this method makes the constraint checkable by the compiler.</p>
+<sample src="CastThisToTypeParameterFix.java" />
+</example>
+</qhelp>

java/ql/src/Language Abuse/CastThisToTypeParameter.ql

@@ -0,0 +1,27 @@
+/**
+ * @name Cast of 'this' to a type parameter
+ * @description Casting 'this' to a type parameter of the current type masks an implicit type constraint that should be explicitly stated.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id java/cast-of-this-to-type-parameter
+ * @tags reliability
+ *       maintainability
+ *       language-features
+ */
+
+import java
+
+from Expr e, GenericType src, TypeVariable dest
+where
+  exists(CastExpr cse |
+    cse = e and
+    exists(cse.getLocation()) and
+    cse.getExpr() instanceof ThisAccess and
+    src = cse.getExpr().getType() and
+    dest = cse.getType()
+  ) and
+  dest.getGenericType() = src
+select e, "Casting 'this' to $@, a type parameter of $@, masks an implicit type constraint that should be explicitly stated.",
+  dest, dest.getName(),
+  src, src.getName()

java/ql/src/Language Abuse/CastThisToTypeParameterFix.java

@@ -0,0 +1,52 @@
+public class CastThisToTypeParameter {
+	private abstract static class GoodBaseNode<T extends GoodBaseNode<T>> {
+		public abstract T getSelf();
+		public abstract T getParent();
+
+		public T getRoot() {
+			// GOOD: introduce an abstract method to enforce the constraint
+			// that 'this' can be converted to T for derived types
+			T cur = getSelf();
+			while(cur.getParent() != null)
+			{
+				cur = cur.getParent();
+			}
+			return cur;
+		}
+	}
+
+	private static class GoodConcreteNode extends GoodBaseNode<GoodConcreteNode> {
+		private String name;
+		private GoodConcreteNode parent;
+
+		public GoodConcreteNode(String name, GoodConcreteNode parent)
+		{
+			this.name = name;
+			this.parent = parent;
+		}
+
+		@Override
+		public GoodConcreteNode getSelf() {
+			return this;
+		}
+
+		@Override
+		public GoodConcreteNode getParent() {
+			return parent;
+		}
+
+		@Override
+		public String toString() {
+			return name;
+		}
+	}
+
+	public static void main(String[] args) {
+		GoodConcreteNode a = new GoodConcreteNode("a", null);
+		GoodConcreteNode b = new GoodConcreteNode("b", a);
+		GoodConcreteNode c = new GoodConcreteNode("c", a);
+		GoodConcreteNode d = new GoodConcreteNode("d", b);
+		GoodConcreteNode root = d.getRoot();
+		System.out.println(a + " " + root);
+	}
+}

java/ql/src/Language Abuse/ChainedInstanceof.java

@@ -0,0 +1,65 @@
+import java.util.*;
+
+public class ChainedInstanceof {
+	public static void main(String[] args) {
+		// BAD: example of a sequence of type tests
+		List<BadAnimal> badAnimals = new ArrayList<BadAnimal>();
+		badAnimals.add(new BadCat());
+		badAnimals.add(new BadDog());
+		for(BadAnimal a: badAnimals) {
+			if(a instanceof BadCat)      System.out.println("Miaow!");
+			else if(a instanceof BadDog) System.out.println("Woof!");
+			else                         throw new RuntimeException("Oops!");
+		}
+
+		// GOOD: solution using polymorphism
+		List<PolymorphicAnimal> polymorphicAnimals = new ArrayList<PolymorphicAnimal>();
+		polymorphicAnimals.add(new PolymorphicCat());
+		polymorphicAnimals.add(new PolymorphicDog());
+		for(PolymorphicAnimal a: polymorphicAnimals) a.speak();
+
+		// GOOD: solution using the visitor pattern
+		List<VisitableAnimal> visitableAnimals = new ArrayList<VisitableAnimal>();
+		visitableAnimals.add(new VisitableCat());
+		visitableAnimals.add(new VisitableDog());
+		for(VisitableAnimal a: visitableAnimals) a.accept(new SpeakVisitor());
+	}
+
+	//#################### TYPES FOR BAD EXAMPLE ####################
+
+	private interface BadAnimal {}
+	private static class BadCat implements BadAnimal {}
+	private static class BadDog implements BadAnimal {}
+
+	//#################### TYPES FOR POLYMORPHIC EXAMPLE ####################
+
+	private interface PolymorphicAnimal {
+		void speak();
+	}
+	private static class PolymorphicCat implements PolymorphicAnimal {
+		public void speak() { System.out.println("Miaow!"); }
+	}
+	private static class PolymorphicDog implements PolymorphicAnimal {
+		public void speak() { System.out.println("Woof!"); }
+	}
+
+	//#################### TYPES FOR VISITOR EXAMPLE ####################
+
+	private interface Visitor {
+		void visit(VisitableCat c);
+		void visit(VisitableDog d);
+	}
+	private static class SpeakVisitor implements Visitor {
+		public void visit(VisitableCat c) { System.out.println("Miaow!"); }
+		public void visit(VisitableDog d) { System.out.println("Woof!"); }
+	}
+	private interface VisitableAnimal {
+		void accept(Visitor v);
+	}
+	private static class VisitableCat implements VisitableAnimal {
+		public void accept(Visitor v) { v.visit(this); }
+	}
+	private static class VisitableDog implements VisitableAnimal {
+		public void accept(Visitor v) { v.visit(this); }
+	}
+}

java/ql/src/Language Abuse/ChainedInstanceof.qhelp

@@ -0,0 +1,83 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Long sequences of type tests are often used to dispatch control to different branches of the code based on the type
+of a variable, as shown in the example below. They are often used to simulate pattern-matching in languages that do
+not support it. Whilst this works as a dispatch method, there are a number of problems:
+</p>
+
+<ul>
+<li>
+They are difficult to maintain. It is easy to add a new subtype and forget to modify all of the type test sequences
+throughout your code.
+</li>
+
+<li>
+They introduce unwanted dependencies on concrete classes. Code cannot be written only in terms of an interface but 
+must instead be written considering all of the different special cases.
+</li>
+
+<li>
+They can be error-prone - it is easy to test for a base type before a derived type, resulting in a failure to 
+execute the code handling the derived type.
+</li>
+
+</ul>
+
+</overview>
+<recommendation>
+
+<p>
+There are a number of different possible solutions to this problem:
+</p>
+
+<ul>
+<li>
+<strong>Polymorphism</strong>. You can add a virtual method to the type hierarchy and put the segments of code to be called
+in the relevant override for each concrete class. This is a good solution when: (a) you can change the type hierarchy 
+and (b) the operation being implemented is core functionality that the types should implement. If you implement this 
+solution then you must be careful not to introduce unwanted dependencies. If the operation depends on entities
+that themselves depend on the type hierarchy, then you cannot move the operation to the type hierarchy without creating a dependency cycle.
+</li>
+
+<li>
+<strong>The visitor pattern</strong>. You can introduce a visitor interface containing a visit method for each type in
+the type hierarchy, and add an <code>accept</code> method to each type in the hierarchy that takes a visitor as its parameter.
+The <code>accept</code> method calls the visit method of the visitor on <code>this</code>. Concrete visitors then implement the 
+interface and process each specific type as necessary. This is a good solution when: (a) you can change the type hierarchy 
+and (b) the type hierarchy should not know about the operation being implemented (either to avoid dependency 
+or because it is not core functionality for the types in the hierarchy). It is also useful when you want to provide
+multiple operations with the same structure, on the same set of types, and you want the types themselves to control the way
+that the operation is structured. For example, "visit this tree using an in-order walk and apply the operation to each
+node". The basic visitor pattern is not suitable for all situations because it is cyclically-dependent, and the infrastructure involved
+is comparatively heavyweight.
+</li>
+
+<li>
+<strong>Reflection</strong>. You can look up one of a set of overloaded methods based on the type of one of the method
+parameters and invoke the method manually. This results in a loss of
+type safety and is rather untidy, but there are times when it is the best solution. In particular, reflection is useful when you
+cannot change the type hierarchy, for example, because it is third-party code.
+</li>
+</ul>
+
+</recommendation>
+<example>
+<p>The following example demonstrates the use "Polymorphism" and "The visitor pattern". More details on reflection can be found in [Flanagan].</p>
+
+<sample src="ChainedInstanceof.java" />
+
+</example>
+<references>
+
+
+  <li>D. Flanagan, <em>Java in a Nutshell: A Desktop Quick Reference</em>.  
+    O'Reilly Media, 1997.</li>
+    <li>E. Gamma, R. Helm, R. Johnson, J. Vlissides, <em>Design patterns: elements of reusable 
+    object-oriented software</em>. Addison-Wesley Longman Publishing Co., Inc. Boston, MA, 1995.</li>
+
+</references>
+</qhelp>

java/ql/src/Language Abuse/ChainedInstanceof.ql

@@ -0,0 +1,39 @@
+/**
+ * @name Chain of 'instanceof' tests
+ * @description Long sequences of type tests on a variable are difficult to maintain.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision high
+ * @id java/chained-type-tests
+ * @tags maintainability
+ *       language-features
+ */
+
+import java
+
+int instanceofCountForIfChain(IfStmt is) {
+  exists(int rest |
+    (
+      if is.getElse() instanceof IfStmt then
+        rest = instanceofCountForIfChain(is.getElse())
+      else
+        rest = 0
+    )
+    and
+    (
+      if is.getCondition() instanceof InstanceOfExpr then
+        result = 1 + rest
+      else
+        result = rest
+    )
+  )
+}
+
+from IfStmt is, int n
+where
+  n = instanceofCountForIfChain(is) and
+  n > 5 and
+  not exists(IfStmt other | is = other.getElse())
+select is,
+  "This if block performs a chain of " + n +
+  " type tests - consider alternatives, e.g. polymorphism or the visitor pattern."

java/ql/src/Language Abuse/DubiousDowncastOfThis.java

@@ -0,0 +1,20 @@
+public class DubiousDowncastOfThis {
+	private static class BadBase {
+		private Derived d;
+
+		public BadBase(Derived d) {
+			if(d != null && this instanceof Derived)
+				this.d = (Derived)this;		// violation
+			else
+				this.d = d;
+		}
+	}
+
+	private static class Derived extends BadBase {
+		public Derived() {
+			super(null);
+		}
+	}
+
+	public static void main(String[] args) {}
+}

java/ql/src/Language Abuse/DubiousDowncastOfThis.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Downcasting <code>this</code> to a derived type creates a dependency cycle. Derived types already depend 
+on their parent type and the cast creates a dependency in the other direction.
+</p>
+
+<p>Dependency cycles should be avoided as they make code both difficult to read and difficult to test. 
+In addition, a type should not know about its specific descendants, even though it may impose some
+constraints on them as a group (for example, abstract classes may require every derived type to implement 
+a method with a specific signature).</p>
+
+</overview>
+<recommendation>
+<p>
+The base and derived types should be redesigned so that there is no need for the base type to depend 
+on the types deriving from it.</p>
+
+</recommendation>
+<example>
+<p>In this example, <code>BadBase</code> introduces a dependency cycle with <code>Derived</code> by 
+coercing the type of <code>this</code> to a derived type. </p>
+
+<sample src="DubiousDowncastOfThis.java" />
+</example>
+</qhelp>

java/ql/src/Language Abuse/DubiousDowncastOfThis.ql

@@ -0,0 +1,29 @@
+/**
+ * @name Dubious downcast of 'this'
+ * @description Casting 'this' to a derived type introduces a dependency cycle
+ *              between the type of 'this' and the target type.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id java/downcast-of-this
+ * @tags testability
+ *       maintainability
+ *       language-features
+ */
+
+import java
+
+from Expr e, RefType src, RefType dest
+where
+  exists(CastExpr cse | cse = e |
+    exists(cse.getLocation()) and
+    cse.getExpr() instanceof ThisAccess and
+    src = cse.getExpr().getType() and
+    dest = cse.getType()
+  ) and
+  src.hasSubtype*(dest) and
+  src != dest and
+  not dest instanceof TypeVariable
+select e, "Downcasting 'this' from $@ to $@ introduces a dependency cycle between the two types.",
+  src, src.getName(),
+  dest, dest.getName()

java/ql/src/Language Abuse/DubiousTypeTestOfThis.java

@@ -0,0 +1,20 @@
+public class DubiousTypeTestOfThis {
+	private static class BadBase {
+		private Derived d;
+
+		public BadBase(Derived d) {
+			if(d != null && this instanceof Derived)	// violation
+				this.d = (Derived)this;
+			else
+				this.d = d;
+		}
+	}
+
+	private static class Derived extends BadBase {
+		public Derived() {
+			super(null);
+		}
+	}
+
+	public static void main(String[] args) {}
+}

java/ql/src/Language Abuse/DubiousTypeTestOfThis.qhelp

@@ -0,0 +1,30 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Testing whether <code>this</code> is an instance of a derived type creates a dependency cycle. 
+Derived types already depend on their parent type and the cast creates a dependency in the other direction.
+</p>
+
+<p>Dependency cycles should be avoided as they make code both difficult to read and difficult to test. 
+In addition, a type should not know about its specific descendants, even though it may impose some
+constraints on them as a group (for example, the need for every derived type to implement a method 
+with a specific signature).</p>
+
+</overview>
+<recommendation>
+
+<p>
+The base and derived types should be redesigned so that there is no need for the base type to depend 
+on the types deriving from it.</p>
+
+</recommendation>
+<example>
+<p>In this example, <code>BadBase</code> introduces a dependency cycle with <code>Derived</code> by 
+testing the type of <code>this</code>.</p>
+
+<sample src="DubiousTypeTestOfThis.java" />
+</example>
+</qhelp>

java/ql/src/Language Abuse/DubiousTypeTestOfThis.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Dubious type test of 'this'
+ * @description Testing whether 'this' is an instance of a derived type introduces
+ *              a dependency cycle between the type of 'this' and the target type.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id java/type-test-of-this
+ * @tags testability
+ *       maintainability
+ *       language-features
+ */
+
+import java
+
+from InstanceOfExpr ioe, RefType t, RefType ct
+where
+  ioe.getExpr() instanceof ThisAccess and
+  t = ioe.getExpr().getType() and
+  ct = ioe.getTypeName().getType() and
+  ct.getASupertype*() = t
+select ioe, "Testing whether 'this' is an instance of $@ in $@ introduces a dependency cycle between the two types.",
+  ct, ct.getName(),
+  t, t.getName()

java/ql/src/Language Abuse/EmptyStatement.java

@@ -0,0 +1,8 @@
+public class Cart {
+	// AVOID: Empty statement
+	List<Item> items = new ArrayList<Cart>();;
+	public void applyDiscount(float discount) {
+		// AVOID: Empty statement as loop body
+		for (int i = 0; i < items.size(); items.get(i++).applyDiscount(discount));
+	}
+}

java/ql/src/Language Abuse/EmptyStatement.qhelp

@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>An <em>empty statement</em> is a single semicolon <code>;</code> that does not
+terminate another statement. Such a statement hinders readability and has no effect on its own.</p>
+
+</overview>
+<recommendation>
+
+<p>Avoid empty statements. If a loop is intended to have an empty body, it is better
+to mark that fact explicitly by using a pair of braces <code>{}</code> containing an explanatory comment
+for the body, rather than a single semicolon.</p>
+
+</recommendation>
+<example>
+
+<p>In the following example, there is an empty statement on line 3, where an additional semicolon is
+used. On line 6, the <code>for</code> statement has an empty body because the condition is
+immediately followed by a semicolon. In this case, it is better to include a pair of braces <code>{}</code> containing
+an explanatory comment for the body instead.
+
+</p><sample src="EmptyStatement.java" />
+
+</example>
+<references>
+
+
+<li>
+Help - Eclipse Platform:
+<a href="http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fpreferences%2Fjava%2Fcompiler%2Fref-preferences-errors-warnings.htm">Java Compiler Errors/Warnings Preferences</a>.
+</li>
+
+
+</references>
+</qhelp>

java/ql/src/Language Abuse/EmptyStatement.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Empty statement
+ * @description An empty statement hinders readability.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision low
+ * @id java/empty-statement
+ * @tags maintainability
+ *       useless-code
+ */
+
+import java
+
+from EmptyStmt empty, string action
+where
+  if exists(LoopStmt l | l.getBody() = empty) then (
+    action = "turned into '{}'"
+  ) else (
+    action = "deleted"
+  )
+select empty, "This empty statement should be " + action + "."

java/ql/src/Language Abuse/EnumIdentifier.java

@@ -0,0 +1,7 @@
+class Old
+{
+	public static void main(String[] args) {
+		int enum = 13;  // AVOID: 'enum' is a variable.
+		System.out.println("The value of enum is " + enum);
+	}
+}

java/ql/src/Language Abuse/EnumIdentifier.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>Enumerations, or enums, were introduced in Java 5, with the keyword
+<code>enum</code>. Code written before this may use <code>enum</code> as
+an identifier. To compile such code, you must compile it with a command
+such as <code>javac -source 1.4 ...</code>. However, this means that you 
+cannot use any new features that are provided in Java 5 and later.</p>
+
+</overview>
+<recommendation>
+
+<p>To make it easier to compile the code and add code that uses new Java features,
+rename any identifiers that are named <code>enum</code> in legacy code.</p>
+
+</recommendation>
+<example>
+
+<p>In the following example, <code>enum</code> is used as the name of a variable. This means that the
+code does not compile unless the compiler's source language is set to 1.4 or earlier. To avoid this
+constraint, the variable should be renamed.</p>
+
+<sample src="EnumIdentifier.java" />
+
+</example>
+<references>
+
+
+<li>
+Java Language Specification:
+<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.9">8.9 Enums</a>.
+</li>
+
+
+</references>
+</qhelp>

java/ql/src/Language Abuse/EnumIdentifier.ql

@@ -0,0 +1,21 @@
+/**
+ * @name String 'enum' used as identifier
+ * @description Using 'enum' as an identifier makes the code incompatible with Java 5 and later.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id java/enum-identifier
+ * @tags portability
+ *       readability
+ *       naming
+ */
+
+import java
+
+Element elementNamedEnum() {
+  result.(CompilationUnit).getPackage().getName().regexpMatch("(.*\\.|)enum(\\..*|)")
+  or
+  result.getName() = "enum"
+}
+
+select elementNamedEnum(), "Code using 'enum' as an identifier will not compile with a recent version of Java."

java/ql/src/Language Abuse/ImplementsAnnotation.java

@@ -0,0 +1,3 @@
+public abstract class ImplementsAnnotation implements Deprecated {
+	// ...
+}

java/ql/src/Language Abuse/ImplementsAnnotation.qhelp

@@ -0,0 +1,52 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>Although an annotation type is a special kind of interface that
+can be implemented by a concrete class, this is not its intended use.
+It is more likely that an annotation type should be used to annotate a class.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that any annotations are used to annotate a class, unless they are really supposed to be 
+extended or implemented by the class.
+</p>
+
+</recommendation>
+<example>
+
+<p>In the following example, the annotation <code>Deprecated</code> is implemented by the class
+<code>ImplementsAnnotation</code>.</p>
+
+<sample src="ImplementsAnnotation.java" />
+
+<p>The following example shows the intended use of annotations: to annotate the class
+<code>ImplementsAnnotationFix</code>.</p>
+
+<sample src="ImplementsAnnotationGood.java" />
+
+</example>
+<references>
+
+
+<li>
+  The Java Language Specification:
+  <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.6">Annotation Types</a>.
+</li>
+<li>
+  The Java Tutorials:
+  <a href="http://docs.oracle.com/javase/tutorial/java/annotations/index.html">Annotations</a>.
+</li>
+<li>
+Help - Eclipse Platform:
+<a href="http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fpreferences%2Fjava%2Fcompiler%2Fref-preferences-errors-warnings.htm">Java Compiler Errors/Warnings Preferences</a>.
+</li>
+
+
+</references>
+</qhelp>

java/ql/src/Language Abuse/ImplementsAnnotation.ql

@@ -0,0 +1,17 @@
+/**
+ * @name Annotation is extended or implemented
+ * @description Extending or implementing an annotation is unlikely to be what the programmer intends.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id java/annotation-subtype
+ * @tags maintainability
+ *       correctness
+ *       logic
+ */
+
+import java
+
+from RefType type, AnnotationType annotation
+where type.getASupertype() = annotation
+select type, "Should this class be annotated by '" + annotation.getName() + "', not have it as a super-type?"

java/ql/src/Language Abuse/ImplementsAnnotationGood.java

@@ -0,0 +1,4 @@
+@Deprecated
+public abstract class ImplementsAnnotationFix {
+	// ...
+}

java/ql/src/Language Abuse/IterableClass.qll

@@ -0,0 +1,18 @@
+import java
+
+/** A class that implements `java.lang.Iterable`. */
+class Iterable extends Class {
+  Iterable() {
+    isSourceDeclaration() and
+    getASourceSupertype+().hasQualifiedName("java.lang", "Iterable")
+  }
+
+  /** The return value of a one-statement `iterator()` method. */
+  Expr simpleIterator() {
+    exists(Method m |
+      m.getDeclaringType().getSourceDeclaration() = this and
+      m.getName() = "iterator" and
+      m.getBody().(SingletonBlock).getStmt().(ReturnStmt).getResult() = result
+    )
+  }
+}

java/ql/src/Language Abuse/IterableIterator.qhelp

@@ -0,0 +1,68 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<include src="IterableOverview.qhelp" />
+
+<recommendation>
+<p>
+When working with custom implementations of <code>Iterator&lt;T&gt;</code> it
+is easy to add <code>implements Iterable&lt;T&gt;</code> and a simple
+<code>return this;</code> implementation of <code>iterator()</code> to support
+the for-each syntax.  This can, however, hide subtle bugs and is
+therefore not recommended.  It is better to separate the two and use a main
+representation that only implements <code>Iterable&lt;T&gt;</code> without
+containing any iteration state.  This object can then return a short-lived
+<code>Iterator&lt;T&gt;</code> each time it needs to be traversed.
+</p>
+<p>
+If this refactoring is undesirable for some reason, then the
+<code>iterator()</code> method should at the very least throw an exception if
+called more than once.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+The following example does not distinguish the iterable from its iterator, and
+therefore causes the second loop to terminate immediately without any effect.
+</p>
+<sample src="IterableIteratorBad.java" />
+
+<p>
+The best solution is a refactoring along the following lines where
+<code>Iterable</code> classes are used to pass around references to data.  This
+allows the <code>Iterator</code> instances to be short-lived and avoids the
+sharing of iteration state.
+</p>
+<sample src="IterableIteratorGood1.java" />
+
+<p>
+If a refactoring, as described above, is too cumbersome or is otherwise
+undesirable, then a guard can be inserted, as shown below.  Using a guard
+ensures that multiple iteration fails early, making it easier to find any related bugs.
+This solution is less ideal than the
+refactoring above, but nevertheless an improvement over the original.
+</p>
+<sample src="IterableIteratorGood2.java" />
+
+</example>
+
+<references>
+
+<li>
+The Java Language Specification:
+<a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-14.html#jls-14.14.2">The enhanced for statement</a>.
+</li>
+<li>
+The Java API Specification:
+<a href="https://docs.oracle.com/javase/8/docs/api/java/lang/Iterable.html">Interface Iterable&lt;T&gt;</a>,
+<a href="https://docs.oracle.com/javase/8/docs/api/java/util/Iterator.html">Interface Iterator&lt;T&gt;</a>,
+<a href="https://docs.oracle.com/javase/8/docs/api/java/nio/file/DirectoryStream.html">Interface DirectoryStream&lt;T&gt;</a>.
+</li>
+
+</references>
+
+</qhelp>

java/ql/src/Language Abuse/IterableIterator.ql

@@ -0,0 +1,38 @@
+/**
+ * @name Iterator implementing Iterable
+ * @description An 'Iterator' that also implements 'Iterable' by returning itself as its 'Iterator'
+ *              does not support multiple traversals. This can lead to unexpected behavior when
+ *              it is viewed as an 'Iterable'.
+ * @kind problem
+ * @problem.severity warning
+ * @precision very-high
+ * @id java/iterator-implements-iterable
+ * @tags correctness
+ *       reliability
+ */
+import java
+import IterableClass
+
+/** An `Iterable` that is also its own `Iterator`. */
+class IterableIterator extends Iterable {
+  IterableIterator() {
+    simpleIterator() instanceof ThisAccess
+  }
+}
+
+/** An `IterableIterator` that never returns any elements. */
+class EmptyIterableIterator extends IterableIterator {
+  EmptyIterableIterator() {
+    exists(Method m |
+      m.getDeclaringType().getSourceDeclaration() = this and
+      m.getName() = "hasNext" and
+      m.getBody().(SingletonBlock).getStmt().(ReturnStmt).getResult().(BooleanLiteral).getBooleanValue() = false
+    )
+  }
+}
+
+from IterableIterator i
+where
+  // Exclude the empty iterator as that is safe to reuse.
+  not i instanceof EmptyIterableIterator
+select i, "This Iterable is its own Iterator, but does not guard against multiple iterations."

java/ql/src/Language Abuse/IterableIteratorBad.java

@@ -0,0 +1,26 @@
+class ElemIterator implements Iterator<MyElem>, Iterable<MyElem> {
+  private MyElem[] data;
+  private idx = 0;
+
+  public boolean hasNext() {
+    return idx < data.length;
+  }
+  public MyElem next() {
+    return data[idx++];
+  }
+  public Iterator<MyElem> iterator() {
+    return this;
+  }
+  // ...
+}
+
+void useMySequence(Iterable<MyElem> s) {
+  // do some work by traversing the sequence
+  for (MyElem e : s) {
+    // ...
+  }
+  // do some more work by traversing it again
+  for (MyElem e : s) {
+    // ...
+  }
+}

java/ql/src/Language Abuse/IterableIteratorGood1.java

@@ -0,0 +1,16 @@
+class ElemSequence implements Iterable<MyElem> {
+  private MyElem[] data;
+
+  public Iterator<MyElem> iterator() {
+    return new Iterator<MyElem>() {
+      private idx = 0;
+      public boolean hasNext() {
+        return idx < data.length;
+      }
+      public MyElem next() {
+        return data[idx++];
+      }
+    };
+  }
+  // ...
+}

java/ql/src/Language Abuse/IterableIteratorGood2.java

@@ -0,0 +1,19 @@
+class ElemIterator implements Iterator<MyElem>, Iterable<MyElem> {
+  private MyElem[] data;
+  private idx = 0;
+  private boolean usedAsIterable = false;
+
+  public boolean hasNext() {
+    return idx < data.length;
+  }
+  public MyElem next() {
+    return data[idx++];
+  }
+  public Iterator<MyElem> iterator() {
+    if (usedAsIterable || idx > 0)
+      throw new IllegalStateException();
+    usedAsIterable = true;
+    return this;
+  }
+  // ...
+}

java/ql/src/Language Abuse/IterableOverview.qhelp

@@ -0,0 +1,33 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Java has two interfaces for dealing with iteration,
+<code>Iterable&lt;T&gt;</code> and <code>Iterator&lt;T&gt;</code>.  An
+<code>Iterable&lt;T&gt;</code> represents a sequence of elements that can be
+traversed, and an <code>Iterator&lt;T&gt;</code> represents the
+<strong>state</strong> of an ongoing traversal.  As an example, all the
+<code>Collection&lt;T&gt;</code> classes in the Java standard library implement
+<code>Iterable&lt;T&gt;</code>.  Comparing this to a traditional
+<code>for</code> loop that increments an integer index and iterates over the
+elements of an array, then the <code>Iterable&lt;T&gt;</code> object
+corresponds to the array, whereas the <code>Iterator&lt;T&gt;</code> object
+corresponds to the index variable.
+</p>
+<p>
+Implementations of <code>Iterable&lt;T&gt;</code> are generally expected to
+support multiple traversals of the element sequence they represent, although
+there can be exceptions if the underlying data somehow makes this undesirable,
+see for example <code>DirectoryStream&lt;T&gt;</code>.  If an implementation of
+<code>Iterable&lt;T&gt;</code> does not support multiple iterations, then its
+<code>iterator()</code> method should throw an exception on its second and
+subsequent calls.  This makes bugs easier to find if such an
+<code>Iterable&lt;T&gt;</code> is used more than once, for example in two
+different for-each loops.
+</p>
+</overview>
+
+</qhelp>

java/ql/src/Language Abuse/MissedTernaryOpportunity.java

@@ -0,0 +1,30 @@
+public class MissedTernaryOpportunity {
+	private static int myAbs1(int x) {
+		// Violation
+		if(x >= 0)
+			return x;
+		else
+			return -x;
+	}
+
+	private static int myAbs2(int x) {
+		// Better
+		return x >= 0 ? x : -x;
+	}
+
+	public static void main(String[] args) {
+		int i = 23;
+
+		// Violation
+		String s1;
+		if(i == 23)
+			s1 = "Foo";
+		else
+			s1 = "Bar";
+		System.out.println(s1);
+
+		// Better
+		String s2 = i == 23 ? "Foo" : "Bar";
+		System.out.println(s2);
+	}
+}

java/ql/src/Language Abuse/MissedTernaryOpportunity.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+An <code>if</code> statement where both branches do nothing but return or write to a variable can be 
+better expressed using the ternary <code>?</code> operator.
+</p><p>
+
+</p><p>
+Use of the ternary operator enhances readability in two ways:</p>
+<ul>
+<li>
+It focuses the reader's attention on the intent of the code (to return or write) rather than the
+testing of a condition.</li>
+<li>
+It is more concise, reducing the amount of code that needs to be read.</li>
+<li>
+You can initialize a variable conditionally on the line on which it is declared, rather than 
+assigning to it after initialization. This ensures that you initialize the variable as you intended.</li>
+</ul>
+
+</overview>
+<recommendation>
+<p>Consider using a ternary operator in this situation.</p>
+
+</recommendation>
+<example>
+<p>The following code includes two examples of <code>if</code> statements, <code>myAbs1</code> and 
+<code>1</code>, which can be simplified using the ternary operator. <code>myAbs2</code> and <code>s2</code> 
+show how the statements can be improved.</p>
+<sample src="MissedTernaryOpportunity.java" />
+</example>
+</qhelp>

java/ql/src/Language Abuse/MissedTernaryOpportunity.ql

@@ -0,0 +1,74 @@
+/**
+ * @name Missed ternary opportunity
+ * @description An 'if' statement where both branches either
+ *             (a) return or (b) write to the same variable
+ *             can often be expressed more clearly using the '?' operator.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision low
+ * @id java/missed-ternary-operator
+ * @tags maintainability
+ *       language-features
+ */
+
+import java
+
+predicate complicatedBranch(Stmt branch) {
+  exists(ConditionalExpr ce |
+    ce.getParent*() = branch
+  ) or
+  count(MethodAccess a | a.getParent*() = branch) > 1
+}
+
+predicate complicatedCondition(Expr cond) {
+  exists(Expr e | e = cond.getAChildExpr*() |
+    e instanceof AndLogicalExpr or
+    e instanceof OrLogicalExpr
+  )
+}
+
+predicate toCompare(Expr left, Expr right) {
+  exists(IfStmt is, AssignExpr at, AssignExpr ae |
+    at.getParent() = is.getThen() and
+    ae.getParent() = is.getElse()
+    |
+    left = at.getDest() and right = ae.getDest() or
+    left = at.getDest().(VarAccess).getQualifier() and right = ae.getDest().(VarAccess).getQualifier()
+  )
+}
+
+predicate sameVariable(VarAccess left, VarAccess right) {
+  toCompare(left, right) and
+  left.getVariable() = right.getVariable() and
+  (
+    exists(Expr q1, Expr q2 |
+      left.getQualifier() = q1 and
+      sameVariable(q1, q2) and
+      right.getQualifier() = q2
+    ) or
+    left.isLocal() and right.isLocal()
+  )
+}
+
+from IfStmt is, string what
+where
+  (
+    is.getThen() instanceof ReturnStmt and
+    is.getElse() instanceof ReturnStmt and
+    what = "return"
+    or
+    exists(AssignExpr at, AssignExpr ae |
+      at.getParent() = is.getThen() and
+      ae.getParent() = is.getElse() and
+      sameVariable(at.getDest(), ae.getDest()) and
+      what = "write to the same variable"
+    )
+  ) and
+  // Exclusions.
+  not (
+    exists(IfStmt other | is = other.getElse()) or
+    complicatedCondition(is.getCondition()) or
+    complicatedBranch(is.getThen()) or
+    complicatedBranch(is.getElse())
+  )
+select is, "Both branches of this 'if' statement " + what + " - consider using '?' to express intent better."

java/ql/src/Language Abuse/OverridePackagePrivate.java

@@ -0,0 +1,29 @@
+// File 1
+package gui;
+
+abstract class Widget
+{
+    // ...
+
+    // Return the width (in pixels) of this widget
+    int width() {
+        // ...
+    }
+
+    // ...
+}
+
+// File 2
+package gui.extras;
+
+class PhotoResizerWidget extends Widget
+{
+    // ...
+ 
+    // Return the new width (of the photo when resized)
+    public int width() {
+        // ...
+    }
+   
+    // ...
+}

java/ql/src/Language Abuse/OverridePackagePrivate.qhelp

@@ -0,0 +1,64 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+If a method is declared with default access (that is, not
+private, protected, nor public), it can
+only be overridden by methods in the same package. If a method of the same
+signature is defined in a subclass in a different package, it is a
+completely separate method and no overriding occurs.
+</p>
+
+<p>
+Code like this can be confusing for other programmers, who have to
+understand that there is no overriding relation, check that the
+original programmer did not intend one method to override the other, and 
+avoid mixing up the two methods by accident.
+</p>
+
+</overview>
+<recommendation>
+
+<p>In cases where there is intentionally no overriding, the best solution is to rename one or both 
+of the methods to clarify their different purposes.</p>
+
+<p>If one method is supposed to override another method that is declared with default access in
+another package, the access of the method must be changed to
+<code>public</code> or <code>protected</code>. Alternatively, the classes must be moved to
+the same package.</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example, <code>PhotoResizerWidget.width</code> does not override 
+<code>Widget.width</code> because one method is in package <code>gui</code> and one method is in
+package <code>gui.extras</code>.
+</p>
+
+<sample src="OverridePackagePrivate.java" />
+
+<p>Assuming that no overriding is intentional, one or both of the methods should be renamed. For 
+example, <code>PhotoResizerWidget.width</code> would be better named 
+<code>PhotoResizerWidget.newPhotoWidth</code>.</p>
+
+</example>
+<references>
+
+
+<li>
+Help - Eclipse Platform:
+<a href="http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fpreferences%2Fjava%2Fcompiler%2Fref-preferences-errors-warnings.htm">Java Compiler Errors/Warnings Preferences</a>.
+</li>
+<li>
+Java Language Specification:
+<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.8.1">8.4.8.1 Overriding (by Instance Methods)</a>.
+</li>
+
+
+</references>
+</qhelp>

java/ql/src/Language Abuse/OverridePackagePrivate.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Confusing non-overriding of package-private method
+ * @description A method that appears to override another method but does not, because the
+ *              declaring classes are in different packages, is potentially confusing.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id java/non-overriding-package-private
+ * @tags maintainability
+ *       readability
+ */
+
+import java
+
+from Method superMethod, Method method
+where
+  overridesIgnoringAccess(method, _, superMethod, _) and
+  not method.overrides(superMethod) and
+  not superMethod.isPublic() and
+  not superMethod.isProtected() and
+  not superMethod.isPrivate()
+select method, "This method does not override $@ because it is private to another package.",
+  superMethod,
+  superMethod.getDeclaringType().getName() + "." + superMethod.getName()

java/ql/src/Language Abuse/TypeVarExtendsFinalType.java

@@ -0,0 +1,7 @@
+class Printer
+{
+	void print(List<? extends String> strings) {  // Unnecessary wildcard
+		for (String s : strings)
+			System.out.println(s);
+	}
+}

java/ql/src/Language Abuse/TypeVarExtendsFinalType.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>A type wildcard with an <code>extends</code> clause (for example
+<code>?&nbsp;extends&nbsp;String</code>) implicitly suggests that a type (in
+this case <code>String</code>) has subclasses. If the type in the <code>extends</code>
+clause is final, the code is confusing because a final class cannot have 
+any subclasses. The only type that satisfies <code>?&nbsp;extends&nbsp;String</code> is 
+<code>String</code>.</p>
+
+</overview>
+<recommendation>
+
+<p>To make the code more readable, omit the wildcard to leave just the final type.</p>
+
+</recommendation>
+<example>
+
+<p>In the following example, a wildcard is used to refer to any type that is a subclass of
+<code>String</code>.</p>
+
+<sample src="TypeVarExtendsFinalType.java" />
+
+<p>However, because <code>String</code> is declared <code>final</code>, it does not have any
+subclasses. Therefore, it is clearer to replace <code>?&nbsp;extends&nbsp;String</code> with 
+<code>String</code>.</p>
+
+</example>
+<references>
+
+
+<li>
+Help - Eclipse Platform:
+<a href="http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fpreferences%2Fjava%2Fcompiler%2Fref-preferences-errors-warnings.htm">Java Compiler Errors/Warnings Preferences</a>.
+</li>
+<li>
+Java Language Specification:
+<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html#jls-4.5.1">4.5.1 Type Arguments and Wildcards</a>,
+<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.1.1.2">8.1.1.2 final Classes</a>.
+</li>
+
+
+</references>
+</qhelp>

java/ql/src/Language Abuse/TypeVarExtendsFinalType.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Type bound extends a final class
+ * @description If 'C' is a final class, a type bound such as '? extends C'
+ *              is confusing because it implies that 'C' has subclasses, but
+ *              a final class has no subclasses.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id java/type-bound-extends-final
+ * @tags maintainability
+ *       readability
+ *       types
+ */
+
+import java
+
+from TypeVariable v, RefType bound
+where
+  v.getATypeBound().getType() = bound and
+  bound.isFinal()
+select v, "Type '" + bound + "' is final, so <" + v.getName() + " extends " + bound + "> is confusing."

java/ql/src/Language Abuse/TypeVariableHidesType.java

@@ -0,0 +1,7 @@
+import java.util.Map;
+import java.util.Map.Entry;
+
+class Mapping<Key, Entry>  // The type variable 'Entry' shadows the imported interface 'Entry'.
+{
+	// ...
+}

java/ql/src/Language Abuse/TypeVariableHidesType.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>Type shadowing occurs if two types have the same name but one is defined within
+the scope of the other. This can arise if you introduce a type variable
+with the same name as an imported class.</p>
+
+<p>Type shadowing may cause the two types to be confused, which can lead to various problems.</p>
+
+</overview>
+<recommendation>
+
+<p>Name the type variable so that its name does not clash with the
+imported class.</p>
+
+</recommendation>
+<example>
+
+<p>In the following example, the type <code>java.util.Map.Entry</code> is imported at the top of the 
+file, but the class <code>Mapping</code> is defined with two type variables,
+<code>Key</code> and <code>Entry</code>. Uses of <code>Entry</code> within the
+<code>Mapping</code> class refer to the type variable, and not the imported
+interface. The type variable therefore shadows <code>Map.Entry</code>.</p>
+
+<sample src="TypeVariableHidesType.java" />
+
+<p>To fix the code, the type variable <code>Entry</code> on line 4 should be renamed.</p>
+
+</example>
+<references>
+
+
+<li>
+Help - Eclipse Platform:
+<a href="http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fpreferences%2Fjava%2Fcompiler%2Fref-preferences-errors-warnings.htm">Java Compiler Errors/Warnings Preferences</a>.
+</li>
+<li>
+Java Language Specification:
+<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-6.html#jls-6.4">6.4 Shadowing and Obscuring</a>.
+</li>
+
+
+</references>
+</qhelp>

java/ql/src/Language Abuse/TypeVariableHidesType.ql

@@ -0,0 +1,39 @@
+/**
+ * @name Type variable hides another type
+ * @description A type variable with the same name as another type that is in scope can cause
+ *              the two types to be confused.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id java/type-variable-hides-type
+ * @tags reliability
+ *       readability
+ *       types
+ */
+
+import java
+
+RefType anOuterType(TypeVariable var) {
+  var.getGenericCallable().getDeclaringType() = result
+  or
+  var.getGenericType() = result
+  or
+  result = anOuterType(var).(NestedType).getEnclosingType()
+}
+
+RefType aTypeVisibleFrom(TypeVariable var) {
+  result = anOuterType(var)
+  or
+  exists(ImportType i |
+    var.getLocation().getFile() = i.getCompilationUnit() and
+    result = i.getImportedType()
+  )
+  or
+  (var.getPackage() = result.getPackage() and result instanceof TopLevelType)
+}
+
+from RefType hidden, TypeVariable var
+where
+  hidden = aTypeVisibleFrom(var) and
+  var.getName() = hidden.getName()
+select var, "Type $@ is hidden by this type variable.", hidden, hidden.getQualifiedName()

java/ql/src/Language Abuse/UselessNullCheck.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Sometimes you can guarantee that a particular variable will never be null.
+For example when that variable has just been assigned a newly created
+object or is the exception caught by a <code>catch</code> clause.
+A null check on such a variable is misleading, and can potentially indicate a
+logic error.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Do not check a variable for null if a null value is clearly impossible.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+The following example shows a null check on a newly created object. An object
+returned by <code>new</code> can never be null, so this check is superfluous.
+</p>
+<sample src="UselessNullCheckBad.java" />
+
+</example>
+
+<references>
+
+<li>
+Java Language Specification:
+<a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.5">Creation of New Class Instances</a>,
+<a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-14.html#jls-14.20.1">Execution of try-catch</a>.
+</li>
+
+</references>
+
+</qhelp>

java/ql/src/Language Abuse/UselessNullCheck.ql

@@ -0,0 +1,28 @@
+/**
+ * @name Useless null check
+ * @description Checking whether an expression is null when that expression cannot
+ *              possibly be null is useless.
+ * @kind problem
+ * @problem.severity warning
+ * @precision very-high
+ * @id java/useless-null-check
+ * @tags maintainability
+ *       useless-code
+ *       external/cwe/cwe-561
+ */
+
+import java
+import semmle.code.java.dataflow.NullGuards
+import semmle.code.java.controlflow.Guards
+
+from Expr guard, Expr e, Expr reason, string msg
+where
+  guard = basicNullGuard(e, _, true) and
+  e = clearlyNotNullExpr(reason) and
+  (if reason instanceof Guard then
+    msg = "This check is useless, $@ cannot be null here, since it is guarded by $@."
+  else if reason != e then
+    msg = "This check is useless, $@ cannot be null here, since $@ always is non-null."
+  else
+    msg = "This check is useless, since $@ always is non-null.")
+select guard, msg, e, e.toString(), reason, reason.toString()

java/ql/src/Language Abuse/UselessNullCheckBad.java

@@ -0,0 +1,4 @@
+Object o = new Object();
+if (o == null) {
+  // this cannot happen!
+}

java/ql/src/Language Abuse/UselessTypeTest.java

@@ -0,0 +1,11 @@
+public class UselessTypeTest {
+	private static class B {}
+	private static class D extends B {}
+
+	public static void main(String[] args) {
+		D d = new D();
+		if(d instanceof B) {	// violation
+			System.out.println("Mon dieu, d is a B!");
+		}
+	}
+}

java/ql/src/Language Abuse/UselessTypeTest.qhelp

@@ -0,0 +1,24 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+It is always the case that, for any type <code>B</code>, an instance of a type derived from <code>B</code> is also an instance of <code>B</code>.
+There is no need to explicitly test that this relationship exists.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Remove the unnecessary type test to simplify the code.
+</p>
+
+</recommendation>
+<example>
+<p>The following example shows an unnecessary type test.</p>
+
+<sample src="UselessTypeTest.java" />
+</example>
+</qhelp>

java/ql/src/Language Abuse/UselessTypeTest.ql

@@ -0,0 +1,22 @@
+/**
+ * @name Useless type test
+ * @description Testing whether a derived type is an instance of its base type is unnecessary.
+ * @kind problem
+ * @problem.severity warning
+ * @precision very-high
+ * @id java/useless-type-test
+ * @tags maintainability
+ *       language-features
+ *       external/cwe/cwe-561
+ */
+
+import java
+
+from InstanceOfExpr ioe, RefType t, RefType ct
+where
+  t = ioe.getExpr().getType() and
+  ct = ioe.getTypeName().getType() and
+  ct = t.getASupertype+()
+select ioe, "There is no need to test whether an instance of $@ is also an instance of $@ - it always is.",
+  t, t.getName(),
+  ct, ct.getName()

java/ql/src/Language Abuse/UselessUpcast.java

@@ -0,0 +1,25 @@
+public class UselessUpcast {
+	private static class B {}
+	private static class D extends B {}
+
+	private static void Foo(B b) { System.out.println("Foo(B)"); }
+	private static void Foo(D d) { System.out.println("Foo(D)"); }
+
+	private static class Expr {}
+	private static class AddExpr extends Expr {}
+	private static class SubExpr extends Expr {}
+
+	public static void main(String[] args) {
+		D d = new D();
+		B b_ = (B)d;	// violation: redundant cast, consider removing
+
+		B b = new D();
+		D d_ = (D)b;	// non-violation: required downcast
+
+		Foo(d);
+		Foo((B)d);		// non-violation: required to call Foo(B)
+
+		// Non-violation: required to specify the type of the ternary operands.
+		Expr e = d != null ? (Expr)new AddExpr() : new SubExpr();
+	}
+}

java/ql/src/Language Abuse/UselessUpcast.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+In most situations, casting an instance of a derived type to a base type serves no purpose, since the conversion can be done implicitly. In such cases,
+the redundant cast can simply be removed. However, an upcast is not redundant in the following situations:
+</p>
+
+<ul>
+<li>It is being used to force a call to an overloaded callable that takes a parameter of the base type rather than one of the derived type.</li>
+<li>It is being used to specify the type to use for the operands of a ternary expression.</li>
+</ul>
+
+<p>
+Both of these special cases are illustrated in the example below. This rule ignores these 
+special cases and highlights upcasts which appear to be redundant.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Remove the unnecessary upcast to simplify the code.
+</p>
+
+</recommendation>
+<example>
+<p>The following code includes an example of a redundant upcast that would be highlighted by this rule. 
+In addition, three examples of upcasts that are required and are ignored by this rule.</p>  
+
+<sample src="UselessUpcast.java" />
+</example>
+</qhelp>

java/ql/src/Language Abuse/UselessUpcast.ql

@@ -0,0 +1,68 @@
+/**
+ * @name Useless upcast
+ * @description Upcasting a derived type to its base type is usually unnecessary.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id java/useless-upcast
+ * @tags maintainability
+ *       language-features
+ *       external/cwe/cwe-561
+ */
+
+import java
+
+predicate usefulUpcast(CastExpr e) {
+  // Upcasts that may be performed to affect resolution of methods or constructors.
+  exists(Call c, int i, Callable target |
+    c.getArgument(i).getProperExpr() = e and
+    target = c.getCallee() and
+    // An upcast to the type of the corresponding parameter.
+    e.getType() = target.getParameterType(i)
+    |
+    // There is an overloaded method/constructor in the class that we might be trying to avoid.
+    exists(Callable other |
+      other.getName() = target.getName() and
+      other.getSourceDeclaration() != target.getSourceDeclaration()
+      |
+      c.(MethodAccess).getReceiverType().(RefType).inherits((Method)other) or
+      other = target.(Constructor).getDeclaringType().getAConstructor()
+    )
+  )
+  or
+  // Upcasts of a varargs argument.
+  exists(Call c, int iArg, int iParam | c.getArgument(iArg).getProperExpr() = e |
+    c.getCallee().getParameter(iParam).isVarargs() and iArg >= iParam
+  )
+  or
+  // Upcasts that are performed on an operand of a ternary expression.
+  exists(ConditionalExpr ce | e = ce.getTrueExpr().getProperExpr() or e = ce.getFalseExpr().getProperExpr())
+  or
+  // Upcasts to raw types.
+  e.getType() instanceof RawType
+  or
+  e.getType().(Array).getElementType() instanceof RawType
+  or
+  // Upcasts that are performed to affect field, private method, or static method resolution.
+  exists(FieldAccess fa | e = fa.getQualifier().getProperExpr() |
+    not e.getExpr().getType().(RefType).inherits(fa.getField())
+  ) or
+  exists(MethodAccess ma, Method m |
+    e = ma.getQualifier().getProperExpr() and m = ma.getMethod() and (m.isStatic() or m.isPrivate())
+    |
+    not e.getExpr().getType().(RefType).inherits(m)
+  )
+}
+
+from Expr e, RefType src, RefType dest
+where
+  exists(CastExpr cse | cse = e |
+    exists(cse.getLocation()) and
+    src = cse.getExpr().getType() and
+    dest = cse.getType()
+  ) and
+  dest = src.getASupertype+() and
+  not usefulUpcast(e)
+select e, "There is no need to upcast from $@ to $@ - the conversion can be done implicitly.",
+  src, src.getName(),
+  dest, dest.getName()

java/ql/src/Language Abuse/WrappedIterator.qhelp

@@ -0,0 +1,60 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<include src="IterableOverview.qhelp" />
+
+<recommendation>
+<p>
+When writing the <code>iterator()</code> method in an
+<code>Iterable&lt;T&gt;</code> then it is important to make sure that each call
+will result in a fresh <code>Iterator&lt;T&gt;</code> instance containing all
+the necessary state for keeping track of the iteration.  If the iterator is
+stored in the <code>Iterable&lt;T&gt;</code>, or somehow refers to iteration
+state stored in the <code>Iterable&lt;T&gt;</code>, then subsequent calls to
+<code>iterator()</code> can result in loops that only traverse a subset of the
+elements or have no effect at all.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+The following example returns the same iterator on every call, and therefore
+causes the second loop to terminate immediately without any effect.
+</p>
+<sample src="WrappedIteratorBad1.java" />
+
+<p>
+This second example returns a newly created iterator each time,
+but still relies on iteration state stored
+in the surrounding class, and therefore also causes the second loop to
+terminate immediately.
+</p>
+<sample src="WrappedIteratorBad2.java" />
+
+<p>
+The code should instead be written like this, such that each call to
+<code>iterator()</code> correctly gives a fresh iterator that starts at the
+beginning.
+</p>
+<sample src="WrappedIteratorGood.java" />
+</example>
+
+<references>
+
+<li>
+The Java Language Specification:
+<a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-14.html#jls-14.14.2">The enhanced for statement</a>.
+</li>
+<li>
+The Java API Specification:
+<a href="https://docs.oracle.com/javase/8/docs/api/java/lang/Iterable.html">Interface Iterable&lt;T&gt;</a>,
+<a href="https://docs.oracle.com/javase/8/docs/api/java/util/Iterator.html">Interface Iterator&lt;T&gt;</a>,
+<a href="https://docs.oracle.com/javase/8/docs/api/java/nio/file/DirectoryStream.html">Interface DirectoryStream&lt;T&gt;</a>.
+</li>
+
+</references>
+
+</qhelp>

java/ql/src/Language Abuse/WrappedIterator.ql

@@ -0,0 +1,49 @@
+/**
+ * @name Iterable wrapping an iterator
+ * @description An 'Iterable' that reuses an 'Iterator' instance does not support multiple traversals
+ *              and can lead to unexpected behavior.
+ * @kind problem
+ * @problem.severity warning
+ * @precision very-high
+ * @id java/iterable-wraps-iterator
+ * @tags correctness
+ *       reliability
+ */
+import java
+import IterableClass
+
+/** An `Iterable` that is merely a thin wrapper around a contained `Iterator`. */
+predicate iteratorWrapper(Iterable it, Field f, boolean wrap) {
+  // A class that implements `java.lang.Iterable` and
+  // declares a final or effectively final field ...
+  f.getDeclaringType().getSourceDeclaration() = it and
+  (
+    f.isFinal() or
+    (
+      strictcount(f.getAnAssignedValue()) = 1 and
+      f.getAnAssignedValue().getEnclosingCallable() instanceof InitializerMethod
+    )
+  ) and
+  // ... whose type is a sub-type of `java.util.Iterator` and ...
+  f.getType().(RefType).getASupertype*().getSourceDeclaration().hasQualifiedName("java.util", "Iterator") and
+  // ... whose value is returned by the `iterator()` method of this class ...
+  exists(Expr iterator | iterator = it.simpleIterator() |
+    // ... either directly ...
+    iterator = f.getAnAccess() and wrap = false or
+    // ... or wrapped in another Iterator.
+    exists(ClassInstanceExpr cie | cie = iterator and wrap = true |
+      cie.getAnArgument() = f.getAnAccess() or
+      cie.getAnonymousClass().getAMethod() = f.getAnAccess().getEnclosingCallable()
+    )
+  )
+}
+
+from Iterable i, Field f, boolean wrap, string appearto, string iteratorbasedon
+where
+  iteratorWrapper(i, f, wrap) and
+  ( wrap = true and appearto = "appear to " and iteratorbasedon = "an iterator based on " or
+    wrap = false and appearto = "" and iteratorbasedon = ""
+  )
+select i, "This class implements Iterable, but does not " + appearto + "support multiple iterations," +
+  " since its iterator method always returns " + iteratorbasedon + "the same $@.",
+  f, "iterator"

java/ql/src/Language Abuse/WrappedIteratorBad1.java

@@ -0,0 +1,19 @@
+class MySequence implements Iterable<MyElem> {
+  // ... some reference to data
+  final Iterator<MyElem> it = data.iterator();
+  // Wrong: reused iterator
+  public Iterator<MyElem> iterator() {
+    return it;
+  }
+}
+
+void useMySequence(MySequence s) {
+  // do some work by traversing the sequence
+  for (MyElem e : s) {
+    // ...
+  }
+  // do some more work by traversing it again
+  for (MyElem e : s) {
+    // ...
+  }
+}

java/ql/src/Language Abuse/WrappedIteratorBad2.java

@@ -0,0 +1,18 @@
+class MySequence implements Iterable<MyElem> {
+  // ... some reference to data
+  final Iterator<MyElem> it = data.iterator();
+  // Wrong: iteration state outside returned iterator
+  public Iterator<MyElem> iterator() {
+    return new Iterator<MyElem>() {
+      public boolean hasNext() {
+        return it.hasNext();
+      }
+      public MyElem next() {
+        return transformElem(it.next());
+      }
+      public void remove() {
+        // ...
+      }
+    };
+  }
+}

java/ql/src/Language Abuse/WrappedIteratorGood.java

@@ -0,0 +1,18 @@
+class MySequence implements Iterable<MyElem> {
+  // ... some reference to data
+  public Iterator<MyElem> iterator() {
+    return new Iterator<MyElem>() {
+      // Correct: iteration state inside returned iterator
+      final Iterator<MyElem> it = data.iterator();
+      public boolean hasNext() {
+        return it.hasNext();
+      }
+      public MyElem next() {
+        return transformElem(it.next());
+      }
+      public void remove() {
+        // ...
+      }
+    };
+  }
+}