16f6c0aecf
Inline adoc files when they are included exactly once. Also fix language tags because this inlining gives us better information on what language the code is written in.
136 lines
3.9 KiB
Plaintext
136 lines
3.9 KiB
Plaintext
== Why is this an issue?
|
|
|
|
Array covariance is the principle that if an implicit or explicit reference conversion exits from type ``++A++`` to ``++B++``, then the same conversion exists from the array type ``++A[]++`` to ``++B[]++``.
|
|
|
|
|
|
While this array conversion can be useful in readonly situations to pass instances of ``++A[]++`` where ``++B[]++`` is expected, it must be used with care, since assigning an instance of ``++B++`` into an array of ``++A++`` will cause an ``++ArrayTypeMismatchException++`` to be thrown at runtime.
|
|
|
|
|
|
=== Noncompliant code example
|
|
|
|
[source,csharp]
|
|
----
|
|
abstract class Fruit { }
|
|
class Apple : Fruit { }
|
|
class Orange : Fruit { }
|
|
|
|
class Program
|
|
{
|
|
static void Main(string[] args)
|
|
{
|
|
Fruit[] fruits = new Apple[1]; // Noncompliant - array covariance is used
|
|
FillWithOranges(fruits);
|
|
}
|
|
|
|
// Just looking at the code doesn't reveal anything suspicious
|
|
static void FillWithOranges(Fruit[] fruits)
|
|
{
|
|
for (int i = 0; i < fruits.Length; i++)
|
|
{
|
|
fruits[i] = new Orange(); // Will throw an ArrayTypeMismatchException
|
|
}
|
|
}
|
|
}
|
|
----
|
|
|
|
|
|
=== Compliant solution
|
|
|
|
[source,csharp]
|
|
----
|
|
abstract class Fruit { }
|
|
class Apple : Fruit { }
|
|
class Orange : Fruit { }
|
|
|
|
class Program
|
|
{
|
|
static void Main(string[] args)
|
|
{
|
|
Orange[] fruits = new Orange[1]; // Compliant
|
|
FillWithOranges(fruits);
|
|
}
|
|
|
|
static void FillWithOranges(Orange[] fruits)
|
|
{
|
|
for (int i = 0; i < fruits.Length; i++)
|
|
{
|
|
fruits[i] = new Orange();
|
|
}
|
|
}
|
|
}
|
|
----
|
|
|
|
|
|
ifdef::env-github,rspecator-view[]
|
|
|
|
'''
|
|
== Implementation Specification
|
|
(visible only on this page)
|
|
|
|
=== Message
|
|
|
|
Refactor the code to not rely on potentially unsafe array conversions.
|
|
|
|
|
|
'''
|
|
== Comments And Links
|
|
(visible only on this page)
|
|
|
|
=== relates to: S3062
|
|
|
|
=== on 11 May 2015, 14:33:24 Dinesh Bolkensteyn wrote:
|
|
I don't get this explanation - I think we need a real example where something fails
|
|
|
|
=== on 12 May 2015, 13:09:33 Ann Campbell wrote:
|
|
how 'bout now [~dinesh.bolkensteyn]?
|
|
|
|
=== on 12 May 2015, 16:20:25 Dinesh Bolkensteyn wrote:
|
|
Note that we probably also want an issue here:
|
|
|
|
|
|
----
|
|
FillWithOranges(new Apple[1]); // Noncompliant
|
|
void FillWithOranges(Fruit[] fruits) { fruits[0] = new Orange(); }
|
|
----
|
|
|
|
and that, in an ideal world, we would actually be able to flag the ``++FillWithOranges++`` method which should be refactored to take ``++Orange[]++`` as argument
|
|
|
|
|
|
I'm thinking that we can change this rule to something like: Don't assign into arrays of A when there exists subtypes of A.
|
|
|
|
|
|
So, on the trivial example:
|
|
|
|
----
|
|
object[] a = new string[42]; // This is actually OK - as long as we only plan to read from "a" then nothing bad will happen
|
|
a[0] = 42; // Noncompliant - here the rule determines that "a" is an array of "object", for which it is able to find many derived types ({{string}}, {{Int32}}, etc.)
|
|
----
|
|
|
|
WDYT [~ann.campbell.2]?
|
|
|
|
=== on 14 May 2015, 12:51:42 Ann Campbell wrote:
|
|
thanks [~dinesh.bolkensteyn]
|
|
|
|
=== on 15 Jun 2015, 12:48:51 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] this rule is missing the severity and scale mappings
|
|
|
|
=== on 15 Jun 2015, 13:13:21 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] can you create a separate rule, which is an actual bug detection rule based on this one?
|
|
|
|
|
|
This rule says that we should report on ``++Fruit[] fruits = new Apple[1];++``, however, that's perfectly fine, that line won't throw an exception. The problem comes when we try to put a non-``++Apple++`` derived fruit in the array. So we should mark this line: ``++fruits[i] = new Orange();++`` that there is a potential bug here.
|
|
|
|
|
|
However, currently we have no way of finding these real bugs. So that's why I would create a separate rule.
|
|
|
|
=== on 15 Jun 2015, 14:24:22 Ann Campbell wrote:
|
|
\[~tamas.vajk] SQALE was already there. I've added severity
|
|
|
|
=== on 15 Jun 2015, 14:47:57 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] True, and it should be disabled by default, if I'm right?
|
|
|
|
=== on 16 Jun 2015, 11:31:44 Ann Campbell wrote:
|
|
Correct [~tamas.vajk]
|
|
|
|
endif::env-github,rspecator-view[]
|