Skip to content

C#: Improvements to cs/useless-upcast #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 5, 2018
Merged

C#: Improvements to cs/useless-upcast #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
386b89a
C#: Improvements to `cs/useless-upcast`
hvitved Aug 30, 2018
919203a
Address review comment
hvitved Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 91 additions & 32 deletions csharp/ql/src/Language Abuse/UselessUpcast.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,63 @@

import csharp

/** A callable that is not an extension method. */
private class NonExtensionMethod extends Callable {
NonExtensionMethod() {
not this instanceof ExtensionMethod
/** A static callable. */
class StaticCallable extends Callable {
StaticCallable() {
this.(Modifiable).isStatic()
}
}

/** Holds if non-extension callable `c` is a member of type `t` with name `name`. */
/** An instance callable, that is, a non-static callable. */
class InstanceCallable extends Callable {
InstanceCallable() {
not this instanceof StaticCallable
}
}

/** A call to a static callable. */
class StaticCall extends Call {
StaticCall() {
this.getTarget() instanceof StaticCallable and
not this = any(ExtensionMethodCall emc | not emc.isOrdinaryStaticCall())
}
}

/** Holds `t` has instance callable `c` as a member, with name `name`. */
pragma [noinline]
private predicate hasCallable(ValueOrRefType t, NonExtensionMethod c, string name) {
predicate hasInstanceCallable(ValueOrRefType t, InstanceCallable c, string name) {
t.hasMember(c) and
name = c.getName()
}

/** Holds if extension method `m` is a method on `t` with name `name`. */
pragma [noinline]
private predicate hasExtensionMethod(ValueOrRefType t, ExtensionMethod m, string name) {
predicate hasExtensionMethod(ValueOrRefType t, ExtensionMethod m, string name) {
t.isImplicitlyConvertibleTo(m.getExtendedType()) and
name = m.getName()
}

/** Holds `t` has static callable `c` as a member, with name `name`. */
pragma [noinline]
predicate hasStaticCallable(ValueOrRefType t, StaticCallable c, string name) {
t.hasMember(c) and
name = c.getName()
}

/** Gets the minimum number of arguments required to call `c`. */
int getMinimumArguments(Callable c) {
result = count(Parameter p |
p = c.getAParameter() and
not p.hasDefaultValue()
)
}

/** Gets the maximum number of arguments allowed to call `c`, if any. */
int getMaximumArguments(Callable c) {
not c.getAParameter().isParams() and
result = c.getNumberOfParameters()
}

/** An explicit upcast. */
class ExplicitUpcast extends ExplicitCast {
ValueOrRefType src;
Expand All @@ -56,47 +92,70 @@ class ExplicitUpcast extends ExplicitCast {
)
}

pragma [noinline]
private predicate isDisambiguatingNonExtensionMethod0(NonExtensionMethod target, ValueOrRefType t) {
exists(Call c |
/** Holds if this upcast may be used to disambiguate the target of an instance call. */
pragma [nomagic]
private predicate isDisambiguatingInstanceCall(InstanceCallable other, int args) {
exists(Call c, InstanceCallable target, ValueOrRefType t |
this.isArgument(c, target) |
t = c.(QualifiableExpr).getQualifier().getType() and
hasInstanceCallable(t, other, target.getName()) and
args = c.getNumberOfArguments() and
other != target
)
}

/** Holds if this upcast may be used to disambiguate the target of an extension method call. */
pragma [nomagic]
private predicate isDisambiguatingExtensionCall(ExtensionMethod other, int args) {
exists(ExtensionMethodCall c, ExtensionMethod target, ValueOrRefType t |
this.isArgument(c, target) |
not c.isOrdinaryStaticCall() and
t = target.getParameter(0).getType() and
hasExtensionMethod(t, other, target.getName()) and
args = c.getNumberOfArguments() and
other != target
)
}

pragma [nomagic]
private predicate isDisambiguatingStaticCall0(StaticCall c, StaticCallable target, ValueOrRefType t) {
this.isArgument(c, target) and
(
t = c.(QualifiableExpr).getQualifier().getType()
or
not exists(c.(QualifiableExpr).getQualifier()) and
not c.(QualifiableExpr).hasQualifier() and
t = target.getDeclaringType()
)
}

/**
* Holds if this upcast may be used to affect call resolution in a non-extension
* method call.
*/
private predicate isDisambiguatingNonExtensionMethodCall() {
exists(NonExtensionMethod target, NonExtensionMethod other, ValueOrRefType t |
this.isDisambiguatingNonExtensionMethod0(target, t) |
hasCallable(t, other, target.getName()) and
/** Holds if this upcast may be used to disambiguate the target of a static call. */
pragma [nomagic]
private predicate isDisambiguatingStaticCall(StaticCallable other, int args) {
exists(StaticCall c, StaticCallable target, ValueOrRefType t |
this.isDisambiguatingStaticCall0(c, target, t) |
hasStaticCallable(t, other, target.getName()) and
args = c.getNumberOfArguments() and
other != target
)
}

/**
* Holds if this upcast may be used to affect call resolution in an extension
* method call.
*/
private predicate isDisambiguatingExtensionMethodCall() {
exists(Call c, ExtensionMethod target, ExtensionMethod other, ValueOrRefType t |
this.isArgument(c, target) |
t = c.getArgument(0).getType() and
hasExtensionMethod(t, other, target.getName()) and
other != target
/** Holds if this upcast may be used to disambiguate the target of a call. */
private predicate isDisambiguatingCall() {
exists(Callable other, int args |
this.isDisambiguatingInstanceCall(other, args)
or
this.isDisambiguatingExtensionCall(other, args)
or
this.isDisambiguatingStaticCall(other, args)
|
args >= getMinimumArguments(other) and
not args > getMaximumArguments(other)
)
}

/** Holds if this is a useful upcast. */
predicate isUseful() {
this.isDisambiguatingNonExtensionMethodCall()
or
this.isDisambiguatingExtensionMethodCall()
this.isDisambiguatingCall()
or
this = any(Call c).(QualifiableExpr).getQualifier() and
dest instanceof Interface
Expand Down
4 changes: 2 additions & 2 deletions csharp/ql/src/semmle/code/csharp/exprs/Call.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ class ExtensionMethodCall extends MethodCall {
override Expr getArgument(int i) {
exists(int j |
result = this.getChildExpr(j) |
if isOrdinaryCall() then
if isOrdinaryStaticCall() then
(j = i and j >= 0)
else
(j = i - 1 and j >= -1)
Expand All @@ -381,7 +381,7 @@ class ExtensionMethodCall extends MethodCall {
* }
* ```
*/
private predicate isOrdinaryCall() {
predicate isOrdinaryStaticCall() {
not exists(this.getChildExpr(-1)) // `Ext(i)` case above
or
exists(this.getQualifier()) // `Extensions.Ext(i)` case above
Expand Down
60 changes: 58 additions & 2 deletions csharp/ql/test/query-tests/Language Abuse/UselessUpcast/UselessUpcast.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
using System;
using System.Collections.Generic;
using System.Linq;
using static StaticMethods;

interface I1 { int Foo(); }
interface I2 { float Foo(); }
interface I3 : I2 { }
interface I4 : I3 { }

class A : I1, I2
{
Expand Down Expand Up @@ -42,7 +45,7 @@ void Fn(A a) { }
void Fn(B a) { }
void Fn2(A a) { }

void Fn(string[] args)
void Test1(string[] args)
{
A a = new A();
B b = new B();
Expand Down Expand Up @@ -73,6 +76,59 @@ void Fn(string[] args)

var act = (Action) (() => { }); // GOOD

var objects = args.Select(s => (object)s); // GOOD
var objects = args.Select(arg => (object)arg); // GOOD

M1((A)b); // GOOD: disambiguate targets from `StaticMethods`
StaticMethods.M1((A)b); // GOOD: disambiguate targets from `StaticMethods`

void M2(A _) { }
M2((A)b); // BAD: local functions cannot be overloaded
}

static void M2(A _) { }

void Test2(B b)
{
// BAD: even though `StaticMethods` has an `M2`, only overloads in
// `Tests` are taken into account
M2((A)b);
}

class Nested
{
static void M2(B _) { }

static void Test(C c)
{
// BAD: even though `StaticMethods` and `Tests` have `M2`s, only
// overloads in `Nested` are taken into account
M2((B)c);
}
}
}

static class IExtensions
{
public static void M1(this I2 i) { }

public static void M1(this I3 i) =>
M1((I2)i); // GOOD

public static void M1(I4 i)
{
M1((I3)i); // GOOD
((I3)i).M1(); // GOOD
}

public static void M2(I2 i) { }

public static void M2(this I3 i) =>
M2((I2)i); // GOOD
}

static class StaticMethods
{
public static void M1(A _) { }
public static void M1(B _) { }
public static void M2(B _) { }
}
9 changes: 6 additions & 3 deletions csharp/ql/test/query-tests/Language Abuse/UselessUpcast/UselessUpcast.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
| UselessUpcast.cs:51:13:51:16 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:17:7:17:7 | B | B | UselessUpcast.cs:10:7:10:7 | A | A |
| UselessUpcast.cs:57:14:57:17 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:17:7:17:7 | B | B | UselessUpcast.cs:10:7:10:7 | A | A |
| UselessUpcast.cs:66:13:66:16 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:17:7:17:7 | B | B | UselessUpcast.cs:10:7:10:7 | A | A |
| UselessUpcast.cs:54:13:54:16 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:20:7:20:7 | B | B | UselessUpcast.cs:13:7:13:7 | A | A |
| UselessUpcast.cs:60:14:60:17 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:20:7:20:7 | B | B | UselessUpcast.cs:13:7:13:7 | A | A |
| UselessUpcast.cs:69:13:69:16 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:20:7:20:7 | B | B | UselessUpcast.cs:13:7:13:7 | A | A |
| UselessUpcast.cs:85:12:85:15 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:20:7:20:7 | B | B | UselessUpcast.cs:13:7:13:7 | A | A |
| UselessUpcast.cs:94:12:94:15 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:20:7:20:7 | B | B | UselessUpcast.cs:13:7:13:7 | A | A |
| UselessUpcast.cs:105:16:105:19 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:27:7:27:7 | C | C | UselessUpcast.cs:20:7:20:7 | B | B |
| UselessUpcastBad.cs:9:23:9:32 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcastBad.cs:4:11:4:13 | Sub | Sub | UselessUpcastBad.cs:3:11:3:15 | Super | Super |