dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.97k stars 4.03k forks source link

"defer" statement #8115

Closed gafter closed 7 years ago

gafter commented 8 years ago

Swift recently added the defer statement. Would that make sense for C# and VB? in C# I imagine it would look something like

    {
        SomeType thing = Whatever...;
        defer {
            thing.Free();
        }
        // some code code using thing
    }

The idea is that the code in the defer block would be executed as the last action in the enclosing block. This would be the same as writing

    {
        SomeType thing = Whatever...;
        try
        {
            // some code code using thing
        }
        finally
        {
            thing.Free();
        }
    }
HaloFour commented 8 years ago

@gafter I'd have to see use cases that aren't already sufficiently met through try/finally or using. It doesn't even seem that Apple could provide a worthwhile example of why you'd use it.

alrz commented 8 years ago

ِDoesn't RAII (#181) address this?

{
  using Disposable thing = ...;

  // disposed
}

Because that kind of operations for a type without being a IDisposable isn't idiomatic C#.

However for this to work it should be able to borrow the object, or totally pass the ownership,

void F(Disposable d) { ... } // won't dispose as always
void G(using Disposable d) { ... } // will dispose at the end

{
  using Disposable d = ...;
  F(d); // d is borrowed
  // d is available
  G(d); // pass the ownership (move)
  // d is not available
}

This probably needs a sophisticated ownership system (then we might be able to use let instead).

Related: dotnet/csharplang#6611, dotnet/roslyn#161.

HaloFour commented 8 years ago

@gafter According to NSHipster the defer statement should be avoided for contexts other than ensuring resource cleanup.

I'd say that defer is Swift's answer to using and my opinion is that C# doesn't need a rebuttal or to copy a feature that can be as potentially misused/abused as this could be.

pawchen commented 8 years ago

This reminds me of the defer attribute for the script tag in HTML. It allows the browser to parallel the download of the script while continue parsing html. I don't see why this is useful in C#.

jamesqo commented 8 years ago

It's possible to just simulate this using Disposable.Create from Rx:

using (Disposable.Create(DoSomething))
{
    DoSomethingElse();
} // end of scope, DoSomething is called here

As such I don't think a defer statement is needed in C#.

gafter commented 8 years ago

The advantages of the defer statement over the alternatives are

jamesqo commented 8 years ago

@gafter Hm, I'm not sure. I was skeptical at first, but the arguments you presented are good.

If we're going to go down that route, we may want to consider adding an option for writing it inline like you can in Go, e.g.

var resource = AcquireResource();

defer resource.Dispose(); // no braces
paulomorgado commented 8 years ago

Still not sold on that!

HaloFour commented 8 years ago

@gafter

  1. If IDisposable is the pattern for creating resources which should be cleaned up then I don't see much validity to this argument. Endorsing an additional pattern, or no pattern at all, would only add confusion.
    1. Rx provides some fantastic helper methods which can convert any operation into a disposable. Why add language support to something that can be done easily through a library?
    2. A corollary to this might be to expand using to support convention-based disposing rather than strictly requiring IDisposable, such as supporting objects that declare an instance Close() method.
  2. Neither would dotnet/roslyn#181 or a variation thereof. My personal preferred syntax would be using var disposable = new SomeDisposableObject(); Same mechanism but the resource is attached to the defining scope, no additional indentation required.
  3. See point 1, this only matters if you're not using IDisposable, which you should be using anyway.

If the purpose of defer is to provide resource cleanup I'd rather find ways of improving upon using in a structured way rather than to toss in a completely new language construct with massive abuse potential which appears to have no real use cases aside resource cleanup. To toss some spaghetti against the wall, how about the following:

var resource = new MyNonDisposableResource() using resource.Close();
alrz commented 8 years ago

@gafter I don't think that real problem with using statement is additional indentions nor that you have to implement an interface — this is not a bad thing at all, so there is a contract for types that need cleanup. I think the actual problem here is that you might forget to dispose disposable types, defer statement doesn't help with this at all, it rather encourages you to not implementat IDisposable inetrface and explicitly call methods like Close or Free which in presence of IDisposable seem like code smells.

HaloFour commented 8 years ago

@alrz Using let as you describe seems like it would be in conflict with dotnet/roslyn#6400 which is proposing that let have a completely different function. Blending the two wouldn't make much sense. And wasn't move/ownership semantics one of the tricky issues in considering dotnet/roslyn#161? I'd worry that it would be too easy to lose track of true ownership, particularly if you ever call into an assembly compiled in any other language (or a previous version of C#).

ufcpp commented 8 years ago

PowerShell has the trap statement, which has similar usage to the defer statement. The trap statement is little confusing and I prefer the try-catch statement.

alrz commented 8 years ago

@HaloFour If we ever wanted it to be the default behavior but yes, then other languages or even earlier versions of C# might be considered as unsafe. But as I said in my first comment here, RAII (using using) does need a lightweight version of ownership, otherwise if you pass the object to another method then you're screwed. dotnet/roslyn#161 and dotnet/csharplang#6611 are about destructible types and move keyword, but wi th an ownership system it would work for any type and there would be no need for destructible or move keywords.

HaloFour commented 8 years ago

@alrz Changing the keywords doesn't change how difficult it may or may not be. And if developers still need to opt-into it then you still haven't solved any of the problems. At least with destructible types it was a property of the type and the consumer didn't have to opt-into anything.

RAII doesn't imply anything about ownership. The convention as it was developed in C++ relies on the lifetime of the object being tied to the stack of the method within which it was created. dotnet/roslyn#181 is a much closer implementation of that than anything involving ownership, reference counting or move semantics.

alrz commented 8 years ago

@HaloFour This is more related to dotnet/csharplang#6611 and dotnet/roslyn#161 and somehow covering dotnet/roslyn#181 but probably out of scope of this proposal, so I just give it up. :smile:

MgSam commented 8 years ago

If the the use case for defer is resource cleanup, then it would seem to be an anti-pattern to me to not require that the thing implement IDisposable. Implementing IDisposable allows tooling to warn you if you create a disposable as a local variable and then never dispose of it. If you make it accepted for resources to not require IDisposable, you lose this benefit.

I think method scoped using statements would be just as effective while being much more idiomatic.

gafter commented 8 years ago

I see lots and lots of code like this in Roslyn:

            var oldMethodOrLambda = this.currentMethodOrLambda;
            this.currentMethodOrLambda = node.Symbol;
            ... // process the current lambda
            this.currentMethodOrLambda = oldMethodOrLambda;

I would hate to use IDisposable for this. defer would be perfect, as it keeps the "begin" and related "end" code together.

            var oldMethodOrLambda = this.currentMethodOrLambda;
            this.currentMethodOrLambda = node.Symbol;
            defer this.currentMethodOrLambda = oldMethodOrLambda;
            ... // process the current lambda
leppie commented 8 years ago

@gafter: That is really dynamic binding. It would be nice to do syntax sugar for what I did in code here http://www.codeproject.com/Articles/153896/Dynamic-Binding-in-C

GeirGrusom commented 8 years ago

I would like to note that D has had the scope guard statement for some time, but it allows you to do something when scope fails, or succeeds as well.

Anyway wouldn't this be more useful as a using extension? Allowing a type to determine what happens when the scope fails, succeeds and finishes (which is what using allows) could be useful.

public interface IScopeGuard
{
  void Failed(Exception  ex);
  void Success();
}

public class Transaction : IScopeGuard, IDisposable
{
  private readonly int id;
  public Transaction()
  {
    id = BeginTransaction();
    Console.WriteLine($"Started transaction {id}");
  }
  void Failed(Exception ex)
  {
    FailTransaction(id);
  }
  void Success()
  {
    CommitTransaction(id);
  }
  void Dispose()
  {
    Console.WriteLine($"Transaction {id} completed.");
  }
}

using(new Transaction())
{
   DoTransactionalStuff();
} 

edit: it occurs to me that this is a digression. Sorry about that.

ghord commented 8 years ago

There is great talk about implementing this feature in C++ by Andrei Alexandrescu. Some string arguemnts in favor of this there.

alrz commented 8 years ago

@gafter That use case is really similar to what Block function does in Mathematica.

block(this.currentMethodOrLambda = node.Symbol) 
{
}

Then there is no need for temporary variable and explicit defer to assign it back. If anything, I would prefer this (perhaps with another keyword) over defer because it really encourages bad design (for resource cleanup).

As an alternative I'd suggest scoped assignments,

{
  scoped this.currentMethodOrLambda = node.Symbol;
}

But it seems that this is useful only for this specific use case.

MgSam commented 8 years ago

@gafter But that only works if there is no other logic you want to come after the restoration of this.currentMethodOrLambda. If there is, then you run the risk of the developer adding that into the defer block too, making your method into a confusing mess where the code that runs at the end is written at the top of the method body rather than the bottom. The last thing you want when you're reading code is to have to remember that there might be extra logic that takes place at the end of the method that was written out somewhere else entirely.

HaloFour commented 8 years ago

@MgSam Agreed. For the most simple cases that may be suitable, otherwise you end up having to defer multiple operations and be extremely conscious as to the fact that they'll execute backwards lexically. This task is pretty easily accomplished through try/finally which, while more verbose, is significantly more structured and easier to follow.

var oldMethodOrLambda = this.currentMethodOrLambda;
try
{
    this.currentMethodOrLambda = node.Symbol;
    // ... process the current lambda
}
finally {
    this.currentMethodOrLambda = oldMethodOrLambda;
}
paulomorgado commented 8 years ago

see lots and lots of code like this in Roslyn:

        var oldMethodOrLambda = this.currentMethodOrLambda;
        this.currentMethodOrLambda = node.Symbol;
        ... // process the current lambda
        this.currentMethodOrLambda = oldMethodOrLambda;

I would hate to use IDisposable for this. defer would be perfect, as it keeps the "begin" and related "end" code together.

        var oldMethodOrLambda = this.currentMethodOrLambda;
        this.currentMethodOrLambda = node.Symbol;
        defer this.currentMethodOrLambda = oldMethodOrLambda;
        ... // process the current lambda

I'm sorry @gafter, but I'm still failing to see the value of this proposal.

pawchen commented 8 years ago

What is the order of multiple defer blocks, and what if exceptions throw in the middle?

HaloFour commented 8 years ago

@diryboy If it were to be implemented as it is in Swift, the following C# code:

static void Main() {
    Console.WriteLine("Hello");
    defer {
        Console.WriteLine("Foo");
    }
    defer {
        Console.WriteLine("Bar");
    }
    defer {
        Console.WriteLine("Baz");
    }
    Console.WriteLine("World");
}

would produce the following output:

Hello
World
Baz
Bar
Foo

As far as I can tell it's illegal to throw from within the defer block.

alrz commented 8 years ago

It's like a finally block in the middle of nowhere so why not reusing finally instead of defer. But when I think how messy it can get I tremble.

pawchen commented 8 years ago

@HaloFour Any method call within defer block could throw. Any code could throw before defer blocks get a chance to execute. The example code is a good example of writing code backwards without any good reason.

To address the org -> current -> org property change, if to introduce language support, IMO it's better to introduce some feature that could express that one off. For example

stash this.currentMethodOrLambda = node.Symbol;
// compiler generate code to revert this.currentMethodOrLambda to it's original value on exit scope
// should exceptions happen before exit scope, the value is not reverted back

Edit: OK, duplicated with @alrz

HaloFour commented 8 years ago

@diryboy

Swift is a lot stricter about exceptions than C# and doesn't permit calling functions that declare that they throw exceptions from within a defer block. As far as I can tell if a function were to fail unexpectedly, such as attempting to forcibly deference a nil, that's a fatal situation and the entire program stops. If the method throws outside of the defer blocks then those blocks are executed as expected. Not unlike wrapping the entire method body in a bunch of try/finally clauses, exception for being in reverse order. I'm certainly not an expert in Swift, though.

I'm not convinced that a temporary swap is so common as to require its own keyword and semantics. It's pretty easy to accomplish now with try/finally or one of the helpers from Rx. I'd even argue that such logic depending on changing some shared state to temporarily change how the program behaves is itself a code smell and probably shouldn't be encouraged.

pawchen commented 8 years ago

@HaloFour

Thanks for sharing.

Me neither.

alrz commented 8 years ago

@HaloFour

changing some shared state to temporarily change how the program behaves is itself a code smell

Exactly. They probably chose this pattern to avoid repeating a parameter in every function call and ended up with this.

HaloFour commented 8 years ago

@paulomorgado Ah yeah, copypasta fail. I'll update.

migueldeicaza commented 8 years ago

I love the idea of the defer statement in C#.

It provides an expressive way to state the programmer's desired intention. While try/finally may accomplish the same work, it pushes the intention far away, and in nested cases becomes inconvenient to navigate.

GeirGrusom commented 8 years ago

I think defer is a very vague name though.

lachbaer commented 8 years ago

I wouldn't recommend 'mixing' the execution order of the code with defer for the same reason that return statements should not be present in the middle of a any method or elsewhere where they could easily be overssen.

For the use-case @gafter mentioned I think it is besser to leave the assignment this.currentMethodOrLambda = oldMethodOrLambda; directly before the exit points in the method. This reminds the reader at the exit point that something is reverted to its original state.

temporaryfile commented 8 years ago

You can mock this up with an IDisposable stack of delegates. Instead of "defer", push a lambda closure to the stack. Inside Dispose(), run them in reverse order. That's a lot of overhead for reaching a finally block that's usually less than a Page Down away. It feels pointlessly clever.

The problem with a struct implementing IDisposable to solve this pattern everywhere is that Dispose() will cause it to be boxed. We need a pattern for the using-statement, not an interface, using method names the compiler knows about, exactly like when I build awaiter/awaitable objects.

svick commented 8 years ago

@playsomethingsaxman

The problem with a struct implementing IDisposable to solve this pattern everywhere is that Dispose() will cause it to be boxed.

No, it won't. When you you use using on a struct, the Dispose() method is called using constrained. callvirt, which does not box the value.

temporaryfile commented 8 years ago

No, it won't. When you you use using on a struct, the Dispose() method is called using constrained. callvirt, which does not box the value.

Niiice, learn something new every day. Not sure what we really need then.

gafter commented 8 years ago

@playsomethingsaxman

Not sure what we really need then.

I suggest you rewrite the original post using your suggested mechanism. Don't modify existing types; if you need to declare a new struct, include it in your solution. Is it easier to read?

temporaryfile commented 8 years ago

I suggest you rewrite the original post using your suggested mechanism. Don't modify existing types; if you need to declare a new struct, include it in your solution. Is it easier to read?

I can't modify existing types but I can create extension methods? Game on. But at some point a .NET-facing library has to do its part and accommodate basic .NET patterns.

To simulate what "defer" really is...

using (var deferrals = new DeferralStack())
{
    SomeType thing = Whatever;
    deferrals.Defer(() => thing.Free());

    SomeType thing2 = Whatever;
    deferrals.Defer(() => thing2.Free());
}

To me that would be a nightmare to step through and debug, especially with GULP locks. Here's another question: what happens to the remaining deferrals if the first one throws and all of them are guaranteed to run? Do we assemble an AggregateException?

gafter commented 8 years ago

To me that would be a nightmare to step through and debug

Yes, the alternative is pretty bad. Yet another reason to support the original proposal.

whoisj commented 8 years ago

The defer statement is fairly novel. It's basically encouraging developers to perform RAII like operations (d'tors, etc) in immediately following code blocks instead of using scope characters (usually {). However, it also keeps state as to what bits of code get executed based on which defer statements are actually reached at run time. No more of the C clean up of if (foo) free(foo); if (bar) free(bar); or the equivalent of whatever Swift offers.

Given that it is only a compiler trick and should require no run-time support, I think this is a fantastic addition to nearly any language. Kudos to the devs at Swift for inventing it, and to the devs here on Roslyn for considering it.

Only question: what happens if a defer statement "throws"?

whoisj commented 8 years ago

Another observation/question: does it make more sense for the sequence of defer statements to be stack or queue oriented? For example, with the following snip-it of code what should the expected outcome be?

{
    Console.WriteLine("A");
    defer { Console.WriteLine("One"); }
    Console.WriteLine("B");
    defer { Console.WriteLine("Two"); }
    Console.WriteLine("C");
    defer { Console.WriteLine("Three"); }
}

Should it be:

A
B
C
Three
Two
One

Or

A
B
C
One
Two
Three

As I understand it, with Swift's implementation the former would be the case, however the later makes more sense to me as that was the order it was declared. Thoughts?

It's like a finally block in the middle of nowhere so why not reusing finally instead of defer. But when I think how messy it can get I tremble.

I agree, but finally already has meaning and overloading the statement could cause all kinds of real errors to escape the compiler checks.

HaloFour commented 8 years ago

@whoisj Since the prototypical use case seems to be resource cleanup I think that reverse order makes more sense. The compiler can't know if cleaning up the first resource might depend on cleaning up subsequent resources. Also, every language I've found with this facility seems to invoke in reverse order, including D, Swift, Go, Rust (via defer! macro) and C++ (via RAII, destructors are always called in reverse order).

svick commented 8 years ago

@whoisj Just like with using and RAII, I think that stack is the only real option. Consider that the non-deferred statements could have dependencies on each other. For example, this code that uses using:

using (var fileStream = File.OpenWrite(@"C:\users\svick\desktop\test.txt"))
using (var gZipStream = new GZipStream(fileStream, CompressionMode.Compress))
using (var writer = new StreamWriter(gZipStream))
{
    writer.WriteLine("Hello World");
}

The natural way to rewrite it with defer would be:

var fileStream = File.OpenWrite(@"C:\users\svick\desktop\test.txt");
defer { fileStream.Close(); }
var gZipStream = new GZipStream(fileStream, CompressionMode.Compress);
defer { gZipStream.Close(); }
var writer = new StreamWriter(gZipStream);
defer { writer.Close(); }
writer.WriteLine("Hello World");

This would only work correctly if defers were executed in reverse order.

whoisj commented 8 years ago

@svick good point! Explains the decision Swift made.

The using statement is fine, but it does require IDisposable which is less than optimal.

As for RAII, I'd love to see support for it. Sadly, I believe that would require more than compiler ticks to implement correctly and CLR/IL changes are pretty much out of scope.

aL3891 commented 8 years ago

To me this sounds like a feature that would add alot of complexity and opportunity for misuse for not a lot of benefit. I assume defer would only work inside a perticular method, but resource cleanup is commonly done at a later time, say when the object itself is cleaned up. I do see the charm, but i just don't think it adds enough to the language to warrant a new keyword.

Also, what is the closure situation? Consider

for(var i = 0; i<10; i++) {
    defer Something(i);
}

Would Foobe called 10 times with 10? or 10,9,8..? i'd imagine the first, unless a local variable is declared, same as if a Func was used?

If defer statements can contain anything, they can also change state i assume, so this would be legal:

var thing = GetThing();
if(Test())
    defer thing.baz = 30
defer thing = GetOtherThing();
defer thing = null;

A bit contrived for sure, but my point is that somewhere half a page up, in some branch something is defered and that might be hard to keep track of. The execution order will jump all over the place (and i guess thats the point) but it will be hard to get an overview, i sort of have to keep stack in my head with all the deferals :)

alrz commented 8 years ago

We might be able to do this with dotnet/csharplang#2478 and code generators, marking the node we want to defer with a attribute and regenerate the syntax tree,

void Method() {
  var something = new Something();
  [[Defer]] something.Free();
  // blah
}

replace void Method() {
  var something = new Something();
  try { 
    // blah
  } finally {
    something.Free();
  }
}

However, I'm not aware if generator API allows to ReplaceNode or that kind of stuff.

gafter commented 8 years ago

@aL3891

Also, what is the closure situation? Consider

for(var i = 0; i<10; i++) {
    defer Something(i);
}

Since defer defers until the }, and the } immediately follows, that has the same behavior as

for(var i = 0; i<10; i++) {
    Something(i);
}

Defer would not be an embedded-statement, so you could not use it as the controlled statement of an if. So no, there will be no defers hidden in branches.

HaloFour commented 8 years ago

@gafter

I think more to the point, what would the behavior of the following be?

void Foo() {
    int i = 1;
    defer Console.WriteLine(i);
    i = 2;
}