Skip to content

Commit 6c1098d

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #120 from hvitved/csharp/query/useless-upcast
Approved by calumgrant
2 parents 1bcae97 + 919203a commit 6c1098d

File tree

4 files changed

+157
-39
lines changed

4 files changed

+157
-39
lines changed

‎csharp/ql/src/Language Abuse/UselessUpcast.ql‎

Lines changed: 91 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -12,27 +12,63 @@
1212

1313
import csharp
1414

15-
/** A callable that is not an extension method. */
16-
private class NonExtensionMethod extends Callable {
17-
NonExtensionMethod() {
18-
not this instanceof ExtensionMethod
15+
/** A static callable. */
16+
class StaticCallable extends Callable {
17+
StaticCallable() {
18+
this.(Modifiable).isStatic()
1919
}
2020
}
2121

22-
/** Holds if non-extension callable `c` is a member of type `t` with name `name`. */
22+
/** An instance callable, that is, a non-static callable. */
23+
class InstanceCallable extends Callable {
24+
InstanceCallable() {
25+
not this instanceof StaticCallable
26+
}
27+
}
28+
29+
/** A call to a static callable. */
30+
class StaticCall extends Call {
31+
StaticCall() {
32+
this.getTarget() instanceof StaticCallable and
33+
not this = any(ExtensionMethodCall emc | not emc.isOrdinaryStaticCall())
34+
}
35+
}
36+
37+
/** Holds `t` has instance callable `c` as a member, with name `name`. */
2338
pragma [noinline]
24-
private predicate hasCallable(ValueOrRefType t, NonExtensionMethod c, string name) {
39+
predicate hasInstanceCallable(ValueOrRefType t, InstanceCallable c, string name) {
2540
t.hasMember(c) and
2641
name = c.getName()
2742
}
2843

2944
/** Holds if extension method `m` is a method on `t` with name `name`. */
3045
pragma [noinline]
31-
private predicate hasExtensionMethod(ValueOrRefType t, ExtensionMethod m, string name) {
46+
predicate hasExtensionMethod(ValueOrRefType t, ExtensionMethod m, string name) {
3247
t.isImplicitlyConvertibleTo(m.getExtendedType()) and
3348
name = m.getName()
3449
}
3550

51+
/** Holds `t` has static callable `c` as a member, with name `name`. */
52+
pragma [noinline]
53+
predicate hasStaticCallable(ValueOrRefType t, StaticCallable c, string name) {
54+
t.hasMember(c) and
55+
name = c.getName()
56+
}
57+
58+
/** Gets the minimum number of arguments required to call `c`. */
59+
int getMinimumArguments(Callable c) {
60+
result = count(Parameter p |
61+
p = c.getAParameter() and
62+
not p.hasDefaultValue()
63+
)
64+
}
65+
66+
/** Gets the maximum number of arguments allowed to call `c`, if any. */
67+
int getMaximumArguments(Callable c) {
68+
not c.getAParameter().isParams() and
69+
result = c.getNumberOfParameters()
70+
}
71+
3672
/** An explicit upcast. */
3773
class ExplicitUpcast extends ExplicitCast {
3874
ValueOrRefType src;
@@ -56,47 +92,70 @@ class ExplicitUpcast extends ExplicitCast {
5692
)
5793
}
5894

59-
pragma [noinline]
60-
private predicate isDisambiguatingNonExtensionMethod0(NonExtensionMethod target, ValueOrRefType t) {
61-
exists(Call c |
95+
/** Holds if this upcast may be used to disambiguate the target of an instance call. */
96+
pragma [nomagic]
97+
private predicate isDisambiguatingInstanceCall(InstanceCallable other, int args) {
98+
exists(Call c, InstanceCallable target, ValueOrRefType t |
6299
this.isArgument(c, target) |
100+
t = c.(QualifiableExpr).getQualifier().getType() and
101+
hasInstanceCallable(t, other, target.getName()) and
102+
args = c.getNumberOfArguments() and
103+
other != target
104+
)
105+
}
106+
107+
/** Holds if this upcast may be used to disambiguate the target of an extension method call. */
108+
pragma [nomagic]
109+
private predicate isDisambiguatingExtensionCall(ExtensionMethod other, int args) {
110+
exists(ExtensionMethodCall c, ExtensionMethod target, ValueOrRefType t |
111+
this.isArgument(c, target) |
112+
not c.isOrdinaryStaticCall() and
113+
t = target.getParameter(0).getType() and
114+
hasExtensionMethod(t, other, target.getName()) and
115+
args = c.getNumberOfArguments() and
116+
other != target
117+
)
118+
}
119+
120+
pragma [nomagic]
121+
private predicate isDisambiguatingStaticCall0(StaticCall c, StaticCallable target, ValueOrRefType t) {
122+
this.isArgument(c, target) and
123+
(
63124
t = c.(QualifiableExpr).getQualifier().getType()
64125
or
65-
not exists(c.(QualifiableExpr).getQualifier()) and
126+
not c.(QualifiableExpr).hasQualifier() and
66127
t = target.getDeclaringType()
67128
)
68129
}
69130

70-
/**
71-
* Holds if this upcast may be used to affect call resolution in a non-extension
72-
* method call.
73-
*/
74-
private predicate isDisambiguatingNonExtensionMethodCall() {
75-
exists(NonExtensionMethod target, NonExtensionMethod other, ValueOrRefType t |
76-
this.isDisambiguatingNonExtensionMethod0(target, t) |
77-
hasCallable(t, other, target.getName()) and
131+
/** Holds if this upcast may be used to disambiguate the target of a static call. */
132+
pragma [nomagic]
133+
private predicate isDisambiguatingStaticCall(StaticCallable other, int args) {
134+
exists(StaticCall c, StaticCallable target, ValueOrRefType t |
135+
this.isDisambiguatingStaticCall0(c, target, t) |
136+
hasStaticCallable(t, other, target.getName()) and
137+
args = c.getNumberOfArguments() and
78138
other != target
79139
)
80140
}
81141

82-
/**
83-
* Holds if this upcast may be used to affect call resolution in an extension
84-
* method call.
85-
*/
86-
private predicate isDisambiguatingExtensionMethodCall() {
87-
exists(Call c, ExtensionMethod target, ExtensionMethod other, ValueOrRefType t |
88-
this.isArgument(c, target) |
89-
t = c.getArgument(0).getType() and
90-
hasExtensionMethod(t, other, target.getName()) and
91-
other != target
142+
/** Holds if this upcast may be used to disambiguate the target of a call. */
143+
private predicate isDisambiguatingCall() {
144+
exists(Callable other, int args |
145+
this.isDisambiguatingInstanceCall(other, args)
146+
or
147+
this.isDisambiguatingExtensionCall(other, args)
148+
or
149+
this.isDisambiguatingStaticCall(other, args)
150+
|
151+
args >= getMinimumArguments(other) and
152+
not args > getMaximumArguments(other)
92153
)
93154
}
94155

95156
/** Holds if this is a useful upcast. */
96157
predicate isUseful() {
97-
this.isDisambiguatingNonExtensionMethodCall()
98-
or
99-
this.isDisambiguatingExtensionMethodCall()
158+
this.isDisambiguatingCall()
100159
or
101160
this = any(Call c).(QualifiableExpr).getQualifier() and
102161
dest instanceof Interface

‎csharp/ql/src/semmle/code/csharp/exprs/Call.qll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ class ExtensionMethodCall extends MethodCall {
357357
override Expr getArgument(int i) {
358358
exists(int j |
359359
result = this.getChildExpr(j) |
360-
if isOrdinaryCall() then
360+
if isOrdinaryStaticCall() then
361361
(j = i and j >= 0)
362362
else
363363
(j = i - 1 and j >= -1)
@@ -381,7 +381,7 @@ class ExtensionMethodCall extends MethodCall {
381381
* }
382382
* ```
383383
*/
384-
private predicate isOrdinaryCall() {
384+
predicate isOrdinaryStaticCall() {
385385
not exists(this.getChildExpr(-1)) // `Ext(i)` case above
386386
or
387387
exists(this.getQualifier()) // `Extensions.Ext(i)` case above

‎csharp/ql/test/query-tests/Language Abuse/UselessUpcast/UselessUpcast.cs‎

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,12 @@
33
using System;
44
using System.Collections.Generic;
55
using System.Linq;
6+
using static StaticMethods;
67

78
interface I1 { int Foo(); }
89
interface I2 { float Foo(); }
10+
interface I3 : I2 { }
11+
interface I4 : I3 { }
912

1013
class A : I1, I2
1114
{
@@ -42,7 +45,7 @@ void Fn(A a) { }
4245
void Fn(B a) { }
4346
void Fn2(A a) { }
4447

45-
void Fn(string[] args)
48+
void Test1(string[] args)
4649
{
4750
A a = new A();
4851
B b = new B();
@@ -73,6 +76,59 @@ void Fn(string[] args)
7376

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

76-
var objects = args.Select(s => (object)s); // GOOD
79+
var objects = args.Select(arg => (object)arg); // GOOD
80+
81+
M1((A)b); // GOOD: disambiguate targets from `StaticMethods`
82+
StaticMethods.M1((A)b); // GOOD: disambiguate targets from `StaticMethods`
83+
84+
void M2(A _) { }
85+
M2((A)b); // BAD: local functions cannot be overloaded
86+
}
87+
88+
static void M2(A _) { }
89+
90+
void Test2(B b)
91+
{
92+
// BAD: even though `StaticMethods` has an `M2`, only overloads in
93+
// `Tests` are taken into account
94+
M2((A)b);
7795
}
96+
97+
class Nested
98+
{
99+
static void M2(B _) { }
100+
101+
static void Test(C c)
102+
{
103+
// BAD: even though `StaticMethods` and `Tests` have `M2`s, only
104+
// overloads in `Nested` are taken into account
105+
M2((B)c);
106+
}
107+
}
108+
}
109+
110+
static class IExtensions
111+
{
112+
public static void M1(this I2 i) { }
113+
114+
public static void M1(this I3 i) =>
115+
M1((I2)i); // GOOD
116+
117+
public static void M1(I4 i)
118+
{
119+
M1((I3)i); // GOOD
120+
((I3)i).M1(); // GOOD
121+
}
122+
123+
public static void M2(I2 i) { }
124+
125+
public static void M2(this I3 i) =>
126+
M2((I2)i); // GOOD
127+
}
128+
129+
static class StaticMethods
130+
{
131+
public static void M1(A _) { }
132+
public static void M1(B _) { }
133+
public static void M2(B _) { }
78134
}

‎csharp/ql/test/query-tests/Language Abuse/UselessUpcast/UselessUpcast.expected‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
| 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 |
2-
| 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 |
3-
| 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 |
1+
| 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 |
2+
| 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 |
3+
| 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 |
4+
| 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 |
5+
| 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 |
6+
| 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 |
47
| 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 |

0 commit comments

Comments
 (0)