dotnet / csharplang

The official repo for the design of the C# programming language
11.02k stars 1.01k forks source link

Champion "defer statement" #1398

Open gafter opened 6 years ago

gafter commented 6 years ago

See #513

This is to be considered alongside #1174 (though we could elect to do one without the other)

alrz commented 6 years ago

This is to be considered alongside #1174 (though we could elect to do one without the other)

I guess this has been brought up before but just to mention, #1174 + "convention-based Dispose" could address the use cases for a "defer statement" (edit: except that it's not anything like resource cleanup, which I believe, such language construction would encourage).

HaloFour commented 6 years ago

@alrz That would be #93

gafter commented 6 years ago

@alrz #1174 + "convention-based Dispose" doesn't help much as a replacement for defer, as it requires creating an object. defer would not create any runtime garbage.

scalablecory commented 6 years ago

I feel like, outside of calling Dispose (for which there are better proposals), this would be so rarely used that I can't really make a case for its inclusion in the language.

I also think it's very easy to misuse. Not always the case, but generally if you have more than one cleanup block in your code it's a code smell.

1174 seems much easier for users to use correctly from the standpoint of separation of concerns.

alrz commented 6 years ago

One valid use case that I can think of, is to save the current state in traversal of some tree structure and reverting back that state once all children are visited.

Still, since this feature is really easy to abuse, I think as long as we keep it undocumented we're fine.

gafter commented 6 years ago

In the spirit of designing the language for myself, there are hundreds of places in the compiler where this would be a simplification of existing code.

yaakov-h commented 6 years ago

@gafter wouldn't "convention-based Dispose" plus a struct similarly not create an object?

gafter commented 6 years ago

@yaakov-h Sure you can do that. And you're going to store a delegate in the struct I presume so you can pass a lambda with your arbitrary code in it?

yaakov-h commented 6 years ago

@gafter #302? 😄

gafter commented 6 years ago

Static delegates cannot capture anything

tannergooding commented 6 years ago

@gafter, static delegates by themselves don't capture anything (as proposed).

This does not, however, mean the compiler could not create some struct which allowed state to be carried and passed to a static delegate when invoked (such as to support some other feature).

tannergooding commented 6 years ago

(a user could of course do the same with static delegates)

ygc369 commented 6 years ago

@gafter I like this idea. It could help people avoid to forget must-be-done things. But I wonder whether this would work when "goto" exists. For example:

{
        SomeType thing = Whatever...;
        defer {
            thing.Free();
        }
        if (SomeCondition) goto outside; //would "thing" be freed before goto outside?
        //some code 
}
outside:
    //some code
svick commented 6 years ago

@ygc369 Doesn't the same question exist with finally today? And the answer is that the finally block is executed when you use goto.

Opiumtm commented 6 years ago

It's still unclear how it would be implemented? Every defer block could be translated by compiler into separate try..finally, or all deffered blocks could be in the single finally? If all would be in separate finally blocks then OK as it would help to avoid multiple nested try...finally in some cases. Then it's unclear in which exact order deffered blocks could be executed.

svick commented 6 years ago

@gafter

In the spirit of designing the language for myself, there are hundreds of places in the compiler where this would be a simplification of existing code.

I wonder if it's more common need in the compiler than in other code. And since compiler writers are not really a target demographic of C#, I think their needs are not that important. "Designing for myself" can work, but I think you need to make sure you're not designing just for yourself.

Static delegates cannot capture anything

Sure, but the struct that's being disposed can.

So I think you could do something like:

using scoped Defer(thing, static t => t.Free());

Or even (assuming using was changed to support ref structs https://github.com/dotnet/csharplang/issues/93):

using scoped Defer(ref thing, static (ref SomeType t) => t.Free());

It's much more verbose than real defer, but maybe it will turn out that it's good enough? If not, defer could be added in a later version.

HaloFour commented 6 years ago

@gafter

I'd love to hear more about these use cases you envision for defer. I know that you've mentioned temporarily swapping field values and reverting at the end of a scope.

I'm not terribly concerned about how the @gafter's of the world might use this feature. It's everyone else.

Also, assuming that defer is scoped to the current block, I'm putting $500 on an immediate demand for the ability to expand to an outer block or the scope of the method. I bet people will find it frustrating trying to juggle scopes to get their deferred actions to execute when they actually want it to.

gafter commented 6 years ago

@HaloFour I assume you're not asking me.

HaloFour commented 6 years ago

@gafter

I assume you're not asking me.

Yes, the question is in response to your statement which I should've quoted:

In the spirit of designing the language for myself, there are hundreds of places in the compiler where this would be a simplification of existing code.

I'm not trying to challenge your statement, I just want to understand the value-add where existing syntax doesn't suffice for one reason or another.

Thaina commented 6 years ago

@gafter

"convention-based Dispose" doesn't help much as a replacement for defer, as it requires creating an object. defer would not create any runtime garbage.

I am support your whole idea and mechanism. But I still thinking that we should not need defer and just use using as of implicit using and solve this problem by compiler and new syntax

Because mechanism-wise, this is actually the using and dispose

As I was propose #249 that we should able to create anonymous local function. In this case we should be able to create local function for mocking the Dispose

As for the simplest case

// Just stranspile in the same way as defer
var thing = Whatever...;
using ~> { // syntax for defer
    thing.Free();
}

And for inline case

// Just stranspile in the same way as defer
using var thing = Whatever... ~> { // syntax for defer
    thing.Free();
};

Also I was once propose https://github.com/dotnet/roslyn/issues/16958 about return using. It would be good if this syntax would be generalized to be disposable object that could be returned

TheUnlocked commented 6 years ago

Just going with @Thaina's syntax,

using var thing = new ThingObj(arg, anotherArg) ~> thing.Free();

could be a possible one-liner. Although in this case, ThingObj should really be properly disposable.

gafter commented 6 years ago

As I was propose #249 that we should able to create anonymous local function. In this case we should be able to create local function for mocking the Dispose

The point is to avoid creating unnecessary garbage. Not even a delegate. This should be as efficient as other local control-flow.

scalablecory commented 6 years ago

If we have implicitly-scoped using, it seems to me that the rest of defer's need could be accomplished with a set of tricky Roslyn optimizations. This'd likely have a double-whammy effect of making the simplest (and probably most common) use of local functions more efficient.

Thaina commented 6 years ago

@gafter That's why I try and try to propose #249 that we should always have a way to create anonymous local function that was not create unnecessary garbage. Not even a delegate. And you can see most people just don't care about garbage and want it to be the same syntax as delegate

And as I said it should be compiler optimization to just know that If there are using in from of the line and ~> befire the block then don't create anything but just skip that logic to the end of the function

We don't need defer to achieve your goal just a context sensitive compiler. As @scalablecory said it just need a set of tricky Roslyn optimizations.

gafter commented 6 years ago

@Thaina As long as the language-defined semantics do not require the creation of a delegate or the lifting of variables into closure classes, we don't need any "optimizations" to implement defer. That is true even if we select a syntax for it that uses a sequence of tokens like using ~>. In that case it has nothing to do with IDisposable or Dispose or convention-based anything.

Thaina commented 6 years ago

@gafter I actually have a little difference opinion about this. I was actually think about using and ~> as a separate syntax, not a sequence token

I was thinking of using still being the syntax for calling disposal logic at the end of block. But I propose that ~> (could be any alternative syntax) is a syntax for creating disposal logic block separately from using. We then just implement compiler optimization trick to optimize it and call it at the end of function if it was in front of using

Generally we could just always make inline optimization for using with Dispose if we know that it was not a virtual function of the object we put in the using

It would be that from the start we might allow ~> only after using but eventually I wish that it could be a syntax for creating disposable logic on their own. As of my https://github.com/dotnet/roslyn/issues/16958

Korporal commented 5 years ago

If I understand correctly defer will behave exactly the same as finallyother than for the fact that an associated tryis not required or permitted. If that is the case then

  1. I don't see how adding an alternative keyword for finally improves the language.
  2. This amounts to making the try optional for finally only (no catch) blocks.

If a junior developer sees try andfinally in some parts of the code and defer in others they will ask (as they quite reasonably should) why the difference? is one better than the other for some scenarios?

Which is a good question why would we use defer instead of try finally? If the proposal then changes to "Let's make the try optional" then it begs the question whether we should also consider allowing catch to be used without try...

This then amounts to:

  1. Provide a new keyword for finally - defer
  2. Provide a new keyword for catch - trap
  3. Do not allow try to accompany these new keywords

OR

  1. Make the try keyword (and block) optional when using catch or finally.
svick commented 5 years ago

@Korporal I think you misunderstand what defer is about. The difference between finally and defer is that when you place defer in the middle of a block, it executes at the end of that block.

For example, look at the code in https://github.com/dotnet/csharplang/issues/513:

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

This would compile into (the position of "some code code using thing" is important):

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

The advantage here is that acquiring and releasing the resource are near each other in code, which makes the code clearer.

(Note that my personal opinion is that improving using would be a better choice for C# than introducing defer. But that doesn't mean I can't see the appeal.)

CyrusNajmabadi commented 5 years ago

If a junior developer sees try and finally in some parts of the code and defer in others they will ask (as they quite reasonably should) why the difference? is one better than the other for some scenarios?

Today, a junior developer will see using in some parts of the code and try/finally in others. If they ask "why the difference?" you can certainly explain what's going on. :) Similarly, the usage any language construct that effective 'rewrites' into anything else.

In this case, it's pretty easy to explain why things are better. For example, It's especially nice for avoid the try/finally pyramid of doom you can get in some cases. You can avoid that today if the values are IDisposable (where you can use 'using'). But in the cases where they aren't, 'defer' just makes it trivially easy to achieve the effect you want with much simpler code over what facilities are there today.

CyrusNajmabadi commented 5 years ago

Provide a new keyword for finally - defer

I don't see how that would work. Say i do the following:

SomeResource r = AcquireResource();
finally { DoSomethingWith(r); }

What semantics does that have? Because if you just want a shorthand for try/finally, then the above would have hte semantics.

SomeResource r = AcquireResource();
try { } finally { DoSomethingWith(r); }

But this isn't what i want. Because DoSomethingWith(r) will run immediately after i acquire the resource, instead of at the end of the block.

Now, say i move things to avoid that problem:

SomeResource r = AcquireResource();
...
// lots of code
...
finally { DoSomethingWith(r); }

I have a similar problem. This would be equivalent to

SomeResource r = AcquireResource();
...
// lots of code
...
try { } finally { DoSomethingWith(r); }

But this also wouldn't be what i wanted. Because it would mean that something may happen to cause early-exist of the method in the // lots of code section.

What i really want is to be able to write the following more easily:

SomeResource r = AcquireResource();
try
{
    ...
    // lots of code
    ...
}
finally { DoSomethingWith(r); }

Furthermore. I want to be able do that with a lot of resources. In the single resource case, it's not too bad. But when you have a bunch of things you need to individually and properly clean up, the try/finally pattern becomes really onerous and hard to get write. Contrast that with just being able to do:

SomeResource r = AcquireResource();
defer{ DoSomethingWith(r); }

SomeOtherResource r2 = ...;
defer{ DoSomethingWith(r2); }

etc. etc.

Now, i've clearly tied cleanup with acquisition. I've also made it so that whne i have many things to deal with, it's clear and simple. And i don't have the try/finally pyrimid of doom that is both onerous and easy to get wrong.

Zastai commented 5 years ago

Is the compiler supposed to look inside the defer block to see which variables are used, and then decide where the equivalent of try/finally should start? Or does it always start right after the preceding statement? Or only the first preceding declaration? Is the defer statement only allowed immediately after a declaration?

In other words, given

{
  // code

  SomeResource r = AcquireResource(), foo = null;
  defer{ DoSomethingWith(r); }

  // code

  SomeOtherResource r2 = ...;
  // code
  var someVar = 0;
  // code
  defer{ DoSomethingWith(r2); }

  // code
}

what would the equivalent try/finally construct be? I would assume it's just nested try/finally blocks at the enclosing scope, with earlier defers in inner finallys. That would require hoisting all locals outside of the try blocks (well, the ones referenced by defer blocks anyway).

{
  SomeResource r; // perhaps = default(SomeResource) so that the defer block never uses it uninitialized
  SomeResource r2; // perhaps = default(SomeResource) so that the defer block never uses it uninitialized
  try {
    try {
      // code

      r = AcquireResource();
      SomeResource foo = null;

      // code

      r2 = ...;
      // code
      var someVar = 0;
      // code

      // code
    }
    finally { DoSomethingWith(r); }
  }
  finally { DoSomethingWith(r2); }
}

I suppose it could also be aggregated into a single try/finally, but then later defer's won't execute if an earlier one throws.

All in all, I only see defer really obscuring what is happening when and in what order (would also make for interesting stepping behaviour when debugging).

TheBuzzSaw commented 5 years ago

@Zastai Why do you feel that defer obscures things while using does not?

Also, I disagree with the proposed transformation. The defer keyword would essentially put everything afterward into a try block. So, in your example...

{
  // code

  SomeResource r = AcquireResource(), foo = null;
  defer{ DoSomethingWith(r); }

  // code

  SomeOtherResource r2 = ...;
  // code
  var someVar = 0;
  // code
  defer{ DoSomethingWith(r2); }

  // code
}

... the transformation would look like this:

{
    // code

    SomeResource r = AcquireResource(), foo = null;
    try
    {
        // code

        SomeOtherResource r2 = ...;
        // code
        var someVar = 0;
        // code

        try
        {
            // code
        }
        finally
        {
            DoSomethingWith(r2);
        }
    }
    finally
    {
        DoSomethingWith(r);
    }
}

There is no hoisting required. The semantics are actually pretty clear.

To anyone else, please please please do not adopt the Go model. Preserving deferred actions until the method ends destroys performance and clarity.

Zastai commented 5 years ago

Ok, so defer is in no way tied to a variable, just to execution scope? In other words, you can’t have:

SomeResource r = Acquire();
if (foo)
  defer { DoSomething(r); }
else
  defer { DoSomethingElse(r); }
// continue using r

because the defers would effectively fire right away.

As for defer being less clear than using: the latter defines its scope clearly; for the former, you need to find the enclosing scope to see where it ends, which is less obvious.

TheBuzzSaw commented 5 years ago

@Zastai Yes, that's right. At least, that's how I would have it behave. I'm not sure if there are any dissenting opinions left on that.

The reason I call out using for being comparably unclear is that, when you see the closing brace, it is still up to the reader to remember that it was inside a using block. There is nothing at the bottom indicating that execution will (in a sense) leap back to the top and unwind a bunch of disposables. To be fair, others have criticized using for this very reason.

Zastai commented 5 years ago

@TheBuzzSaw *shrug* The closing brace of a for loop also does not indicate that it's going to run statements, test a condition and potentially loop. At least when you go to the matching brace (which many non-IDE editors let you do easily), you see what's going to happen. With defer, it can have been snuck in anywhere.

CyrusNajmabadi commented 5 years ago

To be fair, others have criticized using for this very reason.

This is true. Though, having used 'go' for quite a bit of time now, i can say that it doesn't ever seem to be an issue. I'm sure you could abuse this to make a hideous mess of things. But, in reality, it's pretty clear. The pattern is just

// get resource
// defer cleanup of that resource

// get resource
// defer cleanup of that resource

// do more stuff

Things work nicely and naturally and i don't know if we've ever had a moment of confusion or difficulty with this pattern.

Like with all language stuff, don't go crazy with it. Be clear and consistent in your coding. You'll normally find that things work out fine. :)

TheBuzzSaw commented 5 years ago

@CyrusNajmabadi That's been my feeling. It's not that much of a challenge, but it's hard to convey that with anecdotes.

4creators commented 5 years ago

How is it possible that feature which is overwhelmingly rejected by community becomes proposal champion? This raises serious doubts about correctness of feature selection process. Any comments?

@gafter @jaredpar

jaredpar commented 5 years ago

@4creators

How is it possible that feature which is overwhelmingly rejected by community becomes proposal champion?

LDM members choose which features to champion, not community feedback.

This raises serious doubts about correctness of feature selection process.

Who said this feature is being selected? The champion tag merely means a member of LDM is interested enough in a feature that they would be willing to represent it at LDM. It doesn't convey a feature is chosen, scheduled, planned, etc ...

4creators commented 5 years ago

Who said this feature is being selected?

@jaredpar

I inferred it from the fact that feature has already assigned milestone 8.X candidate. If feature is not accepted than perhaps it could better to not set milestone at all or at least set X.X milestone. This would improve communication on timing.

jaredpar commented 5 years ago

@4creators

I inferred it from the fact that feature has already assigned milestone 8.X candidate.

The milestone says "candidate" because it's just that: a candidate for a minor release of C# 8. It's not accepted, approved, etc ...

If feature is not accepted than perhaps it could better to not set milestone at all or at least set X.X milestone.

I described our use of milestones in another thread with you. The meaning has not changed.

theunrepentantgeek commented 5 years ago

@4creators, you wrote:

How is it possible that feature which is overwhelmingly rejected by community becomes proposal champion?

To quote @HaloFour, commenting on #52

Github represents the smallest channel through which Microsoft receives feedback. The up/downvotes here don't even represent statistical noise.

At the very least, this means that we don't get to see most of the signal - so all we know is that some of the readers don't like the proposal. Given there are 48 votes but only 17 participants on this issue, most of those voting haven't even told the rest of us why they voted the way they did.

FWIW, I haven't really participated in this discussion before now because, to me, the defer statement is entirely uncontroversial.

IMHO, defer is a good idea that can simplify a number of common coding scenarios. I don't believe it is any no more prone to misuse and abuse than a bunch of features we already have - including operator overloading, unsafe sections, var declarations, delegate valued properties, or nested generic declarations.

alrz commented 5 years ago

defer is a good idea that can simplify a number of common coding scenarios.

Guess what method is this:

defer array.RemoveLast();
return array.Last();

That is certainly simpler, but doesn't it count as a misuse?

theunrepentantgeek commented 5 years ago

It's a spectacularly inefficient way to implement a dequeue operation if the array represents a queue; or a pop operation if it represents a stack.

Whether that counts as a misuse would be up to the conventions & standards of your team. Assuming that code was the entire method, I'm pretty sure my team would be fine with it. YMMV.

TheBuzzSaw commented 5 years ago

@alrz Is this supposedly superior?

try
{
    return array.Last();
}
finally
{
    array.RemoveLast();
}
alrz commented 5 years ago

@TheBuzzSaw I would just use a local for that, since the order of execution would be lexical it wouldn't be confusing. Anywho, that's just an example of "misuse" in my book, could be acceptable for others.

4creators commented 5 years ago

FWIW, I haven't really participated in this discussion before now because, to me, the defer statement is entirely uncontroversial.

@theunrepentantgeek Thank you for joining discussion. The expression the defer statement is entirely uncontroversial is not entirely clear to me: does it mean that you are neutral about it (you would not vote for or against) or does it mean that your are in favor (in such case I hope that you have had upvoted proposal).

Despite that I am against introducing tons of unnecessary syntactic sugar to the language including this feature (I am on the side of puristic restraint) I am in favor of engaging everyone in discussion and voting on proposals.

I would be even happier if community would start comparing and explicitly prioritizing proposals. There are tons of missing features and I always like to ask: how would you do generic numeric algorithms in C#?

TheUnlocked commented 5 years ago

@4creators As has been mentioned both in this thread and others that you've participated in, the LDT and implementation teams decide on and prioritize features using far more information about users' needs and the teams' own capabilities than what can be provided by this small slice of the community.

In general though, this particular issue thread is not about discussing the system of prioritizing and deciding on features, but rather how defer would/should work and the merits/demerits of adding it to C#.

theunrepentantgeek commented 5 years ago

The expression the defer statement is entirely uncontroversial is not entirely clear to me: does it mean that you are neutral about it (you would not vote for or against) or does it mean that your are in favor (in such case I hope that you have had upvoted proposal).

I mean that I'm surprised that there is any controversy around the feature. I think defer is a good idea, and I'm surprised by the levels of vitriol and hatred that people are expressing against it.

(As an aside, across this repo there's also a growing level of attack against members of the LDM that is quite disheartening, and in my opinion quite unjustified.)

I have (just today) up-voted the defer proposal.

CyrusNajmabadi commented 5 years ago

At the very least, this means that we don't get to see most of the signal - so all we know is that some of the readers don't like the proposal. Given there are 48 votes but only 17 participants on this issue, most of those voting haven't even told the rest of us why they voted the way they did.

On top of that, this isn't really a democracy. Github Votes are just a way for community members to express their views. But the LDM is not beholden to those views at all. It is (as @theunrepentantgeek ) mentions one form of signal. But it's just considered along with everything else the LDM considers when it comes to language decisions.

I am in favor of engaging everyone in discussion and voting on proposals.

You are welcome to do both. However, the voting isn't likely to ever be anything more that a way of expressing your opinion, and nothing more.