Evil code of the day

At a glance, this code doesn’t look particularly evil. What does it do though? Compile it with the C# 4.0b1 compiler and run it…

using System;

class Base
{
    public virtual void Foo(int x, int y)
    {
        Console.WriteLine("Base: x={0}, y={1}", x, y);
    }
}

class Derived : Base
{
    public override void Foo(int y, int x)
    {
        Console.WriteLine("Derived: x={0}, y={1}", x, y);
    }
}


class PureEvil
{
    static void Main()
    {
        Derived d = new Derived();
        Base b = d;
        
        b.Foo(x: 10, y: 20);
        d.Foo(x: 10, y: 20);
    }
}

The results are:

Derived: x=20, y=10
Derived: x=10, y=20

I’m very nearly tempted to leave it there and just see what the reactions are like, but I’ll at least give you a hint as to where to look – section 21.3 of the C# 4 spec explains why this gives odd results. It does make perfect sense, but it’s hideously evil.

I feel dirty.

Bonus questions

  • What happens if you rename the parameters in Derived.Foo to yy and xx?
  • (As suggested by Mehrdad) What happens if you call it with a dynamic value?

35 thoughts on “Evil code of the day”

  1. I haven’t read the spec yet but I think it does make perfect sense. I expected named parameters to be resolved statically. The question is: what happens with “dynamic dy = d; dy.Foo(x:10, y:20);”?

  2. @Mehrdad: It does make sense, although it could have gone the other way. (It could have used the original declaration for the parameter names.)

    I hadn’t even considered the dynamic version. I’d *expect* it to follow the rule of “imagine the dynamic value was statically declared to be the type that it turns out to be” – so the same as the d.Foo version, basically.

  3. When you use dynamic, it gives you x=20, y=10 for both, when you rename the parameters, it comes clear why this is like it is :)

    Derived d = new Derived();
    Base b = d;
    d.Foo(xx: 10, yy: 20);
    b.Foo(x: 10, y: 20);

    Otherwise, it won’t compile

  4. Nemerle also supports named parameters and came to the same conclusion about how this should work:

    using System

    class Base
    public virtual Foo(x : int, y : int) : void
    Console.WriteLine($”Base: x=$x, y=$y”)

    class Derived : Base
    public override Foo(y : int, x : int) : void
    Console.WriteLine($”Derived: x=$x, y=$y”)

    class PureEvil
    static Main() : void
    def d = Derived()
    def b = d : Base
    b.Foo(x=10, y=20)
    d.Foo(x=10, y=20)

  5. @Jon: Having read the spec, I think your expectation is true. The `dynamic` version would behave similarly. RE Resolving top to bottom: Yes it could have done that but IMO, that would be weird. I think when a class “overrides” a method, it *swaps* the whole thing with its own and the overridden version becomes its member, not the original member. By the way, I just wanted to note that this is not a new thing. .NET framework has always behaved like this: `typeof(Derived).GetMethod(“Foo”).GetParameters()[0].Name` will return `y`.

  6. Most likely, this would crop up in real life because of a mistake. The results, then, would be evil even with positional parameters:

    Base b = new Derived();
    b.Foo (10, 20); // x=20, y=10

  7. @Joe: Yup, it would certainly be just as problematic in that case. I think what makes this evil is that it *looks* like you’re being extra careful to get the arguments right…

  8. The question is: Is C# harbouring C++ Complexity Envy. It won’t have to for long if MS keeps adding detail-devils like this. Time will tell of course just how many development dollars adding this feature to C# saves the collective software industry, vs. how many it costs in being pure ‘bug factory’ material.

  9. @Tom, I’m not sure, named parameters can make your code a lot more readable (and Jon will probably agree that readability is key…)

    If you end up in such a situation, I think you deserved it because you were asking for it (yet the compiler could prevent renaming parameters when overridding)

    It is still a very good case study for those you prefer undestanding rather than knowing

  10. I was of course only serious when I made the C++ complexity quip. So far, and I’m hoping Jon can provide the ‘light build moment’, I’m finding that C# 4 just doesn’t “do it” for me in the same way C# 3 did. With luck I’m just being dense and missing all the big value statements. I was kind of hoping C# 4 would including more compiler time help for creating const / immutable structures – but I’m beginning to think that’s going to require at least some radical BCL work and most likely CLR changes too.

  11. @Tom: I think C# 4 will help a little bit in many situations, and a lot in a few situations. If you’re working with COM it’ll be a lot nicer. If you’re working in a C# 4-only shop, optional parameters can reduce the need for lots of overloads, and named arguments *can* add readability when used sensible. Covariance and contravariance just removes a little roadblock (in some situations). Dynamic typing is great for interop with COM or dynamic languages, and also makes reflection easier.

    It’s not *nearly* the same size of change as C# 2 or 3 though. And yes, I’d like to see some more const/immutable help too.

  12. I would suggest that they should add a compiler warning for any call site using named parameters where the parameter names involved have changed in the virtual hierarchy.

  13. I thought that the named parameters were only supposed to be ‘labels’ for the parameters, and not effect behavior at all???

  14. @ShuggyCoUk: That’s a very interesting idea – I’d actually suggest putting the warning on both the call site and the overriding code.

    @John: Parameter names are the names of variables, but in C# 4 you can specify named arguments, which can be in a different order to the declaration order.

  15. For extra giggles, I find myself wondering what happens if Base, Derived, and PureEvil all live in separate assemblies, and you fiddle with the parameter names in Base and/or Derived without recompiling the other assemblies…

  16. @James: At that point the dynamic version and the statically typed version would presumably diverge in behaviour.

    Let’s not do that :)

    For what it’s worth, I’m going to make it very clear in C# in Depth that named arguments make parameter names effectively part of the API for public members: change them at your peril!

  17. C# devs have historically seemed reluctant to add new warnings, the motivation apparently being that this may break users with “Warnings as Errors” on. For instance, even obviously unsafe usage of IDisposable classes still doesn’t cause a compiler warning (when it really should).

    So, if that’s really a prime motivation, then adding a warning at the call site may be lower risk to them – after all, you can’t break old code that way, since no old code has named parameters _anyhow_.

  18. Code analisys (FxCop) has a rule for this one: “Parameter names should match base declaration” (CA1725)

    This rule only checks types with a public access modifier though.

  19. There is nothing nasty about the language here. Only the derived code is nasty.

    You can’t use the parameter names for Base.Foo when directly calling Derived.Foo. If you have a method with parameters ‘foo’ and ‘bar’ it’s ridiculous to have to call it with ‘blah’ and ‘zing’ just because those happen to be the parameter names in some ancestor of the method. Just think of the confusion this would cause.

    You also can’t use the parameter names for Derived.Foo when calling Base.Foo, because there is no reason to suppose that Derived.Foo is any better for taking the names of than SomeOtherDerived.Foo. In fact, there might not even be any derived classes.

    So you always have to use the parameter names for the method you’re calling, regardless of it being virtual or an override or not. Which makes perfect sense.

    There is nothing else the language could logically do. It’s your code that is evil. And there’s a simple solution for that: don’t write this code (and who would!) :-)

    PS. I don’t actually see you saying that the problem is in C# and not your code per se … in fact I basically see you saying the opposite. But then, why call attention to the obvious fact that even if there’s nothing wrong with a language you can still do horrible things in it? I don’t understand.

  20. @Joren: Yes, it’s certainly my code which is evil here. I mainly wanted to draw attention to the fact that something which was previously harmless (renaming parameters) can now be very harmful indeed – and if you just reorder parameter names (instead of giving them completely different names) you can end up causing a bug quite easily.

    Likewise changing the declared type of a variable now not only affects which overloads are considered, but also which argument means what.

    I’m in no way trying to say that named arguments are a misfeature: just that they also introduce more opportunities for error. Use with care, and be aware that parameter names are now part of the API.

    Jon

  21. Anyone who has been concerned with interoperability has always considered parameter names to be a part of the public API, given that VB has had named parameters from the beginning. I believe there is a statement about that in the Framework Design Guidelines, although I can’t remember where at the moment.

  22. @David: Admittedly I hadn’t checked whether VB had named arguments. (Even C# has had named *parameters* forever :) Yes, I hope it’s mentioned in the FDG. I suspect it’s a guideline which hasn’t been treated particularly seriously in the past though – there’s nothing like the risk of it happening *within your own language* to highlight a potential problem to developers :)

    (Of course, it means that it’s yet more names to try to get right – and we know what a pain *they* are.)

  23. Rather unfortunate that the specifications basicallty breaks Liskov.
    If you for some reason renamed the parameters of one of your classes you can no longer make the substitution with out breaking your code.
    You might argue that the the developer is breaking the principle by using named parameters (and changing names) but I none the less finds it questionable when the language makes that possible

  24. This is something that really bothers me about the optional parameter spec in C# 4.0 – that and the fact that optional values are actually compiled into the callsite, rather than dynamically fetched at runtime from the assembly’s metadata and then cached.

    C# 4.0 has essentially made the parameter names a weak part of the signature of the method – as far as overload resolution is concerned for optional/named parameters.

    I have to say, that I believe that this should be a case where the compiler detects and prohibits the override in the derived class.

    In fact, the compiler should enforce that when the override keyword is applied to a method, the argument names must match the base method in both order and name.

    It may not be perfect, especially when parameter names can change in the base class over time, or conflict with other overloads – but it’s better than changing the overload resolution on the fly based on which version of the framework you compile to – or whether you call the method through a base or derived reference, or through a dynamic reference.

  25. Minimally, the compiler should provide a warning when arguments to a method are renamed in an overriding method, or reordered at a call site.

  26. Happily, this is an unlikely situation. Why would you want to overload a method with another method with the same name, the same number of patameters and their same types?
    Overloading is not for this coding evilness!!!

Comments are closed.