You can’t make an omelet…


Sorry about the lack of updates.  I haven’t been able to work on the Managed Language Service like I wanted to.  Instead I went on a great vacation and then have been spending most of my time doing a lot of in depth security work on the C# language service.  I’m not sure if I can talk about what we’ve done, but I’ll try to find out so I can write a post (or few) on that later.

 



 

So we’ve run into an interesting situation and we’re trying to figure out the best plan of attack for Beta2 (and for VS2005 in general).  Best to dive right into specifics.  In VS2003 we had a feature called “auto-complete on override” (AoO).  The way it worked was that if you typed:

 

override<space>

 

then we’d pop up a list of all methods, properties and indexers that were available to be overridden.  After choosing the item you wanted we’d then spit out code like:

 

public override int GetHashCode()

{

    return base.GetHashCode();

}

 

Now, we received a lot of requests from customers who used features like this and “implement interface” and “+= on events” that they wanted more control over the code that was injected into the source.  In order accommodate that, and also to enable some new features we wanted to provide, we developed the “expansions” feature that you can see in VS2005.   These “expansions” use special user-editable .snippet files that can contain pretty much anything.  They’ve got an XML schema that defines them and they feature specialized capabilities like the ability have programmatically replaceable fields. 

 

So now we started using these “snippets” extensively and replaced all the places where we spit out a method to now use the snippet instead.  To give you an idea of what that looks like, here’s the content of the MethodStub.snippet file:

 

<?xml version=1.0 encoding=utf-8?>

<CodeSnippets  xmlns=http://schemas.microsoft.com/VisualStudio/2005/CodeSnippet>

    <CodeSnippet Format=1.0.0>

        <Header>

            <Title>Method Stub – Body</Title>

            <Description>Snippet for generating method stub with a body</Description>

            <Author>Microsoft Corporation</Author>

            <SnippetTypes>

                <SnippetType>Refactoring</SnippetType>

            </SnippetTypes>

        </Header>

        <Snippet>

            <Declarations>

                <Literal Editable=true>

                    <ID>signature</ID>

                    <Default>signature</Default>

                </Literal>

                <Literal>

                    <ID>Exception</ID>

                    <Function>SimpleTypeName(global::System.Exception)</Function>

                </Literal>

            </Declarations>

            <Code Language=csharp>

                <![CDATA[$signature$

{

      $end$throw new $Exception$(“The method or operation is not implemented.”);

}]]>

            </Code>

        </Snippet>

    </CodeSnippet>

</CodeSnippets>

 

There’s a lot to digest, but really the most important components are the “literals” and the section inside the “code” tags.  Inside that you’ll see the $signature$” field.  This field will get replaced by the language service with the signature of the method being overridden.  Next is the $end$ field.  This designates where the cursor will be placed after the expansion is inserted.  Finally there’s the $Exception$” field.  This one is a little different.  If you look at:

 

                <Literal>

                    <ID>Exception</ID>

                    <Function>SimpleTypeName(global::System.Exception)</Function>

                </Literal>

 

you’ll see that the $Exception$ field witll get expanded out into a function call to SimpleTypeName(global::System.Exception).  The way that SimpleTypeName works is that it will use the “usings” in scope to pick the best possible name for the typename specified as its argument.  That way if you have a “using System;” then we’ll inject “throw new Exception(…)” whereas if you don’t have that “using” then we’ll inject “throw new System.Exception(…)”.

 

Remember, this file is user editable.  So by having this expansion, the user now has the ability to configure much of the code that is spit out.  They can now add comments.  Throw their own exceptions.  Have a default doc-comment included by default. Etc.

 

So what does this mean?  Well, between VS2003 and VS2005 we’ve changed the behavior of the AoO so that instead of getting:

 

public override int GetHashCode()

{

    return base.GetHashCode();

}

 

you’d get:

 

public override int GetHashCode()

{

    throw new Exception(…);

}

 

What’s worse is that there is no way to get back to the original behavior.  i.e. you could change the snippet file to not throw the exception, but there’s no way to configure it to call the base method correctly (as that would require know far more information than is exposed to the snippet engine).

 

We received quite a few complaints about this internally and from ProductFeedback: http://lab.msdn.microsoft.com/productfeedback/viewfeedback.aspx?feedbackid=f0799869-79f0-4aa8-91d9-b2250dbc6a91

 

At first we figured that we would postpone adding this functionality back in a later release.  We felt that while it was unfortunate that we were breaking old behavior it just wasn’t that big a deal.  After all, a user could just manually make the call to “base” themselves.  However, just recently we realized that our change was actually fairly detrimental to the user.  Why?  Well, for the correct functioning of overridable methods it’s often the case that the code must call the base method.  Forgetting to do this can lead to subtle bugs where the consistency of the base object is not longer valid.  By always injecting the base call we were putting that fact right in the face of the programmer and allowing them to remove it if they knew it was the right thing to do.

 

So we have a dilemma (albeit a minor one) right now.  We can continue to ship the way the product works today.  Do nothing and tell users “sorry, you can’t get the old behavior and you’ll have to manually fix up overrides.”   Or suck it up and add back this functionality in a way that works with the current snippet architecture.  i.e. we would most likely introduce a snippet that would have contents like this:

 

<![CDATA[$signature$

{

      $end$$CallBase$”

      throw new $Exception$(“The method or operation is not implemented.”);

}]]>

 

(note the addition of $CallBase$).  This field would then expand to the relevant call to the base method, property or indexer.

 

Why wouldn’t we do the latter?  Well, we’re focusing on security right now and absolutely getting VS2005 up to extremely high quality for the beta2 release.  This is the VS released under the “go live” license, meaning you can develop web sites and software that you are allowed to release/sell/whatever.  So it’s paramount that these tools be working very well.  Toward that end we are fixing the bugs we consider important and, in general, postponing anything else unless we feel that there is enough user need to warrant their inclusion.

 

So what we’re trying to find out is how important this feature is to you.   To you feel that it’s important enough to be re-added very late in the cycle (with the possibility of delaying beta2!), or should it be left out?

 

Let us know!  Tell us here (or on ProductFeedback) so that we’ll know what the right thing to do is!

 

Thanks!


Comments (40)

  1. AT says:

    As I’ve indicated at http://lab.msdn.microsoft.com/productfeedback/ViewFeedback.aspx?FeedbackID=FDBK19467

    I prefer TODO: task-list item over global search for NotImplementedException.

    Anyway – with or without throwing exception statement – TODO: comments will be pretty nice to have in all autogenerated code. I like the way VC6 wizards put thouse comments at all correct places – unfortunately – VS 2005 does not do this and you must guess that to do next with your code.

    In case if code generation rules will be hardcored somethere – you will be unable to fix this flaw without Microsoft assistance :-((

    BTW, I’ve discusses this with Cyrus – and proposed to add one more variable $ReturnBase$ thich will be equal to "return $CallBase$" for all return types except "void" one.

    This will allow developers to not add/delete return statements manualy to get compilable code for void/non-void method if throw statement will not be used.

    What do you think ?

  2. Luc Cluitmans says:

    I don’t think there is a ‘right’ way to do it. It depends on how the developer uses the ‘AoO’: to set up a skeleton for the class to be implemented later (an exception is best for this scenario), or to set up a skeleton for the method to be implemented immediately (the base call is best). I know I use it in both ways.

    Most of the time I used the ‘AoO’ in VS2003, I immediately replaced the call to the base method with a ‘throw NotImplementedException()’, so I was very happy to see VS2005 do that by default.

    View this from the scenario where you set up a skeleton for a derived class, but don’t have the time to actually implement that new class yet (for instance you want to reference that new class in the code you are actually working on). Putting in NotImplementedExceptions is much safer than calls to the base implementation: your code can compile correctly, but attempting to run it will immediately warn you that you forgot to implement something (and IIRC FxCop reminds you as well).

    On the other hand, having the call to the base implementation injected in your code saves a bit of typing later on and, as you already mentioned, reminds you that you may need to insert that call.

    Of course, adding both the exception and the call will result in warnings about unreachable code (and I normally treat warnings as errors). How about inserting both, but commenting out the call?

  3. AT says:

    P.S> BTW, I’ve notImpl expansion for this exception (no kidding !).

    If I decide to postpone some activities in middle of coding – I use it – "notImpl<TAB>" and done !!

    Here it is (notImpl.xml):

    <?xml version="1.0" encoding="utf-8" ?>

    <CodeSnippet Format="1.0.0">

    <Header>

    <Title>NotImplemented exception</Title>

    <Shortcut>notImpl</Shortcut>

    <Description>Expansion snippet for "throw new System.NotImplementedException();" statement</Description>

    <SnippetTypes>

    <SnippetType>Expansion</SnippetType>

    </SnippetTypes>

    </Header>

    <Snippet>

    <Declarations>

    </Declarations>

    <Code Language="csharp" Format="CData"><![CDATA[// TODO: XXX Fixme

    throw new System.NotImplementedException();

    ]]>

    </Code>

    </Snippet>

    </CodeSnippet>

  4. AT:

    I prefer TODO: task-list item over global search for NotImplementedException.

    You’re free to modify the snippet file to do exactly that.

    Just get rid of the exception and add the //TODO: whatever

    and you’ll be all set.

  5. Luc:

    Of course, adding both the exception and the call will result in warnings about unreachable code (and I normally treat warnings as errors). How about inserting both, but commenting out the call?

    You won’t get a warning. Take the above example and you’ll get:

    public override int GetHashCode()

    {

    ….base.GetHashCode();

    ….throw new Exception(…);

    }

    So there won’t be any complaints about unreachable code.

    This enforced the idea that you need to call the base method (but not necessarily return the result from it), and it also inject a good exception so that your unit tests will fail.

  6. AT: i like the snippet 🙂

    Maybe we’ll add that to VS2k5!

  7. Luc Cluitmans says:

    "You won’t get a warning."

    Ah, Ok.

    I missed the fact that the ‘return’ was missing. I’m using VS2003 as my everyday development environment, so my mind must have mentally inserted that ‘return’ that VS2003 would insert there…

  8. Lorenzo says:

    I think I’d like most to have Beta2 on time than a feature like this, that you can retrofit in a second moment. But I like very much the $ReturnBase$ equal to "return $CallBase$" for all return types except "void" one.

    I know that this could be a little more complicates, but how do you think of making the method more flexible? maybe making it a function call like SimpleTypeName(global::System.Exception)? I mean, you could have CallFunction($MethodName$) and write base.$callfunction$, or even this.$callfunction$ or proxytarge.$callfunction$. Or return FilterMethod($callfunction$).

    In this way you can also define $ReturnBase$ in terms of CallFunction.

  9. karl says:

    If you don’ thave time to implement $CodeBase$ why not have the default snippet atleast make a suggestion to the user that he or she might which to call the base class?

    //Todo: call base function if necessary

    Throw new Exception(…)

    Atleast ur putting the fact in the face of the programmer, somwhat…

    Karl

  10. Jim Argeropoulos says:

    I think that shipping as is would be a disaster in the making. It seems to me that there are a lot of devs who wouldn’t understand how to make it right. For the sake of the junior engineer out there in the world, I would say make the change.

  11. AndrewSeven says:

    AoO is a great feature.

    Expanding it to snippets is nice, but the important thing to me is what it does now.

    It gives a result specific to the override you choose. For those who always want the same thing (NotImplementedException) I suggest copy-paste.

    If you have the method’s signature,return type and parameters, why can’t you determine what a call would look like?

  12. JavaKid says:

    I like the idea of snippets and hopefully you guys are going to continue to progress this idea into something similar to Quick Code (http://www.dvxp.com/en/QuickCode.aspx). I’ve been longing for templated code generation that I have control over.

    Maybe you guys could also come up with an easy to use/quick template "snippet" editor for us 😉

    Anyways… Cyrus says:

    "What’s worse is that there is no way to get back to the original behavior. i.e. you could change the snippet file to not throw the exception, but there’s no way to configure it to call the base method correctly (as that would require know far more information than is exposed to the snippet engine)."

    This tells me that the snippet engine was most likely designed with a narrow scope, and wouldn’t give me the templating power I would like. Auto-Generated code that is not up to snuff and doesn’t allow me to fully control what gets generated, is a BIG deal me.

    I vote that you suck it up and make the changes.

    JavaKid.

  13. Juan Felipe MAchado says:

    IF it’s going to delay Beta 2, then DON’T DO IT, I can live with this "problem"… but I CANNOT live without beta2…!!!!

  14. James Geurts says:

    I would like to see snippets in Beta 2… Giving the user this flexibility with the product is a good thing…

  15. Kevin Dente says:

    Hmm – I think having the CallBase option is the right long-term answer. Is it something that can be added post-beta 2? If so, I say get beta 2 out the door (since it only affects code-generation and not the runtime, there’s no backward compatibility issues).

    But what I REALLY want is for AoO to display interface members as well as base class members. It drives me bonkers that there’s no easy way to add an implementation stub for an interface method. But I’m guessing I’ll have to wait for v.Next on that one. 😉

  16. JavaKid says:

    By the way… I didn’t see it as an option… but, if you ask me going back to VS 2003 style is better than the current.

    JavaKid.

  17. I think the existing behavior is very important. I’d prefer if it could be done in a way that didn’t delay Beta2 (either by squeezing it in before, or by putting it in between Beta2 and final) but if I had to choose "Beta2 sooner or this behavior" I’d say to delay Beta2 for it. I *do* think it’s that important.

    I have other changes I’d make over VS2003 behavior (I want overriding of properties to put them all on one line by default, ie public override int Foo {get {return base.Foo;}}) but the power of snippets would let me do that. Without $ReturnBase$ available the best I can do would be to put the cursor in the middle and then type it.

    BTW, how come $end$ is just a single cursor insertion point? Wouldn’t it be nicer to select the whole chunk of code that’s going to be replaced?

    $start$throw new $Exception$("The method or operation is not implemented.")$end$

    (assuming that $start$…$end$ demarks the text that will be selected after the operation). That way you could do the override and immediately start typing your method body, in the common case that you *didn’t* want to throw NotImplementedException and *do* want to immediately fill in the body.

  18. By the way how do you manage inserting properties with snippets? Do you have three different snippets for "getter only", "setter only" and "both"?

  19. Code Monkey says:

    I consider this a low priority. If the developer is overriding a method on a base class, they really should know whether or not they need call the base implementation. The developer should make that decision, not the IDE.

  20. Please make the fix. I agree with Jim Argeropoulos’s well-phrased comment.

  21. Karl: I like the suggestion about the TODO 🙂

  22. Andrew: "If you have the method’s signature,return type and parameters, why can’t you determine what a call would look like? "

    Yes, it’s not hard problem to do this, it’s just a question of whther or not it’s worth the cost.

  23. Jim: "I think that shipping as is would be a disaster in the making. It seems to me that there are a lot of devs who wouldn’t understand how to make it right. For the sake of the junior engineer out there in the world, I would say make the change. "

    Very good point. I’ll let everyone here know about that!

  24. JavaKid: "This tells me that the snippet engine was most likely designed with a narrow scope, and wouldn’t give me the templating power I would like. Auto-Generated code that is not up to snuff and doesn’t allow me to fully control what gets generated, is a BIG deal me. "

    Yes. The plan to have an extensible snippet engine that could do this was unable to happen due to time constraints.

    This started as a C# feature, but ended up moving to all languages. It was enormously difficult to do that and didn’t leave us enough time to add the flexibility that many would want.

    In the end, when compared to refactoring and E&C, this fell by the wayside.

  25. Stuart: Great suggestion on $start$ … $end$

    I’ll see if i can get taht done as well!

  26. Duncan says:

    "This started as a C# feature, but ended up moving to all languages."

    That is both great and a real pitty – great for other language users, a pity for us C#’ers that it reduced the dev time for neat features.

    Still, having played with VS2k5 now (I’m in love!) I’d say that having the ability to call a base class is of significant value to me.

    Should it delay beta2? Frankly I don’t care, but it *should* be in the final release…

    I also just noticeda bug in snippet expansion – probably it has already been reported – but if I use a snippet expansion for a ‘for loop’ and then try and repeat this inside that loop to generate a nested set of loops it fails.

    for(int i = 0; i < iMax; ++i)

    {

    for//Placing cursor here and trying to expand snippet does nothing.

    }

    cheers

  27. Keith Hill says:

    >> Should it delay beta2? Frankly I don’t care, but it *should* be in the final release…

    +1

  28. Pierre Arnaud says:

    Postpone BETA 2 and make sure you don’t break any editor behaviour! There are just too much such small discrepancies between VS.NET 2003 and VS.NET 2005. It will make switching to VS.NET 2005 a pain, which is a pity, because all the great features won’t counter-balance it, from my point of view.

  29. DrPizza says:

    It needs to work the way it used to work.

  30. DrPizza says:

    "BTW, I’ve discusses this with Cyrus – and proposed to add one more variable $ReturnBase$ thich will be equal to "return $CallBase$" for all return types except "void" one. "

    Or you could support void returns so that you can actually do things consistently….

  31. I’d like it put back, and I’d like it with the "return" – but mostly, for my money, I’d like to see the throwing of the exception appear above the base call, just to enforce that I look at *all* those overrides before calling them…

  32. Joku says:

    Much simpler fix..

    You suggested the "hack" $end$$CallBase$ ..

    If there is no time to make the hack properly, consider replacing

    $end$$CallBase$

    with

    // <link to local documentation which explains the different cases of handling the issue>

    And one of the cases in the doc would suggest: return base.GetHashCode(); and explain why etc..

    And the other docs need also more explaining why things are done like they are, currently one has to go look answers with google. Atleast the local docs could contain links to online commentary forum about the help subject, similar to the great PHP online help!

  33. AT says:

    Joku,

    Actualy – link to documentation will become very annoying after 2 day of usage.

    Even more – my "// TODO" comments idea in notImpl sniplet must be changed a little bit to become usefull.

    Instead of always writing the same TODO comment – you must set user cursor at location there he can write his own personal comments about reasons why this is not implemented yet.

    While this is possible to automaticaly generate some predefined comment – this comments text must be selected to make it possible delete/override with a single keystroke / letter.

  34. Joku says:

    AT,

    Perhaps, anyway that is the simplest way to add a note about the issue (atleast until it is fixed, if that is still possible before final) and still making the beta2 deadline.. Unless someone invents something better 🙂