QL code and tests for C#/C++/JavaScript.

csharp/ql/src/.project

@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<projectDescription>
+	<name>semmlecode-csharp-queries</name>
+	<comment></comment>
+	<projects>
+	</projects>
+	<buildSpec>
+		<buildCommand>
+			<name>org.eclipse.pde.ManifestBuilder</name>
+			<arguments>
+			</arguments>
+		</buildCommand>
+		<buildCommand>
+			<name>org.eclipse.pde.SchemaBuilder</name>
+			<arguments>
+			</arguments>
+		</buildCommand>
+	</buildSpec>
+	<natures>
+		<nature>org.eclipse.pde.PluginNature</nature>
+		<nature>com.semmle.plugin.qdt.core.qlnature</nature>
+	</natures>
+</projectDescription>

csharp/ql/src/.qlpath

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<ns:qlpath xmlns:ns="https://semmle.com/schemas/qlpath">
+    <librarypath></librarypath>
+    <dbscheme kind="WORKSPACE">semmlecode-csharp-queries/semmlecode.csharp.dbscheme</dbscheme>
+    <defaultImports><defaultImport>csharp</defaultImport></defaultImports>
+</ns:qlpath>

csharp/ql/src/.vs/VSWorkspaceSettings.json

@@ -0,0 +1,8 @@
+{
+    "ql.projects" : {
+        "." : {
+            "dbScheme" : "semmlecode.csharp.dbscheme",
+            "libraryPath" : []
+        }
+    }
+}

csharp/ql/src/API Abuse/CallToGCCollect.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Explicitly forcing garbage collection is not efficient and is almost never necessary outside of 
+benchmarking scenarios.</p>
+
+</overview>
+<recommendation>
+<p>Remove the explicit call to <code>GC.Collect()</code> and run a memory profiler to optimize your 
+application's memory usage. If your application uses unmanaged resources and calls <code>
+GC.Collect()</code> to force finalizers to run, it is better to implement the <code>IDisposable
+</code> pattern and use <code>try</code>/<code>finally</code> clauses to make sure that unmanaged
+resources are disposed of even if an exception interrupts your application.</p>
+
+</recommendation>
+<example>
+<sample src="CallToGCCollectBad.cs" />
+
+</example>
+<references>
+
+  <li>MSDN: <a href="http://msdn.microsoft.com/en-us/library/system.idisposable.aspx">The IDisposable interface</a>.</li>
+  <li>Microsoft: <a href="https://docs.microsoft.com/en-us/visualstudio/profiling/memory-usage">Profile Memory Usage in Visual Studio</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/CallToGCCollect.ql

@@ -0,0 +1,19 @@
+/**
+ * @name Call to GC.Collect()
+ * @description Explicit requests for garbage collection often indicate performance problems and memory leaks.
+ * @kind problem
+ * @problem.severity warning
+ * @precision very-high
+ * @id cs/call-to-gc
+ * @tags efficiency
+ *       maintainability
+ */
+import csharp
+
+from MethodCall c, Method gcCollect
+where
+  c.getTarget() = gcCollect
+  and gcCollect.hasName("Collect")
+  and gcCollect.hasNoParameters()
+  and gcCollect.getDeclaringType().hasQualifiedName("System.GC")
+select c, "Call to 'GC.Collect()'."

csharp/ql/src/API Abuse/CallToGCCollectBad.cs

@@ -0,0 +1,9 @@
+using System;
+
+class Bad
+{
+    void M()
+    {
+        GC.Collect();
+    }
+}

csharp/ql/src/API Abuse/CallToObsoleteMethod.qhelp

@@ -0,0 +1,37 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Methods with the <code>[Obsolete]</code> attribute are obsolete and should not be used. 
+Obsolete methods are no longer supported and maintained, may not work correctly, and may be removed in the future.</p>
+
+</overview>
+<recommendation>
+<p>Replace the method call with a call to a different method. The <code>[Obsolete]</code>
+attribute should suggest a replacement method to call. If the <code>[Obsolete]</code> attribute 
+does not suggest a replacement, then read the class documentation and list of methods to find
+a suitable replacement.</p>
+
+</recommendation>
+<example>
+<p>The following example shows some code which calls an obsolete method in a <code>Logger</code> class.
+The <code>Log</code> method has the attribute 
+<code>[Obsolete("Use Log(LogLevel level, string s) instead")]</code>
+showing that it is obsolete.</p>
+
+<sample src="CallToObsoleteMethodBad.cs" />
+
+<p>The code is fixed by calling a different method in the <code>Logger</code> class as suggested
+by the attribute.</p>
+
+<sample src="CallToObsoleteMethodGood.cs" />
+
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.obsoleteattribute(v=vs.110).aspx">ObsoleteAttribute Class</a>.</li>
+
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/CallToObsoleteMethod.ql

@@ -0,0 +1,26 @@
+/**
+ * @name Call to obsolete method
+ * @description Calls to methods marked as [Obsolete] should be replaced because the method is
+ *              no longer maintained and may be removed in the future.
+ * @kind problem
+ * @problem.severity warning
+ * @precision very-high
+ * @id cs/call-to-obsolete-method
+ * @tags changeability
+ *       maintainability
+ *       external/cwe/cwe-477
+ */
+
+import csharp
+
+class ObsoleteAttribute extends Attribute {
+  ObsoleteAttribute() {
+    this.getType().hasQualifiedName("System", "ObsoleteAttribute")
+  }
+}
+
+from MethodCall c, Method m
+where m = c.getTarget()
+and m.getAnAttribute() instanceof ObsoleteAttribute
+and not c.getEnclosingCallable().(Attributable).getAnAttribute() instanceof ObsoleteAttribute
+select c, "Call to obsolete method $@.", m, m.getName()

csharp/ql/src/API Abuse/CallToObsoleteMethodBad.cs

@@ -0,0 +1,30 @@
+using System;
+
+class Bad
+{
+    void M()
+    {
+        Logger.Log("Hello, World!");
+    }
+
+    static class Logger
+    {
+        [Obsolete("Use Log(LogLevel level, string s) instead")]
+        public static void Log(string s)
+        {
+            // ...
+        }
+
+        public static void Log(LogLevel level, string s)
+        {
+            // ...
+        }
+    }
+
+    enum LogLevel
+    {
+        Info,
+        Warning,
+        Error
+    }
+}

csharp/ql/src/API Abuse/CallToObsoleteMethodGood.cs

@@ -0,0 +1,30 @@
+using System;
+
+class Good
+{
+    void M()
+    {
+        Logger.Log(LogLevel.Info, "Hello, World!");
+    }
+
+    static class Logger
+    {
+        [Obsolete("Use Log(LogLevel level, string s) instead")]
+        public static void Log(string s)
+        {
+            // ...
+        }
+
+        public static void Log(LogLevel level, string s)
+        {
+            // ...
+        }
+    }
+
+    enum LogLevel
+    {
+        Info,
+        Warning,
+        Error
+    }
+}

csharp/ql/src/API Abuse/ClassDoesNotImplementEquals.qhelp

@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>When the class of the object on which <code>Equals(object)</code> is called does not define its own 
+<code>Equals(object)</code> method, an <code>Equals(object)</code> method defined in one of its base
+classes will be called instead. In the worst case, the <code>Equals(object)</code> method of
+<code>System.Object</code> will be called, resulting in a reference equality check. This is probably
+not what was intended.</p>
+
+<p>Classes that implement the <code>==</code> operator should also override the <code>Equals(object)</code>
+method, because otherwise the two forms of equality will behave differently, leading to unexpected behavior.</p>
+</overview>
+<recommendation>
+<p>Implement an <code>Equals(object)</code> method for the highlighted class. Examine subclasses of the
+class highlighted to determine if they should implement their own equals method too.</p>
+
+</recommendation>
+<example>
+<p>The output of this example states that "car1 does equal car2" despite the fact that one is a 
+leaded version and one is an unleaded version. This is because the <code>GasolineCar</code> class 
+is inheriting <code>Equals(object)</code> from <code>Car</code> and that method states that two
+<code>Car</code>s are equal if their make and model are the same.</p>
+<sample src="ClassDoesNotImplementEqualsBad.cs" />
+
+<p>
+In the revised example, <code>GasolineCar</code> overrides <code>Equals(object)</code>, and the output
+is "car1 does not equal car2", as expected.
+</p>
+<sample src="ClassDoesNotImplementEqualsGood.cs" />
+</example>
+
+<references>
+  <li>Microsoft: <a href="https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/equality-operators">Equality Operators</a>, 
+  <a href="https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/equality-comparisons">Equality Comparisons (C# Programming Guide)</a>.</li>
+</references>
+
+</qhelp>

csharp/ql/src/API Abuse/ClassDoesNotImplementEquals.ql

@@ -0,0 +1,43 @@
+/**
+ * @name Class does not implement Equals(object)
+ * @description The class does not implement the 'Equals(object)' method, which can cause
+ *              unexpected behavior. The default 'Equals(object)' method performs reference
+ *              comparison, which may not be what was intended.
+ * @kind problem
+ * @problem.severity error
+ * @precision medium
+ * @id cs/class-missing-equals
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+import semmle.code.csharp.frameworks.System
+
+from Class c, Element item, string message, string itemText
+where
+  c.isSourceDeclaration()
+  and not implementsEquals(c)
+  and not c.isAbstract()
+  and
+  (
+    exists(MethodCall callToEquals |
+      callToEquals.getTarget() instanceof EqualsMethod
+      and callToEquals.getQualifier().getType() = c
+      and message = "but it is called $@"
+      and item = callToEquals
+      and itemText = "here" )
+    or
+    ( item = c.getAnOperator().(EQOperator)
+      and message = "but it implements $@"
+      and itemText = "operator ==" )
+    or
+    exists(IEquatableEqualsMethod eq | item = eq
+      and eq = c.getAMethod()
+      and message = "but it implements $@"
+      and itemText = "IEquatable<" + eq.getParameter(0).getType() + ">.Equals"
+    )
+  )
+
+select c, "Class '" + c.getName() + "' does not implement Equals(object), " + message + ".",
+  item, itemText

csharp/ql/src/API Abuse/ClassDoesNotImplementEqualsBad.cs

@@ -0,0 +1,40 @@
+using System;
+
+class Bad
+{
+    class Car
+    {
+        protected string make;
+        protected string model;
+
+        public Car(string make, string model)
+        {
+            this.make = make;
+            this.model = model;
+        }
+
+        public override bool Equals(object obj)
+        {
+            if (obj is Car c && c.GetType() == typeof(Car))
+                return make == c.make && model == c.model;
+            return false;
+        }
+    }
+
+    class GasolineCar : Car
+    {
+        protected bool unleaded;
+
+        public GasolineCar(string make, string model, bool unleaded) : base(make, model)
+        {
+            this.unleaded = unleaded;
+        }
+    }
+
+    public static void Main(string[] args)
+    {
+        var car1 = new GasolineCar("Ford", "Focus", true);
+        var car2 = new GasolineCar("Ford", "Focus", false);
+        Console.WriteLine("car1 " + (car1.Equals(car2) ? "does" : "does not") + " equal car2.");
+    }
+}

csharp/ql/src/API Abuse/ClassDoesNotImplementEqualsGood.cs

@@ -0,0 +1,47 @@
+using System;
+
+class Good
+{
+    class Car
+    {
+        protected string make;
+        protected string model;
+
+        public Car(string make, string model)
+        {
+            this.make = make;
+            this.model = model;
+        }
+
+        public override bool Equals(object obj)
+        {
+            if (obj is Car c && c.GetType() == typeof(Car))
+                return make == c.make && model == c.model;
+            return false;
+        }
+    }
+
+    class GasolineCar : Car
+    {
+        protected bool unleaded;
+
+        public GasolineCar(string make, string model, bool unleaded) : base(make, model)
+        {
+            this.unleaded = unleaded;
+        }
+
+        public override bool Equals(object obj)
+        {
+            if (obj is GasolineCar gc && gc.GetType() == typeof(GasolineCar))
+                return make == gc.make && model == gc.model && unleaded == gc.unleaded;
+            return false;
+        }
+    }
+
+    public static void Main(string[] args)
+    {
+        var car1 = new GasolineCar("Ford", "Focus", true);
+        var car2 = new GasolineCar("Ford", "Focus", false);
+        Console.WriteLine("car1 " + (car1.Equals(car2) ? "does" : "does not") + " equal car2.");
+    }
+}

csharp/ql/src/API Abuse/ClassImplementsICloneable.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The purpose of implementing the <code>ICloneable</code> interface is to advertise that 
+instances of your class can be cloned, but there are a number of good reasons why doing so is 
+generally a bad idea.</p>
+
+<p>First, the semantics of <code>ICloneable.Clone()</code> are not well-specified. The 
+documentation states that it "creates a new object that is a copy of the current 
+instance", but it then fails to specify what is meant by "copy". This poses a problem, because 
+there are two general ways in which an object can be copied, either deeply or shallowly (a deep 
+copy clones an object and all the objects it references recursively, whereas a shallow copy only 
+clones the top of the object graph - see example below). Since <code>ICloneable</code> does not 
+specify which type of copying should be performed, different implementing classes tend to pick one 
+or the other arbitrarily, making it impossible to call <code>Clone()</code> with any certainty in 
+general about what will happen - the method is thus useless.</p>
+
+<p>A further problem with <code>ICloneable</code> is that if a class implements it, all of its 
+subtypes must implement it as well, as must all of the types of its members if a deep copy is being 
+implemented. Not only does this make using <code>ICloneable</code> rather viral, but also rather 
+error-prone, because it is very easy to forget to override <code>Clone()</code> in subtypes. As 
+such, using <code>ICloneable</code> is widely discouraged.</p>
+
+</overview>
+<recommendation>
+<p>Define your own <code>Clone()</code> or <code>Copy()</code> methods and document whether they perform deep or shallow copying.</p>
+
+</recommendation>
+<example>
+<sample src="ClassImplementsICloneableBad.cs" />
+
+</example>
+<references>
+
+  <li>MSDN, <a href="https://msdn.microsoft.com/en-us/library/system.icloneable.aspx">ICloneable Interface</a>.</li>
+  <li>B Wagner, <em>Effective C#: 50 Specific Ways to Improve Your C#</em> (Second Edition), Item 32: Avoid ICloneable.</li>
+
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/ClassImplementsICloneable.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Class implements ICloneable
+ * @description Implementing 'ICloneable' is discouraged due to its imprecise semantics
+ *              and its viral effect on your code base.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision very-high
+ * @id cs/class-implements-icloneable
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+
+from ValueOrRefType c
+where
+  c.fromSource()
+  and c.getABaseInterface+().hasQualifiedName("System", "ICloneable")
+  and not c.isSealed()
+  and exists(Method m | m.getDeclaringType() = c and m.hasName("Clone"))
+select c, "Class '" + c.getName() + "' implements 'ICloneable'."

csharp/ql/src/API Abuse/ClassImplementsICloneableBad.cs

@@ -0,0 +1,43 @@
+using System;
+
+class Bad
+{
+    class Thing
+    {
+        public int I { get; set; }
+        public Thing(int i) { I = i; }
+    }
+
+    class Shallow : ICloneable
+    {
+        public Thing T { get; set; }
+        public Shallow(Thing t) { T = t; }
+
+        // Implements a shallow clone (compliant with the spec)
+        public object Clone() { return new Shallow(T); }
+    }
+
+    class Deep : ICloneable
+    {
+        public Thing T { get; set; }
+        public Deep(Thing t) { T = t; }
+
+        // Implements a deep clone (also compliant with the spec)
+        public object Clone() { return new Deep(new Thing(T.I)); }
+    }
+
+    static void Main(string[] args)
+    {
+        var s1 = new Shallow(new Thing(23));
+        var s2 = (Shallow)s1.Clone();
+        Console.WriteLine(s2.T.I); // 23
+        s1.T.I = 9;
+        Console.WriteLine(s2.T.I); // 9
+
+        var d1 = new Deep(new Thing(23));
+        var d2 = (Deep)d1.Clone();
+        Console.WriteLine(d2.T.I); // 23
+        d1.T.I = 9;
+        Console.WriteLine(d2.T.I); // 23
+    }
+}

csharp/ql/src/API Abuse/Dispose.qll

@@ -0,0 +1,93 @@
+import csharp
+private import DataFlow
+private import semmle.code.csharp.frameworks.System
+private import semmle.code.csharp.frameworks.system.web.UI
+
+class DisposableType extends RefType {
+  DisposableType() {
+    this.getABaseType+() = getSystemIDisposableInterface()
+  }
+}
+
+class DisposableField extends Field {
+  DisposableField() {
+    this.getType() instanceof DisposableType
+  }
+}
+
+class WebControl extends RefType {
+  WebControl() {
+    this.getBaseClass*() = getSystemWebUIControlClass()
+  }
+}
+
+class WebPage extends RefType {
+  WebPage() {
+    this.getBaseClass*() = getSystemWebUIPageClass()
+  }
+}
+
+/**
+ * Holds if `f` is an auto-disposed web control.
+ *
+ * Web controls that are either child controls or controls on a page
+ * are auto disposed: `System.Web.UI.Control` defines `UnloadRecursive()`
+ * which invokes `Dispose()` recursively on all nested controls;
+ * `System.Web.UI.Page` defines `ProcessRequestCleanup()` which invokes
+ * `UnloadRecursive()`.
+ */
+predicate isAutoDisposedWebControl(Field f) {
+  f.getType() instanceof WebControl
+  and
+  f.getDeclaringType() = any(RefType t |
+    t instanceof WebControl or
+    t instanceof WebPage
+  )
+}
+
+
+/**
+ * An object creation that creates an `IDisposable` instance into the local scope.
+ */
+class LocalScopeDisposableCreation extends Call {
+  LocalScopeDisposableCreation() {
+    exists(Type t |
+      t = this.getType() and
+      // Type extends IDisposable
+      t instanceof DisposableType and
+      // Within function, not field or instance initializer
+      exists(this.getEnclosingCallable()) |
+      // Either an ordinary object creation
+      this instanceof ObjectCreation
+      or
+      // Or a creation using a factory method
+      exists(Method create |
+        this.getTarget() = create |
+        create.hasName("Create") and
+        create.isStatic() and
+        create.getDeclaringType().getSourceDeclaration() = t.getSourceDeclaration()
+      )
+    )
+  }
+
+  /**
+   * Gets an expression that, if it is disposed of, will imply that the object
+   * created by this creation is disposed of as well.
+   */
+  Expr getADisposeTarget() {
+    result = getADisposeTarget0().asExpr()
+  }
+
+  private DataFlow::Node getADisposeTarget0() {
+    result = exprNode(this)
+    or
+    exists(DataFlow::Node mid |
+      mid = this.getADisposeTarget0() |
+      localFlowStep(mid, result)
+      or
+      result.asExpr() = any(LocalScopeDisposableCreation other |
+        other.getAnArgument() = mid.asExpr()
+      )
+    )
+  }
+}

csharp/ql/src/API Abuse/DisposeNotCalledOnException.qhelp

@@ -0,0 +1,52 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+
+<p>
+If an exception is thrown between the allocation of an <code>IDisposable</code> object and a
+<code>Dispose()</code> call on that object, and the <code>Dispose()</code> call is not within a
+<code>catch</code> or <code>finally</code> block, then the <code>Dispose()</code> call may not
+execute.
+</p>
+
+</overview>
+<recommendation>
+<p>
+If possible, wrap the allocation of the object in a <code>using</code> block to automatically
+dispose of the object once the <code>using</code> block has completed.
+</p>
+<p>
+If this is not possible, ensure that <code>Dispose()</code> is called on the object. It is usually
+recommended to call <code>Dispose()</code> within a <code>finally</code> block, to ensure that the
+object is disposed of even if an exception is thrown.
+</p>
+
+</recommendation>
+<example>
+<p>
+In this example, an <code>SqlConnection</code> is created, then an SQL query is run using an
+<code>SqlCommand</code>. The objects are created and disposed of, but if an
+exception is thrown - for example, by the call to <code>ExecuteReader</code> - the method will
+terminate immediately and <code>Dispose()</code> will never be called on <code>cmd</code> and
+<code>conn</code>. In the case of the <code>SqlConnection</code>, this can result in unmanaged
+resources associated with the connection being retained, and possibly cause resource exhaustion
+when trying to create additional future connections.
+</p>
+<sample src="DisposeNotCalledOnExceptionBad.cs" />
+
+<p>
+In the revised example, a pair of <code>using</code> statements are used to ensure that both the
+connection and the command are disposed of after the statements have completed.
+</p>
+<sample src="DisposeNotCalledOnExceptionGood.cs" />
+
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.idisposable.aspx">IDisposable Interface</a>.</li>
+  <li>Microsoft: <a href="https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/using-statement">using Statement (C# Reference)</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/DisposeNotCalledOnException.ql

@@ -0,0 +1,74 @@
+/**
+ * @name Dispose may not be called if an exception is thrown during execution
+ * @description Methods that create objects of type 'IDisposable' should call 'Dispose()' on those
+ *              objects, even during exceptional circumstances, otherwise unmanaged resources may
+ *              not be released.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id cs/dispose-not-called-on-throw
+ * @tags efficiency
+ *       maintainability
+ *       security
+ *       external/cwe/cwe-404
+ *       external/cwe/cwe-459
+ *       external/cwe/cwe-460
+ */
+
+import csharp
+import Dispose
+import semmle.code.csharp.frameworks.System
+
+/**
+ * Gets an exception type that may be thrown during the execution of method `m`.
+ * Assumes any exception may be thrown by library types.
+ */
+ExceptionClass getAThrownException(Method m) {
+  m.fromLibrary() and
+  result = any(SystemExceptionClass sc)
+  or
+  exists(ControlFlowElement cfe |
+    cfe = any(ThrowElement te | result = te.getExpr().getType()) or
+    cfe = any(MethodCall mc | result = getAThrownException(mc.getARuntimeTarget())) |
+    cfe.getEnclosingCallable() = m and
+    not isTriedAgainstException(cfe, result)
+  )
+}
+
+/**
+ * Holds if control flow element is tried against throwing an exception of type
+ * `ec`.
+ */
+pragma [noinline]
+predicate isTriedAgainstException(ControlFlowElement cfe, ExceptionClass ec) {
+  (cfe instanceof ThrowElement or cfe instanceof MethodCall) and
+  exists(TryStmt ts |
+    ts.getATriedElement() = cfe and
+    exists(ts.getAnExceptionHandler(ec))
+  )
+}
+
+/**
+ * Holds if `disposeCall` disposes the object created by `disposableCreation`.
+ */
+predicate disposeReachableFromDisposableCreation(MethodCall disposeCall, Expr disposableCreation) {
+  // The qualifier of the Dispose call flows from something that introduced a disposable into scope
+  (disposableCreation instanceof LocalScopeDisposableCreation or disposableCreation instanceof MethodCall)
+  and DataFlow::localFlowStep+(DataFlow::exprNode(disposableCreation), DataFlow::exprNode(disposeCall.getQualifier()))
+  and disposeCall.getTarget() instanceof DisposeMethod
+}
+
+
+from MethodCall disposeCall, Expr disposableCreation, MethodCall callThatThrows
+where
+  disposeReachableFromDisposableCreation(disposeCall, disposableCreation)
+  // The dispose call is not, itself, within a dispose method.
+  and not disposeCall.getEnclosingCallable() instanceof DisposeMethod
+  // Dispose call not within a finally or catch block
+  and not exists(TryStmt ts |
+    ts.getACatchClause().getAChild*() = disposeCall or ts.getFinally().getAChild*() = disposeCall)
+  // At least one method call exists between the allocation and disposal that could throw
+  and disposableCreation.getAReachableElement() = callThatThrows
+  and callThatThrows.getAReachableElement() = disposeCall
+  and exists(getAThrownException(callThatThrows.getARuntimeTarget()))
+select disposeCall, "Dispose missed if exception is thrown by $@.", callThatThrows, callThatThrows.toString()

csharp/ql/src/API Abuse/DisposeNotCalledOnExceptionBad.cs

@@ -0,0 +1,19 @@
+using System;
+using System.Data.SqlClient;
+
+class Bad
+{
+    public SqlDataReader GetAllCustomers()
+    {
+        var conn = new SqlConnection("connection string");
+        conn.Open();
+
+        var cmd = new SqlCommand("SELECT * FROM Customers", conn);
+        var ret = cmd.ExecuteReader();
+
+        cmd.Dispose();
+        conn.Dispose();
+
+        return ret;
+    }
+}

csharp/ql/src/API Abuse/DisposeNotCalledOnExceptionGood.cs

@@ -0,0 +1,17 @@
+using System;
+using System.Data.SqlClient;
+
+class Good
+{
+    public SqlDataReader GetAllCustomers()
+    {
+        using (var conn = new SqlConnection("connection string"))
+        {
+            conn.Open();
+            using (var cmd = new SqlCommand("SELECT * FROM Customers", conn))
+            {
+                return cmd.ExecuteReader();
+            }
+        }
+    }
+}

csharp/ql/src/API Abuse/FormatInvalid.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+The format string supplied to formatting methods (such as <code>string.Format()</code>)
+must be formatted correctly, otherwise the exception <code>System.FormatException</code> 
+will be thrown.
+</p>
+
+</overview>
+
+<recommendation>
+
+<p>
+Change the format string so that it is correctly formatted. Ensure that each
+format item adheres to the syntax:
+</p>
+
+<blockquote>
+  <p><b>{</b><i>index</i>[<b>,</b><i>alignment</i>][<b>:</b><i>formatString</i>]<b>}</b></p>
+</blockquote>
+
+<p>
+When literals <code>{</code> or <code>}</code> are required, replace them with <code>{{</code> and
+<code>}}</code>, respectively, or supply them as arguments.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In this example, a format string uses both literals <code>{</code> and <code>}</code>, but the
+literals are not properly escaped.
+</p>
+<sample src="FormatInvalidBad.cs" />
+
+<p>
+In the revised example, the literals are properly escaped.
+</p>
+<sample src="FormatInvalidGood.cs" />
+
+</example>
+<references>
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.string.format.aspx">String.Format Method</a>.</li>
+  <li>Microsoft: <a href="https://docs.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting">Composite Formatting</a>.</li>
+</references>
+</qhelp>

csharp/ql/src/API Abuse/FormatInvalid.ql

@@ -0,0 +1,17 @@
+/**
+ * @name Invalid format string
+ * @description Using a format string with an incorrect format causes a 'System.FormatException'.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cs/invalid-format-string
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+import semmle.code.csharp.frameworks.Format
+
+from FormatCall s, InvalidFormatString src
+where src = s.getAFormatSource()
+select src, "Invalid format string used in $@ formatting call.", s, "this"

csharp/ql/src/API Abuse/FormatInvalidBad.cs

@@ -0,0 +1,9 @@
+using System;
+
+class Bad
+{
+    string GenerateEmptyClass(string c)
+    {
+        return string.Format("class {0} { }");
+    }
+}

csharp/ql/src/API Abuse/FormatInvalidGood.cs

@@ -0,0 +1,9 @@
+using System;
+
+class Good
+{
+    string GenerateEmptyClass(string c)
+    {
+        return string.Format("class {0} {{ }}");
+    }
+}

csharp/ql/src/API Abuse/FormatMissingArgument.qhelp

@@ -0,0 +1,41 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Formatting methods (such as <code>String.Format()</code>) that are missing arguments will 
+throw the exception <code>System.FormatException</code>. This is caused by the format 
+string not matching the actual arguments supplied or an incorrect format string.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Supply the correct number of arguments to the format method, or change the format string 
+to use the correct arguments.
+</p>
+</recommendation>
+
+<example>
+<p>
+Here are two examples where the call to <code>String.Format()</code> is missing arguments.
+</p>
+<sample src="FormatMissingArgumentBad.cs"/>
+<ul>
+  <li>On line 5, the second argument (<code>last</code>) is not supplied.</li>
+  <li>On line 6, the format items are numbered <code>{1}</code> and <code>{2}</code>,
+    instead of <code>{0}</code> and <code>{1}</code> as they should be.</li>
+</ul>
+
+<p>
+In the revised example, both arguments are supplied.
+</p>
+<sample src="FormatMissingArgumentGood.cs"/>
+</example>
+
+<references>
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.string.format.aspx">String.Format Method</a>.</li>
+  <li>Microsoft: <a href="https://docs.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting">Composite Formatting</a>.</li>
+</references>
+</qhelp>

csharp/ql/src/API Abuse/FormatMissingArgument.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Missing format argument
+ * @description Supplying too few arguments to a format string causes a 'System.FormatException'.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cs/format-argument-missing
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+import semmle.code.csharp.frameworks.Format
+
+from FormatCall format, ValidFormatString src, int used, int supplied
+where src = format.getAFormatSource()
+and used = src.getAnInsert()
+and supplied = format.getSuppliedArguments()
+and used >= supplied
+select format, "Argument '{" + used + "}' has not been supplied to $@ format string.", src, "this"

csharp/ql/src/API Abuse/FormatMissingArgumentBad.cs

@@ -0,0 +1,10 @@
+using System;
+
+class Bad
+{
+    void Hello(string first, string last)
+    {
+        Console.WriteLine("Hello {0} {1}", first);
+        Console.WriteLine("Hello {1} {2}", first, last);
+    }
+}

csharp/ql/src/API Abuse/FormatMissingArgumentGood.cs

@@ -0,0 +1,9 @@
+using System;
+
+class Good
+{
+    void Hello(string first, string last)
+    {
+        Console.WriteLine("Hello {0} {1}", first, last);
+    }
+}

csharp/ql/src/API Abuse/FormatUnusedArgument.qhelp

@@ -0,0 +1,37 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Arguments which are passed to formatting methods (such as <code>String.Format()</code>)
+but are not used, are either unnecessary or mean that the format string is incorrect. The result
+is that the argument will be ignored, which may not be the intended behavior.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Change the format string to use the highlighted argument, or remove the unnecessary argument.
+</p>
+</recommendation>
+
+<example>
+<p>
+Here are three examples where the format string does not use all the arguments.
+</p>
+<sample src="FormatUnusedArgumentBad.cs"/>
+<ul>
+  <li>On line 5, the second argument (<code>ex.HResult</code>) is not logged.</li>
+  <li>On line 6, the first argument (<code>ex</code>) is not logged but the second 
+    argument (<code>ex.HResult</code>) is logged twice.</li>
+  <li>On line 4, a C-style format string is used, which is incorrect, and neither 
+    argument will be logged.</li>
+</ul>  
+</example>
+
+<references>
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.string.format.aspx">String.Format Method</a>.</li>
+  <li>Microsoft: <a href="https://docs.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting">Composite Formatting</a>.</li>
+</references>
+</qhelp>

csharp/ql/src/API Abuse/FormatUnusedArgument.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Unused format argument
+ * @description Supplying more arguments than are required for a format string may indicate an error in the format string.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cs/format-argument-unused
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+import semmle.code.csharp.frameworks.Format
+
+from FormatCall format, int unused, ValidFormatString src
+where src = format.getAFormatSource()
+and unused = format.getAnUnusedArgument(src)
+select format, "The $@ ignores $@.", src, "format string", format.getSuppliedExpr(unused), "this supplied value"

csharp/ql/src/API Abuse/FormatUnusedArgumentBad.cs

@@ -0,0 +1,11 @@
+using System;
+
+class Bad
+{
+    void M(Exception ex)
+    {
+        Console.WriteLine("Error processing file: {0}", ex, ex.HResult);
+        Console.WriteLine("Error processing file: {1} ({1})", ex, ex.HResult);
+        Console.WriteLine("Error processing file: %s (%d)", ex, ex.HResult);
+    }
+}

csharp/ql/src/API Abuse/InconsistentEqualsGetHashCode.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+A class that overrides only one of <code>Equals(object)</code> and <code>GetHashCode()</code>
+is likely to violate the contract of the <code>GetHashCode()</code> method. The contract
+requires that <code>GetHashCode()</code> gives the same integer result for any two equal objects.
+Not enforcing this property may cause unexpected results when storing and
+retrieving objects of such a class in a hashing data structure.
+</p>
+
+</overview>
+<recommendation>
+<p>Provide an implementation of the missing method that is consistent with the present method.</p>
+
+</recommendation>
+
+<example>
+
+<p>In the following example, the class <code>Bad</code> overrides 
+<code>Equals(object)</code> but not <code>GetHashCode()</code>.</p>
+
+<sample src="InconsistentEqualsGetHashCodeBad.cs" />
+
+<p>In the revised example, the class <code>Good</code> overrides both
+<code>Equals(object)</code> and <code>GetHashCode()</code>.</p>
+
+<sample src="InconsistentEqualsGetHashCodeGood.cs" />
+
+</example>
+
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/bsc2ak47.aspx">Object.Equals Method (Object)</a>,
+  <a href="https://msdn.microsoft.com/en-us/library/system.object.gethashcode.aspx">Object.GetHashCode Method</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/InconsistentEqualsGetHashCode.ql

@@ -0,0 +1,31 @@
+/**
+ * @name Inconsistent Equals(object) and GetHashCode()
+ * @description If a class overrides only one of 'Equals(object)' and 'GetHashCode()', it may mean that
+ *              'Equals(object)' and 'GetHashCode()' are inconsistent.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id cs/inconsistent-equals-and-gethashcode
+ * @tags reliability
+ *       maintainability
+ *       external/cwe/cwe-581
+ */
+import csharp
+import semmle.code.csharp.frameworks.System
+
+from Class c, Method present, string missing
+where c.isSourceDeclaration() and
+  (
+    (
+      present = (EqualsMethod)c.getAMethod() and
+      not c.getAMethod() instanceof GetHashCodeMethod and
+      missing = "GetHashCode()"
+    ) or
+    (
+      present = (GetHashCodeMethod)c.getAMethod() and
+      not implementsEquals(c) and
+      missing = "Equals(object)"
+    )
+  )
+select c, "Class '" + c.getName() + "' overrides $@, but not " + missing + ".",
+  present, present.getName()

csharp/ql/src/API Abuse/InconsistentEqualsGetHashCodeBad.cs

@@ -0,0 +1,15 @@
+using System;
+
+class Bad
+{
+    private int id;
+
+    public Bad(int Id) { this.id = Id; }
+
+    public override bool Equals(object other)
+    {
+        if (other is Bad b && b.GetType() == typeof(Bad))
+            return this.id == b.id;
+        return false;
+    }
+}

csharp/ql/src/API Abuse/InconsistentEqualsGetHashCodeGood.cs

@@ -0,0 +1,17 @@
+using System;
+
+class Good
+{
+    private int id;
+
+    public Good(int Id) { this.id = Id; }
+
+    public override bool Equals(object other)
+    {
+        if (other is Good b && b.GetType() == typeof(Good))
+            return this.id == b.id;
+        return false;
+    }
+
+    public override int GetHashCode() => id;
+}

csharp/ql/src/API Abuse/IncorrectCompareToSignature.qhelp

@@ -0,0 +1,32 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>If you wish to make a class comparable with type <code>T</code> then you should both make a 
+<code>CompareTo(T)</code> method and inherit from the <code>IComparable&lt;T&gt;</code> interface. 
+If a <code>CompareTo(T)</code> method has been added without the interface also being implemented, 
+it is sometimes an indication that the programmer has forgotten to inherit the interface despite 
+providing the implementation for it.</p>
+
+</overview>
+<recommendation>
+<p>The problem can be easily resolved by making the class implement <code>IComparable&lt;T&gt;
+</code> (either directly or indirectly).</p>
+
+</recommendation>
+<example>
+<p>In this example, the developer has implemented a <code>CompareTo(Bad)</code> method, but has
+forgotten to add the corresponding <code>IComparable&lt;Bad&gt;</code> interface.</p>
+<sample src="IncorrectCompareToSignatureBad.cs" />
+
+<p>In the revised example, the interface is added.</p>
+<sample src="IncorrectCompareToSignatureGood.cs" />
+
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.icomparable.aspx">IComparable Interface</a></li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/IncorrectCompareToSignature.ql

@@ -0,0 +1,47 @@
+/**
+ * @name Potentially incorrect CompareTo(...) signature
+ * @description The declaring type of a method with signature 'CompareTo(T)' does not implement 'IComparable<T>'.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id cs/wrong-compareto-signature
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+import semmle.code.csharp.frameworks.System
+
+predicate implementsIComparable(ValueOrRefType t, Type paramType) {
+  exists(ConstructedType ct |
+    t.getABaseType+() = ct |
+    ct = any(SystemIComparableTInterface i).getAConstructedGeneric() and
+    paramType = ct.getATypeArgument()
+  )
+  or
+  t instanceof SystemIComparableTInterface
+  or
+  (
+    t.getABaseType*() instanceof SystemIComparableInterface and
+    paramType instanceof ObjectType
+  )
+}
+
+predicate compareToMethod(Method m, Type paramType) {
+  m.hasName("CompareTo")
+  and m.fromSource()
+  and m.isPublic()
+  and m.getReturnType() instanceof IntType
+  and m.getNumberOfParameters() = 1
+  and paramType = m.getAParameter().getType()
+}
+
+from Method m, RefType declaringType, Type actualParamType
+where m.isSourceDeclaration()
+  and declaringType = m.getDeclaringType()
+  and compareToMethod(m, actualParamType)
+  and not implementsIComparable(declaringType, actualParamType)
+select m, "The parameter of this 'CompareTo' method is of type $@, but $@ does not implement 'IComparable<$@>'.",
+  actualParamType, actualParamType.getName(),
+  declaringType, declaringType.getName(),
+  actualParamType, actualParamType.getName()

csharp/ql/src/API Abuse/IncorrectCompareToSignatureBad.cs

@@ -0,0 +1,6 @@
+using System;
+
+class Bad
+{
+    public int CompareTo(Bad b) => 0;
+}

csharp/ql/src/API Abuse/IncorrectCompareToSignatureGood.cs

@@ -0,0 +1,6 @@
+using System;
+
+class Good : IComparable<Good>
+{
+    public int CompareTo(Good g) => 0;
+}

csharp/ql/src/API Abuse/IncorrectEqualsSignature.qhelp

@@ -0,0 +1,33 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The intent of defining an <code>Equals(object)</code> method on a class is generally to ensure
+that instances of that class are compared using it, both in client code and in library code. The 
+standard <code>Equals(object)</code> method has an <code>object</code> parameter, so
+<code>Equals(T)</code> methods where <code>T</code> is not <code>object</code>, will often not be
+used in favor of <code>Equals(object)</code>.</p>
+
+</overview>
+<recommendation>
+<p>Define an <code>Equals(object)</code> method that returns false if the object parameter is not of 
+the type containing the method.</p>
+
+</recommendation>
+<example>
+<p>In this example, the <code>Equals</code> method only takes an instance of <code>Bad</code> as a 
+parameter.</p>
+<sample src="IncorrectEqualsSignatureBad.cs" />
+
+<p>In the revised example, <code>Equals(object)</code> is overridden and defined in terms of the
+<code>Equals(Good)</code> method.
+</p>
+<sample src="IncorrectEqualsSignatureGood.cs" />
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/bsc2ak47.aspx">Object.Equals Method (Object)</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/IncorrectEqualsSignature.ql

@@ -0,0 +1,34 @@
+/**
+ * @name Potentially incorrect Equals(...) signature
+ * @description The declaring type of a method with signature 'Equals(T)' does not implement 'Equals(object)'.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id cs/wrong-equals-signature
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+import semmle.code.csharp.frameworks.System
+
+class EqualsOtherMethod extends Method {
+  EqualsOtherMethod() {
+    this.hasName("Equals")
+    and this.getNumberOfParameters() = 1
+    and this.getReturnType() instanceof BoolType
+    and this.getDeclaringType() instanceof Class
+    and not this instanceof EqualsMethod
+    and not this instanceof IEquatableEqualsMethod
+  }
+
+  Type getType() {
+    result = this.getParameter(0).getType()
+  }
+}
+
+from EqualsOtherMethod equalsOther
+where equalsOther.isSourceDeclaration()
+  and not implementsEquals(equalsOther.getDeclaringType())
+select equalsOther, "The $@ of this 'Equals(" + equalsOther.getType().getName() + ")' method does not override 'Equals(object)'.",
+  equalsOther.getDeclaringType(), "declaring type"

csharp/ql/src/API Abuse/IncorrectEqualsSignatureBad.cs

@@ -0,0 +1,12 @@
+class Bad
+{
+    private int id;
+
+    public Bad(int Id)
+    {
+        this.id = Id;
+    }
+
+    public bool Equals(Bad b) =>
+      this.id == b.id;
+}

csharp/ql/src/API Abuse/IncorrectEqualsSignatureGood.cs

@@ -0,0 +1,19 @@
+class Good
+{
+    private int id;
+
+    public Good(int Id)
+    {
+        this.id = Id;
+    }
+
+    public bool Equals(Good g) =>
+      this.id == g.id;
+
+    public override bool Equals(object o)
+    {
+        if (o is Good g && g.GetType() = typeof(Good))
+            return this.Equals(g);
+        return false;
+    }
+}

csharp/ql/src/API Abuse/MissingDisposeCall.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Classes that implement <code>IDisposable</code> and have members of <code>IDisposable</code> type should
+dispose those members in their <code>Dispose()</code> method.</p>
+
+</overview>
+<recommendation>
+<p>Add a call to <code>m.Dispose()</code> in your <code>Dispose</code> method for each member <code>m</code> that implements <code>IDisposable</code>.</p>
+
+</recommendation>
+<example>
+<p>In this example, the class <code>Bad</code> contains two disposable fields <code>stream1</code> and
+<code>stream2</code>, but only <code>stream1</code> is disposed in <code>Bad</code>'s <code>Dispose()</code>
+method.
+</p>
+<sample src="MissingDisposeCallBad.cs" />
+
+<p>
+In the revised example, both <code>stream1</code> and <code>stream2</code> are disposed.
+</p>
+<sample src="MissingDisposeCallGood.cs" />
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.idisposable.aspx">IDisposable Interface</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/MissingDisposeCall.ql

@@ -0,0 +1,31 @@
+/**
+ * @name Missing Dispose call
+ * @description Classes that implement 'IDisposable' and have members of 'IDisposable' type
+ *              should dispose those members in their 'Dispose()' method.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id cs/member-not-disposed
+ * @tags efficiency
+ *       maintainability
+ *       external/cwe/cwe-404
+ *       external/cwe/cwe-459
+ */
+
+import csharp
+import Dispose
+import semmle.code.csharp.frameworks.System
+
+from DisposableType t, DisposableField f, Method dispose
+where f.getDeclaringType() = t
+  and not f.isStatic()
+  and t.isSourceDeclaration()
+  and dispose = getInvokedDisposeMethod(t)
+  and dispose.getDeclaringType() = t
+  and not exists(MethodCall mc |
+    mc.getTarget() instanceof DisposeMethod
+    and mc.getQualifier() = f.getAnAccess()
+    and mc.getEnclosingCallable() = dispose
+  )
+select dispose, "This 'Dispose()' method does not call 'Dispose()' on `IDisposable` field $@.",
+  f, f.getName()

csharp/ql/src/API Abuse/MissingDisposeCallBad.cs

@@ -0,0 +1,13 @@
+using System;
+using System.IO;
+
+class Bad : IDisposable
+{
+    private FileStream stream1 = new FileStream("a.txt", FileMode.Open);
+    private FileStream stream2 = new FileStream("b.txt", FileMode.Open);
+
+    public void Dispose()
+    {
+        stream1.Dispose();
+    }
+}

csharp/ql/src/API Abuse/MissingDisposeCallGood.cs

@@ -0,0 +1,14 @@
+using System;
+using System.IO;
+
+class Good : IDisposable
+{
+    private FileStream stream1 = new FileStream("a.txt", FileMode.Open);
+    private FileStream stream2 = new FileStream("b.txt", FileMode.Open);
+
+    public void Dispose()
+    {
+        stream1.Dispose();
+        stream2.Dispose();
+    }
+}

csharp/ql/src/API Abuse/MissingDisposeMethod.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Classes that implement <code>IDisposable</code> and have members of <code>IDisposable</code> type
+should also declare/override <code>Dispose()</code>.
+</p>
+
+</overview>
+<recommendation>
+<p>Override the <code>Dispose()</code> method.</p>
+
+</recommendation>
+<example>
+<p>In the following example, <code>Bad</code> extends the <code>IDisposable</code> class <code>BadBase</code>,
+but does not override <code>Dispose()</code>.
+</p>
+<sample src="MissingDisposeMethodBad.cs" />
+
+<p>In the revised example, <code>Good</code> overrides <code>Dispose()</code>.
+</p>
+<sample src="MissingDisposeMethodGood.cs" />
+
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.idisposable.aspx">IDisposable Interface</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/MissingDisposeMethod.ql

@@ -0,0 +1,26 @@
+/**
+ * @name Missing Dispose method
+ * @description Classes that implement 'IDisposable' and have members of 'IDisposable' type
+ *              should also declare/override 'Dispose()'.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id cs/missing-dispose-method
+ * @tags efficiency
+ *       maintainability
+ *       external/cwe/cwe-404
+ *       external/cwe/cwe-459
+ */
+
+import csharp
+import Dispose
+import semmle.code.csharp.frameworks.System
+
+from DisposableType t, DisposableField f
+where f.getDeclaringType() = t
+  and t.isSourceDeclaration()
+  and not f.isStatic()
+  and not implementsDispose(t)
+  and not isAutoDisposedWebControl(f)
+select t, "This type does not override 'Dispose()' but has disposable field $@.",
+  f, f.getName()

csharp/ql/src/API Abuse/MissingDisposeMethodBad.cs

@@ -0,0 +1,17 @@
+using System;
+using System.IO;
+
+class BadBase : IDisposable
+{
+    private FileStream stream1 = new FileStream("a.txt", FileMode.Open);
+
+    public virtual void Dispose()
+    {
+        stream1.Dispose();
+    }
+}
+
+class Bad : BadBase
+{
+    private FileStream stream2 = new FileStream("b.txt", FileMode.Open);
+}

csharp/ql/src/API Abuse/MissingDisposeMethodGood.cs

@@ -0,0 +1,23 @@
+using System;
+using System.IO;
+
+class GoodBase : IDisposable
+{
+    private FileStream stream1 = new FileStream("a.txt", FileMode.Open);
+
+    public virtual void Dispose()
+    {
+        stream1.Dispose();
+    }
+}
+
+class Good : BadBase
+{
+    private FileStream stream2 = new FileStream("b.txt", FileMode.Open);
+
+    public override void Dispose()
+    {
+        base.Dispose();
+        stream2.Dispose();
+    }
+}

csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.qhelp

@@ -0,0 +1,41 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Objects whose type implements <code>IDisposable</code> should be disposed of by calling
+<code>Dispose</code>.
+</p>
+
+</overview>
+<recommendation>
+<p>
+If possible, wrap the allocation of the object in a <code>using</code> block to automatically
+dispose of the object once the <code>using</code> block has completed.
+</p>
+<p>
+If this is not possible, ensure that <code>Dispose</code> is called on the object. It is usually
+recommended to call <code>Dispose</code> within a <code>finally</code> block, to ensure that the
+object is disposed of even if an exception is thrown.
+</p>
+
+</recommendation>
+<example>
+<p>
+In this example, a <code>FileStream</code> is created, but it is not disposed of.
+</p>
+<sample src="NoDisposeCallOnLocalIDisposableBad.cs" />
+<p>
+In the revised example, a <code>using</code> statement is used to ensure that the file
+stream is properly closed.
+</p>
+<sample src="NoDisposeCallOnLocalIDisposableGood.cs" />
+
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/system.idisposable.aspx">IDisposable Interface</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql

@@ -0,0 +1,76 @@
+/**
+ * @name Missing Dispose call on local IDisposable
+ * @description Methods that create objects of type 'IDisposable' should call 'Dispose' on those
+ *              objects, otherwise unmanaged resources may not be released.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cs/local-not-disposed
+ * @tags efficiency
+ *       maintainability
+ *       security
+ *       external/cwe/cwe-404
+ *       external/cwe/cwe-459
+ *       external/cwe/cwe-460
+ */
+
+import csharp
+import Dispose
+import semmle.code.csharp.frameworks.System
+import semmle.code.csharp.commons.Disposal
+
+/** Holds if expression `e` escapes the local method scope. */
+predicate escapes(Expr e) {
+  // Things that return escape
+  exists(Callable c | c.canReturn(e) or c.canYieldReturn(e)) or
+  // Things that are assigned to fields, properties, or indexers escape the scope
+  exists(AssignableDefinition def, Assignable a |
+    def.getSource() = e and
+    a = def.getTarget() |
+    a instanceof Field or
+    a instanceof Property or
+    a instanceof Indexer
+  ) or
+  // Things that are added to a collection of some kind are likely to escape the scope
+  exists(MethodCall mc |
+    mc.getTarget().hasName("Add") |
+    mc.getAnArgument() = e
+  )
+}
+
+/** Holds if the `disposable` is a whitelisted result. */
+predicate isWhitelisted(LocalScopeDisposableCreation disposable) {
+  exists(MethodCall mc |
+    // Close can often be used instead of Dispose
+    mc.getTarget().hasName("Close") or
+    // Used as an alias for Dispose
+    mc.getTarget().hasName("Clear")
+    |
+    mc.getQualifier() = disposable.getADisposeTarget()
+  ) or
+  // WebControls are usually disposed automatically
+  disposable.getType() instanceof WebControl
+}
+
+from LocalScopeDisposableCreation disposable
+  // The disposable is local scope - the lifetime is the execution of this method
+where not escapes(disposable.getADisposeTarget())
+  // Only care about library types - user types often have spurious IDisposable declarations
+  and disposable.getType().fromLibrary()
+  // Disposables constructed in the initializer of a `using` are safe
+  and not exists(UsingStmt us | us.getAnExpr() = disposable.getADisposeTarget())
+  // Foreach calls Dispose
+  and not exists(ForeachStmt fs | fs.getIterableExpr() = disposable.getADisposeTarget())
+  // As are disposables on which the Dispose method is called explicitly
+  and not exists(MethodCall mc |
+    mc.getTarget() instanceof DisposeMethod
+    and mc.getQualifier() = disposable.getADisposeTarget()
+  )
+  // Ignore whitelisted results
+  and not isWhitelisted(disposable)
+  // Not passed to a disposing method
+  and not exists(Call c, Parameter p |
+    disposable.getADisposeTarget() = c.getArgumentForParameter(p) |
+    mayBeDisposed(p)
+  )
+select disposable, "Disposable '" + disposable.getType() + "' is created here but is not disposed."

csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposableBad.cs

@@ -0,0 +1,11 @@
+using System;
+using System.IO;
+
+class Bad
+{
+    long GetLength(string file)
+    {
+        var stream = new FileStream(file, FileMode.Open);
+        return stream.Length;
+    }
+}

csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposableGood.cs

@@ -0,0 +1,11 @@
+using System;
+using System.IO;
+
+class Good
+{
+    long GetLength(string file)
+    {
+        using (var stream = new FileStream(file, FileMode.Open))
+            return stream.Length;
+    }
+}

csharp/ql/src/API Abuse/NonOverridingMethod.qhelp

@@ -0,0 +1,37 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>It is generally very easy to make sure you are overriding the method you intend in C# - you use 
+the <code>override</code> keyword and the compiler will detect any errors you make. However, it is 
+still possible to add methods that appear to override something but do not override because they do 
+not use the <code>override</code> keyword or are misspelt. Methods of this type are confusing to 
+other programmers and may act unexpectedly.</p>
+
+</overview>
+<recommendation>
+<p>The appropriate solution involves carefully examining the method in question: if it should be 
+overriding another method, change it accordingly; if not, rename or remove it to avoid further 
+confusion.</p>
+
+</recommendation>
+<example>
+<p>In the following example, the <code>Sub</code> class introduces the <code>Foo()</code> method,
+but hides <code>Super.Foo()</code> instead of overriding it.
+</p>
+<sample src="NonOverridingMethodBad.cs" />
+
+</example>
+<example>
+<p>In the revised example, <code>Sub.Foo()</code> overrides <code>Super.Foo()</code>.
+</p>
+<sample src="NonOverridingMethodGood.cs" />
+
+</example>
+<references>
+
+  <li>Microsoft: <a href="https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/override">override (C# Reference)</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/NonOverridingMethod.ql

@@ -0,0 +1,43 @@
+/**
+ * @name Non-overriding method
+ * @description A method looks like it should override a virtual method from a base type, but does not actually do so.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision medium
+ * @id cs/nonoverriding-method
+ * @tags reliability
+ *       readability
+ *       naming
+ */
+
+import csharp
+
+private predicate potentialOverride(Method vm, Method m) {
+  vm.getDeclaringType() = m.getDeclaringType().getBaseClass+()
+}
+
+/**
+ * Holds if method `m` looks like it should override the virtual method `vm`,
+ * but does not do so.
+ */
+predicate nonOverridingMethod(Method m, Method vm) {
+  vm.isVirtual()
+  and not vm.isOverride()
+  and not vm.overrides()
+  and potentialOverride(vm, m)
+  and not m.overrides()
+  and not m.isOverride()
+  and not m.isNew()
+  and m=m.getSourceDeclaration()
+  and m.getNumberOfParameters() = vm.getNumberOfParameters()
+  and forall(int i, Parameter p1, Parameter p2 |
+    p1=m.getParameter(i) and p2=vm.getParameter(i) |
+    p1.getType() = p2.getType())
+  and m.getName().toLowerCase() = vm.getName().toLowerCase()
+}
+
+from Method m, Method vm
+where m.fromSource()
+  and nonOverridingMethod(m, vm)
+select m, "Method '" + m.getName() + "' looks like it should override $@ but does not do so.",
+  vm.getSourceDeclaration(), vm.getQualifiedName()

csharp/ql/src/API Abuse/NonOverridingMethodBad.cs

@@ -0,0 +1,12 @@
+class Bad
+{
+    class Super
+    {
+        public virtual void Foo() { }
+    }
+
+    class Sub : Super
+    {
+        public void Foo() { }
+    }
+}

csharp/ql/src/API Abuse/NonOverridingMethodGood.cs

@@ -0,0 +1,12 @@
+class Good
+{
+    class Super
+    {
+        public virtual void Foo() { }
+    }
+
+    class Sub : Super
+    {
+        public override void Foo() { }
+    }
+}

csharp/ql/src/API Abuse/NullArgumentToEquals.qhelp

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>It is common to want to check an object against <code>null</code>, but this should not be done 
+using the <code>Equals</code> method. If the object really is <code>null</code>, a <code>
+NullReferenceException</code> is thrown when attempting to call <code>Equals</code>, with 
+unexpected results.</p>
+
+</overview>
+<recommendation>
+<p>The offending call should be replaced with either <code>==</code> or <code>ReferenceEquals</code>
+ (the difference being that <code>==</code> can be overridden but <code>ReferenceEquals</code> 
+cannot).</p>
+
+</recommendation>
+<example>
+<p>In the following example, <code>IsNull</code> will throw a <code>NullReferenceException</code>
+when <code>o</code> is <code>null</code>.
+</p>
+<sample src="NullArgumentToEqualsBad.cs" />
+
+<p>In the revised example, <code>IsNull</code> will correctly return <code>true</code>
+when <code>o</code> is <code>null</code>.
+</p>
+<sample src="NullArgumentToEqualsGood.cs" />
+</example>
+<references>
+
+  <li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/bsc2ak47.aspx">Object.Equals Method (Object)</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/NullArgumentToEquals.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Null argument to Equals(object)
+ * @description Calls of the form 'o.Equals(null)' always return false for non-null 'o', and
+ *              throw a 'NullReferenceException' when 'o' is null.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cs/null-argument-to-equals
+ * @tags reliability
+ *       correctness
+ */
+
+import csharp
+import semmle.code.csharp.frameworks.System
+
+from MethodCall c, EqualsMethod equals
+where c.getTarget().getSourceDeclaration() = equals
+  and c.getArgument(0) instanceof NullLiteral
+  and not c.getQualifier().getType() instanceof NullableType
+select c, "Equality test with 'null' will never be true, but may throw a 'NullReferenceException'."

csharp/ql/src/API Abuse/NullArgumentToEqualsBad.cs

@@ -0,0 +1,4 @@
+class Bad
+{
+    bool IsNull(object o) => o.Equals(null);
+}

csharp/ql/src/API Abuse/NullArgumentToEqualsGood.cs

@@ -0,0 +1,4 @@
+class Good
+{
+    bool IsNull(object o) => o == null;
+}

csharp/ql/src/API Abuse/UncheckedReturnValue.qhelp

@@ -0,0 +1,32 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Ignoring a method's return value can be a security risk and a common source of defects. If an 
+attacker can force the method to fail, the subsequent program logic could lead to a vulnerability 
+because the program will be running in an unexpected state. This rule checks that the return value 
+of standard library methods like <code>System.IO.Stream.Read(...)</code> is always used. 
+Furthermore, it identifies any calls to methods that ignore the return value despite it being 
+frequently used elsewhere. That is, if more than 90% of the total calls to a particular method use 
+its return value, all other calls that discard its return value are flagged as suspicious.</p>
+
+</overview>
+<recommendation>
+<p>Check the return value of the method and take appropriate action.</p>
+
+</recommendation>
+<example>
+<p>In the method <code>IgnoreOne</code>, there are lots of calls to <code>DoPrint</code> where the 
+return value is checked. However, the return value of <code>DoPrint("J")</code> is not checked.</p>
+<p>In the method <code>IgnoreRead</code>, the result of the call to <code>FileStream.Read</code> is
+ignored. This is dangerous, as the amount of data read might be less than the length of the array it
+is being read into.</p>
+<sample src="UncheckedReturnValueBad.cs" />
+
+</example>
+<references>
+
+
+</references>
+</qhelp>

csharp/ql/src/API Abuse/UncheckedReturnValue.ql

@@ -0,0 +1,115 @@
+/**
+ * @name Unchecked return value
+ * @description If most of the calls to a method use the return value
+ *              of that method, the calls that do not check the return value may be mistakes.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id cs/unchecked-return-value
+ * @tags reliability
+ *       correctness
+ *       external/cwe/cwe-252
+ *       statistical
+ *       non-attributable
+ */
+import csharp
+import semmle.code.csharp.frameworks.system.IO
+import semmle.code.csharp.Chaining
+
+/** Holds if `m` is a method whose return value should always be checked. */
+predicate important(Method m) {
+  exists(Method read |
+    read = any(SystemIOStreamClass c).getReadMethod() |
+    m = read.getAnOverrider*()
+  )
+  or
+  exists(Method readByte |
+    readByte = any(SystemIOStreamClass c).getReadByteMethod() |
+    m = readByte.getAnOverrider*()
+  )
+  // add more ...
+}
+
+/** Holds if the return type of `m` is an instantiated type parameter from `m`. */
+predicate methodHasGenericReturnType(ConstructedMethod cm) {
+  exists(UnboundGenericMethod ugm |
+    ugm = cm.getSourceDeclaration() and
+    ugm.getReturnType() = ugm.getATypeParameter()
+  )
+}
+
+/** Holds if `m` is a method whose return value should be checked because most calls to `m` do. */
+// statistically dubious:
+predicate dubious(Method m, int percentage) {
+  not important(m) and
+  // Suppress on Void methods
+  not m.getReturnType() instanceof VoidType and
+  // Suppress on methods designed for chaining
+  not designedForChaining(m) and
+  exists(int used, int total, Method target |
+    target = m.getSourceDeclaration() and
+    used = count(MethodCall mc |
+      mc.getTarget().getSourceDeclaration() = target and
+      not mc instanceof DiscardedMethodCall and
+      (methodHasGenericReturnType(m) implies m.getReturnType() = mc.getTarget().getReturnType())
+    ) and
+    total = count(MethodCall mc |
+      mc.getTarget().getSourceDeclaration() = target and
+      (methodHasGenericReturnType(m) implies m.getReturnType() = mc.getTarget().getReturnType())
+    ) and
+    used != total and
+    percentage = used * 100 / total and
+    percentage >= 90 and
+    chainedUses(m) * 100 / total <= 45 // no more than 45% of calls to this method are chained
+  )
+}
+
+int chainedUses(Method m) {
+  result = count(MethodCall mc, MethodCall qual |
+    m = mc.getTarget() and
+    hasQualifierAndTarget(mc, qual, qual.getTarget())
+  )
+}
+
+predicate hasQualifierAndTarget(MethodCall mc, Expr qualifier, Method m) {
+  qualifier = mc.getQualifier() and
+  m = mc.getTarget()
+}
+
+/** Holds if `m` is a white-listed method where checking the return value is not required. */
+predicate whitelist(Method m) {
+  m.hasName("TryGetValue") or
+  m.hasName("TryParse") or
+  exists(Namespace n |
+    n = m.getDeclaringType().getNamespace().getParentNamespace*() |
+    n.getName().regexpMatch("(Fluent)?NHibernate") or
+    n.getName() = "Moq"
+  )
+  // add more ...
+}
+
+class DiscardedMethodCall extends MethodCall {
+  DiscardedMethodCall() {
+    this.getParent() instanceof ExprStmt
+  }
+
+  string query() {
+    exists(Method m |
+      m = getTarget() and
+      not whitelist(m) and
+      // Do not alert on "void wrapper methods", i.e., methods that are inserted
+      // to deliberately ignore the returned value
+      not getEnclosingCallable().getStatementBody().getNumberOfStmts() = 1 |
+      (important(m) and result = "should always be checked")
+      or
+      exists(int percentage |
+        dubious(m, percentage) |
+        result = percentage.toString() + "% of calls to this method have their result used"
+      )
+    )
+  }
+}
+
+from DiscardedMethodCall dmc, string message
+where message = dmc.query()
+select dmc, "Result of call to '" + dmc.getTarget().getName() + "' is ignored, but " + message + "."

csharp/ql/src/API Abuse/UncheckedReturnValueBad.cs

@@ -0,0 +1,38 @@
+using System;
+using System.IO;
+
+class Bad
+{
+    public bool DoPrint(string s) => true;
+
+    public void IgnoreOne()
+    {
+        if (DoPrint("A"))
+            Console.WriteLine("A");
+        if (DoPrint("B"))
+            Console.WriteLine("B");
+        if (DoPrint("C"))
+            Console.WriteLine("C");
+        if (DoPrint("D"))
+            Console.WriteLine("D");
+        if (DoPrint("E"))
+            Console.WriteLine("E");
+        if (DoPrint("F"))
+            Console.WriteLine("F");
+        if (DoPrint("G"))
+            Console.WriteLine("G");
+        if (DoPrint("H"))
+            Console.WriteLine("H");
+        if (DoPrint("I"))
+            Console.WriteLine("I");
+
+        DoPrint("J");
+    }
+
+    void IgnoreRead(string path)
+    {
+        var file = new byte[10];
+        using (var f = new FileStream(path, FileMode.Open))
+            f.Read(file, 0, file.Length);
+    }
+}

csharp/ql/src/ASP/BlockCodeResponseWrite.qhelp

@@ -0,0 +1,41 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>An inline code block containing a single <code>Response.Write()</code>
+can be written more clearly using an inline expression.</p>
+
+<p>ASP.NET provides general-purpose <em>inline code</em>, using the
+syntax "<code>&lt;%...%></code>". The inline code can emit content
+into the resulting HTML page by calling <code>Response.Write()</code>.</p>
+
+<p>In many cases, the inline code is only one line long, and does nothing more
+than issue a single call to <code>Response.Write()</code>. For such cases, the
+call to <code>Response.Write()</code> can be longer than the code to compute
+what will be embedded. This makes it harder to understand the intent of the code.</p>
+</overview>
+
+<recommendation>
+<p>ASP.NET also provides <em>inline expressions</em>, using
+the syntax "<code>&lt;%=...></code>". An inline expression does not need
+to call <code>Response.Write()</code>. The equals sign (=)
+is a concise way to tell ASP.NET to call <code>Response.Write()</code>.</p>
+</recommendation>
+
+<example>
+<p>This example shows a page where an inline code block writes
+content using <code>Response.Write()</code>.</p>
+<sample src="BlockCodeResponseWriteBad.aspx" />
+<p>In the following example, the code block is replaced with an inline expression,
+and is thus more concise and direct.</p>
+<sample src="BlockCodeResponseWriteGood.aspx" />
+</example>
+
+<references>
+<li>Microsoft: <a href="https://support.microsoft.com/en-us/help/976112/introduction-to-asp-net-inline-expressions-in-the--net-framework">Introduction
+    to ASP.NET inline expressions in the .NET Framework</a>.</li>
+<li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/ms973868.aspx">Introduction to ASP.NET and Web Forms</a>,
+<a href="https://msdn.microsoft.com/en-us/library/fy30at8h(v=vs.100).aspx">ASP.NET Page Syntax</a>.</li>
+</references>
+</qhelp>

csharp/ql/src/ASP/BlockCodeResponseWrite.ql

@@ -0,0 +1,16 @@
+/**
+ * @name Block code with a single Response.Write()
+ * @description Embedded code blocks with a single 'Response.Write()' reduce the readability of the page.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision high
+ * @id cs/asp/response-write
+ * @tags maintainability
+ *       frameworks/asp.net
+ */
+
+import semmle.code.asp.AspNet
+
+from AspBlockCode code
+where code.getBody().trim().matches("Response.Write(%")
+select code, "Non-inline code calls 'Response.Write()'."

csharp/ql/src/ASP/BlockCodeResponseWriteBad.aspx

@@ -0,0 +1,7 @@
+<%@ Page Language="C#" %>
+
+<html>
+<body>
+<p>2 + 3 = <%Response.Write(2 + 3)%></p>
+</body>
+</html>

csharp/ql/src/ASP/BlockCodeResponseWriteGood.aspx

@@ -0,0 +1,7 @@
+<%@ Page Language="C#" %>
+
+<html>
+<body>
+<p>2 + 3 = <%=2 + 3%></p>
+</body>
+</html>

csharp/ql/src/ASP/ComplexInlineCode.qhelp

@@ -0,0 +1,35 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>ASP.NET allows arbitrary amounts of code to be embedded into a page. If
+that code is lengthy, then the overall page can be harder to read.
+The code itself does not fully benefit from IDE features, and the
+HTML content of the page is harder to read due to being obstructed
+by the code.</p>
+</overview>
+
+<recommendation>
+<p>Consider updating the page to use the Code-Behind Page Model
+(see <a href="https://msdn.microsoft.com/en-us/library/015103yb.aspx">ASP.NET
+Web Page Code Model for details</a>). In this model the markup and
+programming code are stored in separate files for easier maintenance.</p>
+</recommendation>
+
+<example>
+<p>This example uses a large amount of code in the middle of a page.</p>
+<sample src="ComplexInlineCodeBad.aspx" />
+<p>In the following example, the code is stored in a code-behind
+file. This separation of the HTML content from the VB.Net content
+makes the intention of the ASP page clearer and also simplifies reuse
+of the code.</p>
+<sample src="ComplexInlineCodeGood.aspx" />
+</example>
+
+<references>
+<li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/ms973868.aspx">Introduction to ASP.NET and Web Forms</a>,
+<a href="https://msdn.microsoft.com/en-us/library/fy30at8h(v=vs.100).aspx">ASP.NET Page Syntax</a>,
+<a href="https://msdn.microsoft.com/en-us/library/015103yb.aspx">ASP.NET Web Page Code Model</a>.</li>
+</references>
+</qhelp>

csharp/ql/src/ASP/ComplexInlineCode.ql

@@ -0,0 +1,16 @@
+/**
+ * @name Complex inline code
+ * @description Embedding complex inline code makes .aspx pages more difficult to maintain.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id cs/asp/complex-inline-code
+ * @tags maintainability
+ *       frameworks/asp.net
+ */
+
+import semmle.code.asp.AspNet
+
+from AspCode code
+where code.getBody().matches("%\n%")
+select code, "Inline code contains multi-line logic."

csharp/ql/src/ASP/ComplexInlineCodeBad.aspx

@@ -0,0 +1,20 @@
+<%@ Page Language="C#" %>
+
+<html>
+<body>
+<%
+  if (builder == null)
+    builder = ec.GetTemporaryLocal (type);
+
+  if (builder.LocalType.IsByRef) {
+    //
+    // if is_address, than this is just the address anyways,
+    // so we just return this.
+    //
+    ec.Emit (Response, OpCodes.Ldloc, builder);
+  } else {
+    ec.Emit (Response, OpCodes.Ldloca, builder);
+  }
+%>
+</body>
+</html>

csharp/ql/src/ASP/ComplexInlineCodeGood.aspx

@@ -0,0 +1,7 @@
+<%@ Page Language="C#" %>
+
+<html>
+<body>
+<%= emitLoad() %>
+</body>
+</html>

csharp/ql/src/ASP/NonInternationalizedText.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Some websites are internationalized, meaning they are implemented
+in a way that supports translation and adaption into multiple natural languages.
+For such a website, it is an error to embed English text directly into the page.</p>
+</overview>
+
+<recommendation>
+<p>Consider whether the text needs to be translated for each language
+the website supports. If so, then the text should be replaced by
+a code expression that retrieves the appropriate translation of the text.</p>
+</recommendation>
+
+<example>
+<p>In the following example, the English word "Amount" is included directly
+in the page.</p>
+<sample src="NonInternationalizedTextBad.aspx" />
+<p>
+The revised example makes use of a hypothetical "I18n" library that is
+used to retrieve the translation of a given string of text into whichever
+language is currently in effect. The correct internationalization library
+to use will depend on the website, and might not be called exactly "I18n".
+</p>
+<sample src="NonInternationalizedTextGood.aspx" />
+</example>
+
+<references>
+<li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/ms973868.aspx">Introduction to ASP.NET and Web Forms</a>,
+<a href="https://msdn.microsoft.com/en-us/library/fy30at8h(v=vs.100).aspx">ASP.NET Page Syntax</a>.</li>
+<li>W3C: <a href="http://www.w3.org/standards/webdesign/i18n">Internationalization</a>,
+<a href="http://www.w3.org/International/questions/qa-i18n">Localization vs. Internationalization</a>.</li>
+</references>
+</qhelp>

csharp/ql/src/ASP/NonInternationalizedText.ql

@@ -0,0 +1,17 @@
+/**
+ * @name Text has not been internationalized
+ * @description Including text directly on a page makes it more difficult to internationalize the website.
+ * @kind problem
+ * @problem.severity warning
+ * @precision low
+ * @id cs/asp/text-not-internationalized
+ * @tags maintainability
+ *       frameworks/asp.net
+ */
+
+import semmle.code.asp.AspNet
+
+from AspText text
+where
+  exists(text.getBody().regexpFind("\\w{3,}", _, _))
+select text, "This text has not been internationalized."

csharp/ql/src/ASP/NonInternationalizedTextBad.aspx

@@ -0,0 +1,7 @@
+<%@ Page Language="C#" %>
+
+<html>
+<body>
+<p>Amount: <%= Amount %></p>
+</body>
+</html>

csharp/ql/src/ASP/NonInternationalizedTextGood.aspx

@@ -0,0 +1,7 @@
+<%@ Page Language="C#" %>
+
+<html>
+<body>
+<p><%= I18n.getMessage("Amount") %>: <%= Amount %></p>
+</body>
+</html>

csharp/ql/src/ASP/SplitControlStructure.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>ASP.NET is very flexible about the code that can be embedded into
+a page. It is not necessary that the individual code render blocks
+are valid syntax on their own. Rather, it is only necessary that
+all the code render blocks can compile as a group, once they are
+all emitted into a class that the ASP.NET infrastructure
+generates to implement the page.</p>
+
+<p>This flexibility is sometimes used to implement conditional
+portions of an HTML page. One code render block will have the beginning
+of an <code>If...Then</code> statement, and another will have the <code>End If</code>,
+with arbitrary ASP content in between.
+This conditional code can make the source code of the
+page hard to read and maintain, especially if the embedded ASP content
+has further conditional code within it.</p>
+</overview>
+
+<recommendation>
+<p>Consider moving the entire block of conditional code into a code-behind file.
+If the embedded ASP content is more than a few lines long, consider converting
+the ASP content into a user control.
+See <a href="https://msdn.microsoft.com/en-us/library/015103yb.aspx">ASP.NET Web Page Code Model</a>
+and <a href="https://msdn.microsoft.com/en-us/library/y6wb1a0e.aspx">ASP.NET User Controls</a>.</p>
+</recommendation>
+
+<example>
+<p>This example uses a split control structure, which can make the page harder
+to read.</p>
+<sample src="SplitControlStructureBad.aspx" />
+<p>The following example moves the code to a code-behind file. By separating the
+HTML content from the VB.Net content, the overall body of code should be easier
+to read and maintain.</p>
+<sample src="SplitControlStructureGood.aspx" />
+</example>
+
+<references>
+<li>MSDN: <a href="https://msdn.microsoft.com/en-us/library/ms973868.aspx">Introduction to ASP.NET and Web Forms</a>,
+<a href="https://msdn.microsoft.com/en-us/library/fy30at8h(v=vs.100).aspx">ASP.NET Page Syntax</a>,
+<a href="https://msdn.microsoft.com/en-us/library/y6wb1a0e.aspx">ASP.NET User Controls</a>,
+<a href="https://msdn.microsoft.com/en-us/library/015103yb.aspx">ASP.NET Web Page Code Model</a>.</li>
+</references>
+</qhelp>

csharp/ql/src/ASP/SplitControlStructure.ql

@@ -0,0 +1,17 @@
+/**
+ * @name Split control structure
+ * @description Splitting control structures across multiple code blocks makes .aspx pages more difficult to maintain.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision medium
+ * @id cs/asp/split-control-structure
+ * @tags maintainability
+ *       frameworks/asp.net
+ */
+
+import semmle.code.asp.AspNet
+
+from AspCode code
+where
+  exists(code.getBody().regexpFind("(Then|\\{)\\s*$",_,_))
+select code, "Split control structure."

csharp/ql/src/ASP/SplitControlStructureBad.aspx

@@ -0,0 +1,9 @@
+<%@ Page Language="VB" %>
+
+<html>
+<body>
+<% If ShouldWarn() Then %>
+<p>WARNING: <%=warning()%></p>
+<% End If %>
+</body>
+</html>

csharp/ql/src/ASP/SplitControlStructureGood.aspx

@@ -0,0 +1,7 @@
+<%@ Page Language="VB" %>
+
+<html>
+<body>
+<%=maybeEmitWarning()%>
+</body>
+</html>

csharp/ql/src/AlertSuppression.ql

@@ -0,0 +1,81 @@
+/**
+ * @name Alert suppression
+ * @description Generates information about alert suppressions.
+ * @kind alert-suppression
+ * @id cs/alert-suppression
+ */
+
+import csharp
+
+/**
+ * An alert suppression comment.
+ */
+class SuppressionComment extends SinglelineComment {
+  string annotation;
+
+  SuppressionComment() {
+    exists(string text |
+      text = this.getText() |
+      // match `lgtm[...]` anywhere in the comment
+      annotation = text.regexpFind("(?i)\\blgtm\\s*\\[[^\\]]*\\]", _, _)
+      or
+      // match `lgtm` at the start of the comment and after semicolon
+      annotation = text.regexpFind("(?i)(?<=^|;)\\s*lgtm(?!\\B|\\s*\\[)", _, _).trim()
+    )
+  }
+
+  /** Gets the suppression annotation in this comment. */
+  string getAnnotation() {
+    result = annotation
+  }
+
+  /**
+  * Holds if this comment applies to the range from column `startcolumn` of line `startline`
+  * to column `endcolumn` of line `endline` in file `filepath`.
+  */
+  predicate covers(string filepath, int startline, int startcolumn, int endline, int endcolumn) {
+    this.getLocation().hasLocationInfo(filepath, startline, _, endline, endcolumn) and
+    startcolumn = 1
+  }
+
+  /** Gets the scope of this suppression. */
+  SuppressionScope getScope() {
+    this = result.getSuppressionComment()
+  }
+}
+
+/**
+ * The scope of an alert suppression comment.
+ */
+class SuppressionScope extends @commentline {
+  SuppressionScope() {
+    this instanceof SuppressionComment
+  }
+
+  /** Gets a suppression comment with this scope. */
+  SuppressionComment getSuppressionComment() {
+    result = this
+  }
+
+  /**
+  * Holds if this element is at the specified location.
+  * The location spans column `startcolumn` of line `startline` to
+  * column `endcolumn` of line `endline` in file `filepath`.
+  * For more information, see
+  * [LGTM locations](https://lgtm.com/help/ql/locations).
+  */
+  predicate hasLocationInfo(string filepath, int startline, int startcolumn, int endline, int endcolumn) {
+    this.(SuppressionComment).covers(filepath, startline, startcolumn, endline, endcolumn)
+  }
+
+  /** Gets a textual representation of this element. */
+  string toString() {
+    result = "suppression range"
+  }
+}
+
+from SuppressionComment c
+select c,                 // suppression comment
+       c.getText(),       // text of suppression comment (excluding delimiters)
+       c.getAnnotation(), // text of suppression annotation
+       c.getScope()       // scope of suppression

csharp/ql/src/Architecture/Dependencies/MutualDependency.qhelp

@@ -0,0 +1,82 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+A mutual dependency exists when two types depend directly on each other.
+Mutual dependencies are caused by unwanted dependencies in one or both directions. There
+are many different kinds of dependency; here are a few examples of how an inter-type dependency
+from <code>T1</code> to <code>T2</code> can occur:
+</p>
+
+<ul>
+<li><code>T1</code> derives from a type involving <code>T2</code>, for example <code>T2</code> itself or <code>List&lt;T2&gt;</code>.</li>
+<li><code>T1</code> declares a field of a type involving <code>T2</code>.</li>
+<li><code>T1</code> declares a method whose return type involves <code>T2</code>.</li>
+<li>A method of <code>T1</code> declares a local variable whose type involves <code>T2</code>.</li>
+<li>A method of <code>T1</code> catches an exception of a type involving <code>T2</code>.</li>
+</ul>
+
+<p>
+Mutual dependencies prevent you from considering either entity in isolation,
+affecting readability and testability. For example, if types <code>T1</code> and <code>T2</code> depend on each other, then it is
+generally impossible to fully understand <code>T1</code> without understanding <code>T2</code>, and vice-versa. Moreover, neither
+type can be tested without the other being present. Whilst mocking can alleviate this latter problem
+to some extent, breaking the mutual dependency is a better solution. For example, suppose we could
+remove all of the dependencies from <code>T2</code> to <code>T1</code> - in that case, we would be able to test <code>T2</code> in isolation, and
+completely side-step the need to provide a <code>T1</code>, mocked or otherwise.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Breaking mutual dependencies involves finding ways of removing the unwanted individual dependencies that
+cause them. The way to do this depends on the kind of dependency in question, with some kinds (for example,
+dependencies caused by inheritance) being much harder to break than others. A full list of ways to
+break cycles is beyond the scope of this help topic, however, a few high-level techniques
+for breaking a dependency from <code>T1</code> to <code>T2</code> include:
+</p>
+
+<ul>
+<li>
+Introducing an interface that is implemented by <code>T2</code>. <code>T1</code>
+can then be refactored to use <code>T2</code> only via the interface, which breaks the cycle.
+</li>
+
+<li>
+Moving the depended-on code in <code>T2</code> to a third (possibly new) entity.
+<code>T1</code> can then depend on this third entity instead of on <code>T2</code>,
+breaking the cycle. <code>T2</code> is allowed to depend on the third entity as
+well, although it does not have to if there is no need.
+</li>
+
+<li>
+Merging <code>T1</code> and <code>T2</code> together (for example, if there was an artificial separation between two parts
+of the same concept). This is not a generally-applicable solution, but is sometimes the right thing to
+do. It has the effect of internalizing the cycle, which is sufficient to solve the problem.
+</li>
+</ul>
+
+<p>
+For more information on how to break
+unwanted dependencies, see the references (particularly [Lakos]).
+</p>
+
+</recommendation>
+<example>
+<p>In this example <code>BadModel</code> and <code>BadView</code> are mutually dependent.</p>
+<sample src="MutualDependencyBad.cs" />
+<p>The interface technique can be used to break the dependency between the model and the view. The <code>IModelListener</code> interface allows <code>BetterView</code> to interact with <code>BetterModel</code> without dependency.</p>
+<sample src="MutualDependencyGood.cs" /> 
+
+</example>
+<references>
+
+
+  <li>J. Lakos. <em>Large-Scale C++ Software Design</em>. Addison-Wesley, 1996.</li>
+  <li>M. Fowler. <em>Refactoring</em>. Addison-Wesley, 1999.</li>
+
+</references>
+</qhelp>

csharp/ql/src/Architecture/Dependencies/MutualDependency.ql

@@ -0,0 +1,46 @@
+/**
+ * @name Mutually-dependent types
+ * @description Mutual dependency between types makes code difficult to understand and test.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision low
+ * @id cs/mutually-dependent-types
+ * @tags testability
+ *       maintainability
+ *       modularity
+ */
+
+import csharp
+import semmle.code.csharp.metrics.Coupling
+
+/** inner is nested (possibly more than one level deep) within outer */
+predicate nestedWithin(ValueOrRefType outer, NestedType inner) {
+  inner.getDeclaringType() = outer
+  or nestedWithin(outer, inner.getDeclaringType())
+}
+
+from ValueOrRefType t1, ValueOrRefType t2
+where
+  t1 != t2
+  and depends(t1, t2)
+  and depends(t2, t1)
+
+  // PREVENT SYMMETRICAL RESULTS
+  and t1.getName() < t2.getName()
+
+  // ADDITIONAL CONSTRAINTS
+  and t1.fromSource()
+  and t2.fromSource()
+
+  // EXCLUSIONS
+  and not
+  (
+    nestedWithin(t1, t2)
+    or nestedWithin(t2, t1)
+    or t1.getName().toLowerCase().matches("%visitor%")
+    or t2.getName().toLowerCase().matches("%visitor%")
+    or t1.getAMember().getName().toLowerCase().matches("%visit%")
+    or t2.getAMember().getName().toLowerCase().matches("%visit%")
+  )
+select t1, "This type and type $@ are mutually dependent.",
+  t2, t2.getName()

csharp/ql/src/Architecture/Dependencies/MutualDependencyBad.cs

@@ -0,0 +1,39 @@
+public class Bad
+{
+    private class BadModel
+    {
+        private int i;
+        private BadView view;
+
+        public int GetI()
+        {
+            return i;
+        }
+
+        public void SetI(int i)
+        {
+            this.i = i;
+            if (view != null) view.ModelChanged();
+        }
+
+        public void SetView(BadView view)
+        {
+            this.view = view;
+        }
+    }
+
+    private class BadView
+    {
+        private BadModel model;
+
+        public BadView(BadModel model)
+        {
+            this.model = model;
+        }
+
+        public void ModelChanged()
+        {
+            System.Console.WriteLine("Model Changed: " + model.GetI());
+        }
+    }
+}

csharp/ql/src/Architecture/Dependencies/MutualDependencyGood.cs

@@ -0,0 +1,44 @@
+public class Good
+{
+    private interface IModelListener
+    {
+        void ModelChanged();
+    }
+
+    private class BetterModel
+    {
+        private int i;
+        private IModelListener listener;
+
+        public int GetI()
+        {
+            return i;
+        }
+
+        public void SetI(int i)
+        {
+            this.i = i;
+            if (listener != null) listener.ModelChanged();
+        }
+
+        public void SetListener(IModelListener listener)
+        {
+            this.listener = listener;
+        }
+    }
+
+    private class BetterView : IModelListener
+    {
+        private BetterModel model;
+
+        public BetterView(BetterModel model)
+        {
+            this.model = model;
+        }
+
+        public void ModelChanged()
+        {
+            System.Console.WriteLine("Model Changed: " + model.GetI());
+        }
+    }
+}

csharp/ql/src/Architecture/Refactoring Opportunities/FeatureEnvy.qhelp

@@ -0,0 +1,64 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p><em>Feature envy</em> refers to situations where a method is "in the wrong place", because
+it does not use many methods or variables of its own class, but uses a whole range of methods or variables from
+some other class. This violates the principle of putting data and behavior in the
+same place, and exposes internals of the other class to the method.</p>
+
+</overview>
+<recommendation>
+
+<p>For each method that may exhibit feature envy, see if it needs to be declared in
+its present location, or if you can move it to the class it is "envious" of.
+A common example is a method that calls a large number of getters on
+another class to perform a calculation that does not rely on
+anything from its own class. In such cases, you should move the method to the class containing the data. 
+If the calculation depends on some values from the method's current class, they can either be passed as
+arguments or accessed using getters from the other class.</p>
+
+<p>If it is inappropriate to move the entire method, see if all the dependencies
+on the other class are concentrated in just one part of the method. If so, you can move them into a method
+of their own. You can then move this method to the other class and call it from the original method.</p>
+
+<p>If a class is envious of functionality defined in a superclass, perhaps the
+superclass needs to be rewritten to become more extensible and allow its subtypes to
+define new behavior without them depending so deeply on the superclass's implementation. The
+<em>template method</em> pattern may be useful in achieving this.</p>
+
+<p>Modern IDEs provide several refactorings that may be useful in addressing
+instances of feature envy, typically under the names of "Move method" and "Extract
+method".</p>
+
+<p>Occasionally, behavior can be misinterpreted as feature envy when in fact it is justified. The
+most common examples are complex design patterns like <em>visitor</em> or <em>strategy</em>, where
+the goal is to separate data from behavior.
+
+</p></recommendation>
+<example>
+<p>In the following example, the method <code>GetTotalPrice</code> is in the
+<code>Basket</code> class, but it only uses data belonging to the <code>Item</code> class.
+Therefore, it represents an instance of feature envy.
+</p>
+
+<sample src="FeatureEnvyBad.cs" />
+
+<p>In the revised example, <code>GetTotalPrice</code> is moved to the <code>Item</code> class and its
+parameter is removed.</p>
+
+<sample src="FeatureEnvyGood.cs" />
+
+</example>
+<references>
+
+<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>
+<li>W. C. Wake, <em>Refactoring Workbook</em>, pp. 93&ndash;94. Addison-Wesley Professional, 2004.
+</li>
+</references>
+</qhelp>

csharp/ql/src/Architecture/Refactoring Opportunities/FeatureEnvy.ql

@@ -0,0 +1,82 @@
+/**
+ * @name Feature envy
+ * @description A method that uses more methods or variables from another (unrelated) class than
+ *              from its own class violates the principle of putting data and behavior in the same
+ *              place.
+ * @kind problem
+ * @precision low
+ * @problem.severity recommendation
+ * @id cs/method-feature-envy
+ * @tags changeability
+ *       maintainability
+ *       modularity
+ */
+import csharp
+
+Member getAUsedMember(Method m) {
+  exists(MemberAccess ma |
+    ma.getEnclosingCallable() = m |
+    result = ma.getTarget().getSourceDeclaration()
+  )
+  or
+  exists(Call c |
+    c.getEnclosingCallable() = m |
+    result = c.getTarget().getSourceDeclaration()
+  )
+}
+
+int dependencyCount(Method source, RefType target) {
+  result = strictcount(Member m | m = getAUsedMember(source) and m = target.getAMember())
+}
+
+predicate methodDependsOn(Method m, RefType target) {
+  exists(dependencyCount(m, target))
+}
+
+predicate dependsOn(RefType source, RefType target) {
+  methodDependsOn(source.getAMethod(), target)
+}
+
+int selfDependencyCount(Method source) {
+  result = sum(dependencyCount(source, source.getDeclaringType+()))
+}
+
+predicate dependsHighlyOn(Method source, RefType target, int selfCount, int depCount) {
+  depCount = dependencyCount(source, target) and
+  selfCount = selfDependencyCount(source) and
+  depCount > 2*selfCount and
+  depCount > 4
+}
+
+predicate query(Method m, RefType targetType, int selfCount, int depCount) {
+  exists (RefType sourceType | sourceType = m.getDeclaringType() |
+    dependsHighlyOn(m, targetType, selfCount, depCount) and
+    // Interfaces are depended upon by their very nature
+    not targetType instanceof Interface and
+    // Do not move extension methods
+    not m instanceof ExtensionMethod and
+    // Do not move up/down the class hierarchy
+    not (
+      sourceType.getABaseType*().getSourceDeclaration() = targetType or
+      targetType.getABaseType*().getSourceDeclaration() = sourceType
+    ) and
+    // Do not move between nested types
+    not (sourceType.getDeclaringType*() = targetType or targetType.getDeclaringType*() = sourceType) and
+    // Check that the target type already depends on every type used by the method
+    forall(RefType dependency |
+      methodDependsOn(m, dependency) |
+      dependsOn(targetType, dependency) or
+      targetType = dependency or
+      dependency.getNamespace().hasName("System")
+    )
+  )
+}
+
+from Method m, RefType other, int selfCount, int depCount
+where query(m, other, selfCount, depCount) and
+      // Don't include types that are used from many different places - we only highlight
+      // relatively local fixes that could reasonably be implemented.
+      count(Method yetAnotherMethod | query(yetAnotherMethod, other, _, _)) < 10
+select m, "Method " + m.getName() + " is too closely tied to $@: " + depCount +
+          " dependencies to it, but only " + selfCount + " dependencies to its own type.",
+          other, other.getName()

csharp/ql/src/Architecture/Refactoring Opportunities/FeatureEnvyBad.cs

@@ -0,0 +1,26 @@
+using System;
+
+class Bad
+{
+    class Item
+    {
+        public bool IsOutOfStock;
+        public decimal Price;
+        public decimal Tax;
+        public bool IsOnSale;
+        public decimal SaleDiscount;
+    }
+
+    class Basket
+    {
+        decimal GetTotalPrice(Item i)
+        {
+            if (i.IsOutOfStock)
+                throw new Exception("Item ${i} is out of stock.");
+            var price = i.Price + i.Tax;
+            if (i.IsOnSale)
+                price = price - i.SaleDiscount * price;
+            return price;
+        }
+    }
+}

csharp/ql/src/Architecture/Refactoring Opportunities/FeatureEnvyGood.cs

@@ -0,0 +1,23 @@
+using System;
+
+class Good
+{
+    class Item
+    {
+        public bool IsOutOfStock;
+        public decimal Price;
+        public decimal Tax;
+        public bool IsOnSale;
+        public decimal SaleDiscount;
+
+        decimal GetTotalPrice(Item i)
+        {
+            if (i.IsOutOfStock)
+                throw new Exception("Item ${i} is out of stock.");
+            var price = i.Price + i.Tax;
+            if (i.IsOnSale)
+                price = price - i.SaleDiscount * price;
+            return price;
+        }
+    }
+}

csharp/ql/src/Architecture/Refactoring Opportunities/InappropriateIntimacy.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p><em>Inappropriate intimacy</em> is an anti-pattern that describes a pair of otherwise unrelated classes
+that are too tightly coupled: each class uses a significant number of methods and fields of
+the other. This makes both classes difficult to maintain, change and understand. Inappropriate 
+intimacy is the same as the "feature envy" anti-pattern but in both directions: each class is 
+"envious" of some functionality or data defined in the other class.</p>
+
+</overview>
+<recommendation>
+
+<p>The solution might be as simple as moving some misplaced methods to their rightful place, or 
+perhaps some tangled bits of code need to be extracted to their own methods first before being moved.</p>
+
+<p>Sometimes the entangled parts (both fields and methods) indicate a
+missing object or level of abstraction. It might make sense to combine them into a new
+type that can be used in both classes. You may need to introduce delegation to
+hide some implementation details.</p>
+
+<p>It may be necessary to convert the bidirectional association into a
+unidirectional relationship, possibly by using dependency inversion.</p>
+
+<p>Modern IDEs provide refactoring support for this sort of issue, usually
+with the names "Move method", "Extract method" or "Extract class".</p>
+
+</recommendation>
+<references>
+
+<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>
+<li>W. C. Wake, <em>Refactoring Workbook</em>, pp. 95&ndash;96. Addison-Wesley Professional, 2004.</li>
+
+</references>
+</qhelp>

csharp/ql/src/Architecture/Refactoring Opportunities/InappropriateIntimacy.ql

@@ -0,0 +1,60 @@
+/**
+ * @name Inappropriate intimacy
+ * @description Two otherwise unrelated classes that share too much information about each other are
+ *              difficult to maintain, change and understand.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision high
+ * @id cs/coupled-types
+ * @tags maintainability
+ *       modularity
+ */
+import csharp
+
+predicate enclosingRefType(Variable v, RefType type) {
+  v.(Field).getDeclaringType() = type or
+  v.(LocalScopeVariable).getDeclaringType() = type
+}
+
+predicate remoteVarAccess(RefType source, RefType target, VariableAccess va) {
+  va.getEnclosingCallable().getDeclaringType() = source and
+  enclosingRefType(va.getTarget(), target) and
+  source != target
+}
+
+predicate remoteFunAccess(RefType source, RefType target, Call fc) {
+  fc.getEnclosingCallable().getDeclaringType() = source and
+  target = fc.getTarget().getDeclaringType()
+}
+
+predicate candidateTypePair(RefType source, RefType target) {
+  remoteVarAccess(source, target, _) or remoteFunAccess(source, target, _)
+}
+
+predicate variableDependencyCount(RefType source, RefType target, int res) {
+  candidateTypePair(source, target) and
+  res = count(VariableAccess va | remoteVarAccess(source, target, va))
+}
+
+predicate functionDependencyCount(RefType source, RefType target, int res) {
+  candidateTypePair(source, target) and
+  res = count(Call fc | remoteFunAccess(source, target, fc))
+}
+
+predicate dependencyCount(RefType source, RefType target, int res) {
+  exists(int varCount, int funCount |
+    variableDependencyCount(source, target, varCount) and
+    functionDependencyCount(source, target, funCount) and
+  res = varCount + funCount
+  and res > 15)
+}
+
+from RefType a, RefType b, int ca, int cb
+where dependencyCount(a, b, ca) and
+      dependencyCount(b, a, cb) and
+      ca > 15 and
+      cb > 15 and
+      ca >= cb and
+      a != b
+select a, "Type " + a.getName() + " is too closely tied to $@ (" + ca.toString() +
+          " dependencies one way and " + cb.toString() + " the other).", b, b.getName()

csharp/ql/src/Bad Practices/CallsUnmanagedCode.cs

@@ -0,0 +1,14 @@
+using System;
+using System.Windows.Forms;
+using System.Runtime.InteropServices;
+
+public partial class UnmanagedCodeExample : Form
+{
+    [DllImport("User32.dll")]
+    public static extern int MessageBox(int h, string m, string c, int type);
+
+    private void btnSayHello_Click(object sender, EventArgs e)
+    {
+        MessageBox(0, "Hello World", "Title", 0); // BAD
+    }
+}

csharp/ql/src/Bad Practices/CallsUnmanagedCode.qhelp

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Microsoft defines two broad categories for source code. Managed code compiles into bytecode and is then executed by a virtual 
+machine. Unmanaged code is compiled directly into machine code. All C# code is managed but it is possible to call external unmanaged 
+code. This rule finds calls to <code>extern</code> methods that are implemented by unmanaged code. Managed code has many advantages 
+over unmanaged code such as built in memory management performed by the virtual machine and the ability to run compiled programs 
+on a wider variety of architectures.</p>
+
+</overview>
+<recommendation>
+<p>Consider whether the calls could be replaced by calls to managed code instead.</p>
+
+</recommendation>
+<example>
+<p>This example shows a function that displays a message box when clicked. It is implemented with unmanaged code from 
+the User32.dll library.</p>
+<sample src="CallsUnmanagedCode.cs" />
+
+</example>
+<section title="Fixing by Using Managed Code">
+<p>This code example does the exact same thing except it uses managed code to do so.</p>
+<sample src="CallsUnmanagedCodeFix.cs" />
+
+</section>
+<references>
+
+  <li>MSDN, C# Reference <a href="http://msdn.microsoft.com/en-us/library/e59b22c5(v=vs.80).aspx">extern</a>.</li>
+  <li>Wikipedia, <a href="http://en.wikipedia.org/wiki/Managed_code">Managed code</a>.</li>
+
+</references>
+</qhelp>

csharp/ql/src/Bad Practices/CallsUnmanagedCode.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Calls to unmanaged code
+ * @description Finds calls to "extern" methods (which are implemented by unmanaged code).
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision high
+ * @id cs/call-to-unmanaged-code
+ * @tags reliability
+ *       maintainability
+ */
+
+import csharp
+
+from Class c, Method m, MethodCall call
+where m.isExtern()
+  and m.getDeclaringType() = c
+  and call.getTarget() = m
+select call, "Replace this call with a call to managed code if possible."

csharp/ql/src/Bad Practices/CallsUnmanagedCodeFix.cs

@@ -0,0 +1,10 @@
+using System;
+using System.Windows.Forms;
+
+public partial class ManagedCodeExample : Form
+{
+    private void btnSayHello_Click(object sender, EventArgs e)
+    {
+        MessageBox.Show("Hello World", "Title");
+    }
+}

csharp/ql/src/Bad Practices/CatchOfNullReferenceException.cs

@@ -0,0 +1,21 @@
+class CatchOfNullReferenceException
+{
+    public static Person findPerson(string name)
+    {
+        // ...
+    }
+
+    public static void Main(string[] args)
+    {
+        Console.WriteLine("Enter name of person:");
+        Person p = findPerson(Console.ReadLine());
+        try
+        {
+            Console.WriteLine("Person is {0:D} years old", p.getAge());
+        }
+        catch (NullReferenceException e)
+        {
+            Console.WriteLine("Person not found.");
+        }
+    }
+}