dotnet / csharplang

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

Champion: Implicitly scoped using statement (16.3, Core 3) #114

Open HaloFour opened 7 years ago

HaloFour commented 7 years ago

See https://github.com/dotnet/roslyn/issues/5881 and https://github.com/dotnet/roslyn/issues/181

Motivation (TL;DR edition):

Allow using keyword to be used to declare a variable that will be disposed at the end of the current block. This avoids creating a new block with an additional level of indentation:

Before:

public void Foo() {
    using (var connection = new SqlConnection(connectionString)) {
        connection.Open();
        using (var command = connection.CreateCommand()) {
            command.CommandText = "SELECT FOO FROM BAR";
            using (var reader = command.ExecuteReader()) {
                while (reader.Read()) {
                    ProcessRecord(reader);
                }
            }
        }
    }
}

After:

public void Foo() {
    using var connection = new SqlConnection(connectionString));
    connection.Open();
    using var command = connection.CreateCommand());
    command.CommandText = "SELECT FOO FROM BAR";
    using var reader = command.ExecuteReader());
    while (reader.Read()) {
        ProcessRecord(reader);
    }
}

As an implementation detail, the variables would be disposed in the reverse order in which they are declared. So given the above code, the order in which the resources would be disposed is reader, command and then connection.

scottdorman commented 7 years ago

I think this has a potential to introduce a lot of unintended consequences and unexpected behavior. How is this any better than the "before" code, except I didn't need to type 2 extra sets of parens and 2 extra sets of curly braces?

Using already creates an implicit single-line block, in much the same way as a single line if statement. Dropping the curly braces has too much risk to introduce bugs in the code, in much the same way a single line if statement without curly braces can. (Adding what you think is a second statement in the if block, but because there aren't curly braces it's actually not part of the if block even if it's indented.)

Disposability is non-deterministic and there isn't anything the compiler can do to tell the runtime GC that things should be disposed of in a certain order.

HaloFour commented 7 years ago

How is this any better than the "before" code, except I didn't need to type 2 extra sets of parens and 2 extra sets of curly braces?

And extra indentation. When your code doesn't even start until half-way off of the screen it's a big waste of real-estate.

Disposability is non-deterministic and there isn't anything the compiler can do to tell the runtime GC that things should be disposed of in a certain order.

This has nothing to do with GC. It is strictly syntax candy for using blocks, and it is strictly deterministic. The reverse declaration disposal order is the expected behavior.

jnm2 commented 7 years ago

I'll copy my comments from https://github.com/dotnet/roslyn/issues/16579:

This is very easy to confuse visually (I intentionally indented it wrong):

using var stream1 = new MemoryStream();
    string v1 = new StreamReader(stream1).ReadToEnd();

using (var stream1 = new MemoryStream())
    string v1 = new StreamReader(stream1).ReadToEnd();

Even worse, here the indentation is fine:

using var stream1 = new MemoryStream(); // Oops, stream1 will live on
using (var deflate = new DeflateStream(stream1, CompressionMode.Decompress))
    string v1 = new StreamReader(deflate).ReadToEnd();

using (var stream1 = new MemoryStream())
using (var deflate = new DeflateStream(stream1, CompressionMode.Decompress))
    string v1 = new StreamReader(deflate).ReadToEnd();

On the humorous side, since you aren't requiring parentheses, this could be legal:

private static ILolz System;

void Lolz()
{
    using System.Text;
    // ...
}
scottdorman commented 7 years ago

And extra indentation. When your code doesn't even start until half-way off of the screen it's a big waste of real-estate.

I'd argue that if you have so many nested using statements (or other nested code blocks) where that's a problem then you need to refactor into simpler/smaller methods. That point aside, I still don't see this giving us a real benefit but introduces a lot of places which will lead to incorrect code. (See @jnm2's earlier comment. All of the issues mentioned are legitimate concerns, are harder to read, and can introduce even harder to find runtime bugs.

This has nothing to do with GC. It is strictly syntax candy for using blocks, and it is strictly deterministic. The reverse declaration disposal order is the expected behavior.

It does have to do with GC as the using statement has to do with GC since it's expanded by the compiler to a call to IDisposable.Dispose(). Since GC is nondeterministic, we have no control over the order which the GC will decide to dispose of objects once they're out of scope so you can't say that this is deterministic, especially if you're saying that it's merely syntax sugar for using blocks.

jnm2 commented 7 years ago

@scottdorman @HaloFour's proposal still has the disposable.Dispose() call deterministically, but now it's invisible and as late as possible without the disposable instance going out of scope. GC is not involved because it's always disposed in the scope it was created.

scottdorman commented 7 years ago

@jnm2 Ok. I think I understand the meaning here now. It's deterministic in the sense that this code

public void Foo() {
    using var connection = new SqlConnection(connectionString));
    connection.Open();
    using var command = connection.CreateCommand());
    command.CommandText = "SELECT FOO FROM BAR";
    using var reader = command.ExecuteReader());
    while (reader.Read()) {
        ProcessRecord(reader);
    }
}

will (roughly) expand to this code:

public void Foo() {
   var connection = new SqlConnection(connectionString));
   try {
      connection.Open();
      var command = connection.CreateCommand();
      try {
         command.CommandText = "SELECT FOO FROM BAR";
         var reader = command.ExecuteReader();
         try {
            while (reader.Read()) {
           ProcessRecord(reader);
            }
         }
         finally {
            ((IDisposable)reader)?.Dispose();
         }
      }
      finally {
         ((IDisposable)command)?.Dispose();
      }
   }
   finally {
      ((IDisposable)connection)?.Dispose();
   }
}

Which means the order of the calls to Dispose() are in the same order. But that's it, the GC doesn't really care what order they're called in and isn't guaranteed to release the resources in that order. Just because you call Dispose() does not mean the GC has done any work yet.

jnm2 commented 7 years ago

@scottdorman I don't like this syntax sugar at all, but the GC is not affected by this proposal. If you manually type the braces for the using statements, they way we work today, the effect on the GC is identical.

scottdorman commented 7 years ago

@jnm2 The motivation in the initial post says

So given the above code, the order in which the resources would be disposed is reader, command and then connection.

Maybe it's just the way I read it, but it specificly defines an order in which the resources are expected to be disposed, which isn't possible.

Other than that, you're right, this proposal theoretically doesn't change the compiler generated expansion of the using statements, so the resulting code should be identical to what is produced today.

I'm still not a fan of this syntax as I think it has a huge potential to introduce hard to find/debug runtime errors and decreases the code readability.

jnm2 commented 7 years ago

But you do have that guaranteed order of resource release in both scenarios! The GC doesn't release resources like connections, commands and readers. Dispose does that, deterministically, right when you tell it to. All GC does is free managed memory when it can be proved that freeing the memory has no effect on the executing code. Dispose and GC honestly have almost nothing to do with each other.

Opiumtm commented 7 years ago

@jnm2

Dispose and GC honestly have almost nothing to do with each other.

Not completely right.

Finalizers are called by GC and most holders of unmanaged resources do implement finalizer. SafeHandle, for example, is freed via finalizer by GC if don't freed explicitly already. When IDisposable.Dispose() is called, finalizer is suppressed.

There are two drawbacks of finalizers (and it's why they're not used in most real-life scenarios and IDisposable.Dispose() is used instead).

  1. No guaranteed order of execution
  2. Relatively big cost of finalizers as they require more work on GC thread and can cause noticeable performance degradation
HaloFour commented 7 years ago

@scottdorman

Yes, this proposed syntax is intended to behave (and expand) exactly the same as using blocks do today, except that instead of specifying a separate nested block that the compiler uses the remainder of the current block. The "Before" and "After" code blocks in the proposal are intended to behave exactly the same. This is strictly compiler candy dealing with calling Dispose on disposables.

As for excessive nesting, the example above is real world and fairly common. It's three extra levels of nesting just for the sake of defining those blocks. In some cases it is possible to avoid that nesting. You can declare multiple resources in the same using statement, but that requires that they all be of the same type. Or you can use another using statement as the embedded statement of another using statement. But, as in the example above, you have to invoke any code on the resource before defining another resource, it's currently not possible to avoid the nesting. And refactoring code as simple as such above would require significantly more work, especially since you'd have to return something representing a composite disposable but still exposes the same contract as the original resource. This proposal simplifies all of that.

I'll give @jnm2 the concern that the syntax can be confusing. Afterall, the difference is only in parenthesis. Of the proposals on the Roslyn repo this was the syntax that @gafter seemed to prefer, so that's why I re-proposed it here. I'd be willing to explore other syntax.

scottdorman commented 7 years ago

@jnm2

Dispose and GC honestly have almost nothing to do with each other.

They absolutely have everything to do with each other. The only reason IDisposable existed in the first place was to provide us a way to manually release unmanaged resources as early as possible.

When IDisposable.Dispose() is called, finalizer is suppressed.

Only if it's written properly, which many are not.

SafeHandle was created well after .NET released partly because there were so many incorrect implementations when dealing with IntPtrs. The SafeHandle derived classes are meant to wrap IntPtrs so the dispose pattern is implemented properly without relying on the developer to "get it right".

Dispose does that, deterministically, right when you tell it to. All GC does is free managed memory when it can be proved that freeing the memory has no effect on the executing code.

GC deals with both managed and unmanaged resources and does so by calling Dispose(bool) and/or the finalizer. It doesn't know how to release memory, only how to figure out if there is still a live reference to an object (and it really only does that work itself as a last resort and instead asks various parts of the runtime, like the JIT, for what's still alive). The actual mechanism of releasing any resources is done so in the context of the Dispose(bool) method and/or finalizer.

scottdorman commented 7 years ago

@HaloFour Yes, I realize the code example you gave is very real-world and have written many such blocks myself but have never felt it burdensome. To be honest, if we're going to introduce syntax sugar around dispose, I'd much rather see syntax sugar introduced that makes it easier/trivial to write the pattern properly (the IDisposable.Dispose() method is correct, calls to the base classDispose` method are chained in properly, etc.) than syntax sugar that reduces the amount of indention and curly braces I have to write.

That is, in my opinion, completely trivial when compared with ensuring the pattern is correct. A great example of this is a strongly typed WCF client, which does not follow the pattern properly and cannot be used inside a using statement as it will throw an exception when the scope is closed.

This proposal reduces the amount of indenting and curly braces I have to write, but introduces a much greater risk for invalid code due to the syntax. Given the syntax between the two referenced proposals, this syntax is better, but I still disagree that this feature is valuable.

mikedn commented 7 years ago

using var stream1 = new MemoryStream(); // Oops, stream1 will live on

So what? In most cases this is at best a performance issue because the object may be disposed later than it could be. It may sometimes be a correctness issue if you're trying to reuse a resource that you think you released earlier but in fact you did not:

using Stream s1 = File.Open(...);
s1.Write...
using Stream s2 = File.Open(...); // tough luck, this may fail due to sharing violation
s2.Write...

But such code is rare in my experience.

SafeHandle was created well after .NET released partly because there were so many incorrect implementations when dealing with IntPtrs.

Sort of. It would be more correct to say that it was created because it is practically impossible to deal with IntPtr safely. But what does this has to do with this issue? This is not about how GC works, it's about some rather trivial syntactic sugar.

I'm mostly "for" this proposal but that may be due my C++ background where this is basically the "default" syntax.

That said, I think that the most important drawback of this proposal is that it introduces an alternative way to do things. Such alternatives have already been introduced in the language but usually the new way is better and becomes the default while the old way becomes irrelevant (e.g. anonymous methods vs. lambda expressions).

HaloFour commented 7 years ago

@scottdorman

Yes, I realize the code example you gave is very real-world and have written many such blocks myself but have never felt it burdensome.

I'll let you borrow my Surface Pro 2 for a while. It was my primary development machine for a couple of years and it served that purpose very well. Not everyone has really wide monitors. 😄

To be honest, if we're going to introduce syntax sugar around dispose, I'd much rather see syntax sugar introduced that makes it easier/trivial to write the pattern properly

Well, unless this becomes an either/or proposition and it's impossible for the team to consider both separately, there's no harm in proposing it.

I completely acknowledge that this proposal is very minor in the grand scheme of things. I didn't even originally propose it, either on CodePlex or on Github. I just liked it enough, and knew that it at least had a little nod from members on the LDM team, that I thought it deserved to be brought over here.

scottdorman commented 7 years ago

@HaloFour

I'll let you borrow my Surface Pro 2 for a while. It was my primary development machine for a couple of years and it served that purpose very well. Not everyone has really wide monitors.

Fair enough. Screen size constraints are a real thing. I have a 3 monitor arrangement on my main development system, but definitely feel the loss of productivity when I work on my single-screen 15" laptop.

Well, unless this becomes an either/or proposition and it's impossible for the team to consider both separately, there's no harm in proposing it. I completely acknowledge that this proposal is very minor in the grand scheme of things. I didn't even originally propose it, either on CodePlex or on Github. I just liked it enough, and knew that it at least had a little nod from members on the LDM team, that I thought it deserved to be brought over here.

Absolutely. We would all be doing ourselves (and the language team) a disservice if we didn't bring up a proposal and weren't able to have discussions about it. That's the whole point here. Just because somebody (me, in this instance) may disagree with a proposal, doesn't mean it shouldn't still be discussed.

sharwell commented 7 years ago

@scottdorman It sounds like you are confusing IDisposable.Dispose with finalization in some of your comments above. Consider the simple case of using a FileStream to write to a file. In this case, there are two resources of interest:

  1. The handle to the open file provided by the operating system
  2. The instance of FileStream being used by the application

While each of these resources needs to be freed at some point, the timing of the first is particularly important - until the OS handle is closed, a user will not be able to open Explorer and delete the file. To ensure resources like this are only kept open while they are used, the IDisposable interface (and by extension the using statement) allows the user to explicitly release the operating system handle after the file is no longer being written. The second resource, which is the FileStream instance itself, doesn't have observable side effects like the OS handle, so it's simply left for the garbage collector to reclaim at some point in the future. There are two important observations here:

  1. User code is solely responsible for releasing unmanaged resources, such as the operating system handle from the example. (Sometimes this is implemented in the base class libraries, but the point is it's not part of the GC.)
  2. The GC is responsible for reclaiming the memory allocated to instances of managed objects which are not longer in use, such as the FileStream instance from the example.

The using statement is a language construct which allows users to perform the operation they are responsible for in a deterministic manner. Finalizers exist as a fallback to avoid memory leaks over time due to misuse of disposable objects (failure to call Dispose, followed by the object no longer being used). However, finalizers incur substantial overhead for tracking and have extremely difficult-to-understand semantics, so they should only be implemented on small objects whose sole purpose is exposing the unmanaged resource to managed code. Since the .NET Framework (and now .NET Standard) provides two very flexible base classes for these wrappers - SafeHandle and CriticalHandle - we are now at a place where user-defined finalizers should essentially disappear from application code. In the end the only part we really need to deal with is IDisposable.Dispose and figuring out when to call it ourselves (with a using statement where possible). In this way I think what @HaloFour mentioned regarding using not being about the GC makes sense. Even in the total absence of a GC the using statement could be used to release unmanaged resources such as OS handles.

I cannot recommend the following highly enough: http://joeduffyblog.com/2005/04/08/dg-update-dispose-finalization-and-resource-management/

iam3yal commented 7 years ago

Reducing the level of nesting is a great thing when it comes to make code legible and the syntax is almost the same so why not? :)

Reiterating on what @MadsTorgersen wrote:

So let's not change the subject here, I mean it's about adding an alternative syntax so derailing the discussion into GC and/or resource management is really not the place to do it.

sharwell commented 7 years ago

I lean in favor of this proposal, but perhaps not for the reasons mentioned previously. One example where I think this could be useful is in recording telemetry around actions invoked by a user. In these situations, the telemetry recording which wraps the "real" work of a method can often be implemented using a using statement, but it isn't really a resource and the method implementation typically wouldn't interact with it. For example:

// BEFORE
public async Task UserOperationAsync()
{
  using (Telemetry.Operation(nameof(RunOperationAsync)))
  {
    // work here
  }
}

// AFTER
public async Task UserOperationAsync()
{
  using Telemetry.Operation(nameof(RunOperationAsync));

  // work here
}

:warning: My primary point of concern at this point is the ambiguity with parenthesized expressions. If the new form of using statement was defined as the following:

scoped_using_statement
  : 'using' resource_acquisition ';'
  ;

:dragon: Then the resource_acquisition here could actually be a parenthesized_expression, and you end up with a valid using_statement with totally different semantics in the current language.

:memo: I would consider this proposal relevant/related to #85, but while the scenarios overlap I wouldn't say either fully encompasses the other.

iam3yal commented 7 years ago

I don't know but how do you feel about the following?

var connection = using new SqlConnection(connectionString));
HaloFour commented 7 years ago

@eyalsk

My only concerns there would be that it implies that the behavior is detached from the declaration, which is a big departure from the current behavior where the variable is declared at that point and is readonly. Would that assignment be legal to an existing variable? Could the variable be reassigned?

Those differences might not be blockers, though. The compiler could still ensure that the instance itself is disposed, but there might be a question as to which scope that instance would be attached to. That of the variable, or that of where the using expression is evaluated:

public void Foo() {
    SqlConnection connection = null;
    if (condition) {
        connection = using new SqlConnection(connectionString);
        // do stuff here
    }
    // do other stuff here
}

// equivalent to

public void Foo() {
    SqlConnection connection = null;
    if (condition) {
        using (var $temp = new SqlConnection(connectionString)) {
            connection = $temp;
            // do stuff here
        }
    }
    // do other stuff here
}

// or equivalent to

public void Foo() {
    SqlConnection connection = null;
    try {
        if (condition) {
            connection = new SqlConnection(connectionString);
            // do stuff here
        }
        // do other stuff here
    }
    finally {
        (connection as IDisposable)?.Dispose();
    }
}

Personally I'd prefer the former. It might be weird that the variable lifetime and scope outlives that of the resource, and attempting to use it afterwards could result in exceptions. The compiler could potentially warn about that. It's no different than assigning off a reference to that instance anyway, which you've always been able to do.

Furthermore, should the compiler care if you assign that instance to fields or ref/out parameters? Probably not, for the same reason above.

iam3yal commented 7 years ago

@HaloFour

Would that assignment be legal to an existing variable? Could the variable be reassigned?

Excellent point.

The reason I suggested it is because people raised the concern where it looks too similar to the current statement and it might be confusing but yeah I fully agree with your point.

scottdorman commented 7 years ago

@sharwell Fair enough (some of those comments may not have been as clear as they could have been). Yes, with the use of SafeHandle and CriticalHandle it's almost never necessary to implement a finalizer anymore.

@eyalsk Agreed. The discussion about GC is off-topic to this proposal. Sorry for the temporary derailment.

@sharwell's point about this being more akin to a scoped unit of work type scenario is valid and I'd still much rather see something around that than a different syntax to simply reduce the amount of indenting and which can introduce correctness problems.

Thaina commented 7 years ago

@jnm2 Copied from my proposal too

First, for

using (var stream1 = new MemoryStream())
string v1 = new StreamReader(stream1).ReadToEnd();

There are error Embedded statement cannot be a declaration or labeled statement

Second, for

using (var stream1 = new MemoryStream())
string v1 = new StreamReader(stream1).ReadToEnd();

using (var stream1 = new MemoryStream())
using var deflate = new DeflateStream(stream1, CompressionMode.Decompress);
    string v1 = new StreamReader(deflate).ReadToEnd();

It will become obvious if you let formatter do indent

using (var stream1 = new MemoryStream())
    string v1 = new StreamReader(stream1).ReadToEnd();

using (var stream1 = new MemoryStream())
    using var deflate = new DeflateStream(stream1, CompressionMode.Decompress);
string v1 = new StreamReader(deflate).ReadToEnd();

And Embedded statement cannot be a declaration or labeled statement Will pop out too

Thaina commented 7 years ago

We could avoid reassignment by forcing using var cannot be reassign. That would be most obvious solution

Thaina commented 7 years ago

If there are condition need to met I would use ternary operator

public void Foo()
{
    using var connection = condition ? new SqlConnection(connectionString) : null;
    if (connection != null) {
        // do stuff here
    }
    // do other stuff here
}
jnm2 commented 7 years ago

@Thaina like I said before: the embedded statement error doesn't matter. Pretend I removed the word var and my original point still stands.

Like I said before: the formatter does not indent like you say. Because there are no braces, both using statements stay at the same level of indent. This is the style used everywhere I've ever seen. So my original point here still stands as well.

Thaina commented 7 years ago

@jnm2 And like I was response to you that if you don't assign variable in that line then it would not cause any problem. It will only be a problem when you try to use any variable

using (var stream1 = new MemoryStream())
    new StreamReader(stream1).ReadToEnd();
// vs
using var stream1 = new MemoryStream();
    new StreamReader(stream1).ReadToEnd();

What would be difference? it only that you would have stream1 in difference scope and just that

jnm2 commented 7 years ago

it only that you would have stream1 in difference scope and just that

For a MemoryStream, that's not really a problem. For a file stream, it could be. For all the disparate things we use using to scope, like registrations, it could escape notice and be very bad.

Thaina commented 7 years ago

@jnm2 Difference scope means you would not be able to use that fileStream if it already disposed. And if you can use that variable it means you are not dispose it yet because the scope of using was not bracketed

But if you mean you will try to access fileStream that you already readToEnd. I think that's not the problem of using scope anymore. You could refresh the seeker to read it again so it up to your logic. We also help guard that in scope of function, using var would not be able to reassign

jnm2 commented 7 years ago

The problem is when you think the file or event or state registration is disposed and you don't try to use the variable, but it has side effects with other things. (Like, keeping a file locked.)

Thaina commented 7 years ago

@jnm2 So you mean you will try to access the same file 2 times consecutively while the first time you just write code one line to read it without doing nothing?

I think this is very extreme case that so wrong in many level

jnm2 commented 7 years ago

No. Say you read or write the whole file, but accidentally leave it locked. Then you call out to other libraries and distant parts of the application which may or may not end up trying to delete or overwrite or read the file. Perhaps the entire method is in the stack for a sizable duration of time and things are happening that aren't just related to the contents of the method. Cake build scripts are a particularly good example of this.

scottdorman commented 7 years ago

For all the disparate things we use using to scope, like registrations, it could escape notice and be very bad...The problem is when you think the file or event or state registration is disposed and you don't try to use the variable, but it has side effects with other things. (Like, keeping a file locked.)

And that, in my opinion, is one of the bigger and more important problems with having implicitly defined scope. Since the scope block is effectively hidden from view, it's extremely easy to introduce ambiguities and unintended side effects without realizing it. (This is very similar to the issues that can be experienced with single-line if statements that don't contain curly braces. You rely on the indenting to "see" the implied scope, but indenting can be wrong and lead you into problems.)

Thaina commented 7 years ago

@scottdorman @jnm2 OK I got your point now

But I think now the problem is opposite. It not that using var is the problem. The problem is because we allow one line using like one line if. And it make ambiguity from that. While using var is just intend to always dispose at the end of function

But then again. We might solve this problem with using syntax var a = using LoadSomething()

And the code would transpile into

using(var __using_0 = LoadSomething())
{
    var a = __using_0; // a will be just normal object to ensure dispose on what really using
}
mikedn commented 7 years ago

using (var stream1 = new MemoryStream()) using var deflate = new DeflateStream(stream1, CompressionMode.Decompress); string v1 = new StreamReader(deflate).ReadToEnd();

Indented or not this would be an error because the deflate variable is used in a scope where it does not exist. So I have no idea why indentation is brought into this.

Like I said before: the formatter does not indent like you say. Because there are no braces, both using statements stay at the same level of indent. This is the style used everywhere I've ever seen. So my original point here still stands as well.

If you're referring to the above example then I don't see what the formatter has to do with it. We're talking about a language feature that does not exist.

Say you read or write the whole file, but accidentally leave it locked.

Many things can happen accidentally. Including dropping new code inside an existing using block instead of outside it. While the scope widening that this feature inherently generates is definitely a concern I don't think it can be considered as a show stopper.

And that, in my opinion, is one of the bigger and more important problems with having implicitly defined scope.

This feature doesn't introduce any implicitly defined scope.

phi1010 commented 7 years ago

@jnm2, @Thaina: I consider some of the suggestions for indentation mentioned quite weird:

Like I said before: the formatter does not indent like you say. Because there are no braces, both using statements stay at the same level of indent. This is the style used everywhere I've ever seen.

This may be true when using a generic indenter only relying on braces, but in C#, scopes/blocks can also be generated with other statements, and I would expect them to be indented without using braces:

using (IDisposable foo = null)
    Console.WriteLine(foo); // new scope, indented
Console.WriteLine(foo); // error, foo not declared.

Thus, I would definitly expect indents to be consistent, interpreting any using (var ... = ...) as new scope, indenting the following lines, and interpreting any using var ... = ... as a variable assignment, not changing the indentation in any way.

Both of these variants mentioned above seem absolutely wrong -- regardless of whether they would cause compiler errors or not:

using var stream1 = new MemoryStream();
    new StreamReader(stream1).ReadToEnd();
using (var stream1 = new MemoryStream())
using var deflate = new DeflateStream(stream1, CompressionMode.Decompress);
    string v1 = new StreamReader(deflate).ReadToEnd();

@scottdorman

Dropping the curly braces has too much risk to introduce bugs in the code, in much the same way a single line if statement without curly braces can.

Whether curly braces are deemed to improve or reduce readability is a controverse topic, but since the current C# specification already allows not using them, there is no way to prevent bugs without using any automatic tool that either enforces correct indent or correct use of braces -- both variants leading to readable code, with different opinions about whether these styles are pleasant to look at.

Since imho such a tool (autoformatter or automatic code inspector) is mandatory, I would not want to rate this feature based on whether wrong indentation or missing braces can be misleading.

jnm2 commented 7 years ago

@phi1010 It's widely common practice to align all usings with single indent for the statement or body. Tooling aligns it this way and you see it all over. This is a common sight:

using (var file = File.OpenRead(path))
using (var decompressed = new DeflateStream(file, CompressionMode.CompressionMode.Decompress))
using (var jsonReader = new JsonReader(new StreamReader(decompressed)))
{
    list.AddRange(serializer.Deserialize<List<Foo>>(jsonReader));
}

Making the top line using var file = File.OpenRead(path); is not as quickly differentiable as I'd like.

This also raises the question, if it's worth doing for using statements, why aren't we doing it for lock statements? For statements? While statements? Checked and unchecked, fixed? We nest all these things. If the nesting is too deep, you can refactor into a separate method. I don't see what makes using statements special in this regard. Sure, it's different, but it's a reach to say that it's different enough for this treatment.

Thaina commented 7 years ago

I was once propose that we should return IEnumerable from for and while too. The same as try and if and switch. I propose that we should have var a = switch(obj) but it was overshadowed by match syntax

phi1010 commented 7 years ago

@jnm2 : You have a point, I also configure my IDE to have multiple usings on the same line, starting the indentation at the first line not starting with a using (or the line after it, if it begins with a brace) -- although I usually do not use braces at all, if possible.

As for why it has been suggested for usings, and subjectively seems more practical: Cleaning up (disposing objects) is something that might often be done only at the end of a method.

Why I have no interes in the same suggestion for locks:

My reasons for for and while are similar; I rarely use checked/unchecked, usually only as a compiler option. As for fixed: Why not, it seems quite useful -- but first we should find a usable syntax for the using one. If the GC had a pair of methods Fix<T>(ref T) and Unfix<T>(ref T), we could also get the fixed problem fixed by using a using construct: using Fixed(ref myvalue);

sharwell commented 7 years ago

💭 I need to look over the entire thread again, but it sounds like dotnet/roslyn#161 could offer a more flexible and powerful solution to what is discussed here. Should something like that be implemented, it would be trivial to create a destructable type which wrapped an IDisposable type and simply called Dispose in the destructor (meaning there is no use case supported by this issue which could not be trivially supported by the one I linked to).

alrz commented 7 years ago

@sharwell

With this feature one can have an analyzer to "simplify" a using statement which contains entirety of its enclosing block to a using var declaration. You can't just "simplify" code to use that wrapper type. We can't make disposable types destructible neither, because it's likely to be a breaking change.

TyOverby commented 6 years ago

Not having gone through the csharplang issues before, I created a proposal that is very similar to the one proposed here.

Here is a link to my proposal for posterity.

The main difference between #114 and my proposal is that in my proposal there is a two-word keyword using scoped which removes the ambiguity with using expression and using (expression) and also makes the statement more visibly different from similar lines, making it harder to confuse them.

CyrusNajmabadi commented 6 years ago

IME, the deeply nested using problem hasn't really arisen. Usually tehre are a few usings in a row (which of course don't need nesting except the last one), and then maybe one more using inside that. All in all, it's just not bad enough to warrant any changes.

Now, what i wouldn't mind is something like "defer". That way i could have my 'run at end of scope' concept, but not have to wrap things with a Disposable struct in order to clean them up.

TyOverby commented 6 years ago

@CyrusNajmabadi I work on a service now and we have deeply nested usings all over the place, mostly for @sharwell's example of telemetry:

public async Task UserOperationAsync()
{
  using (Telemetry.Operation(nameof(RunOperationAsync)))
  {
    // work here
  }
}

Except that we occasionally have multiple of these in a method (inside if-else cases for example).

Your "defer" example could be implemented as

class Deferable: IDisposable {
   private Action _action;
   private Deferable(Action action) { _action = action; }
   public static Deferable Defer(Action action) => new Deferable(action);
   override void Dispose() { _action(); }
}

And then the usage would be

using scoped Defer(() => Console.log("end"));

but more importantly (for me), my example would turn into

using scoped Operation("Operation Name");

And it would provide metrics on initialization and shutdown, being able to share state between them.

CyrusNajmabadi commented 6 years ago
public async Task UserOperationAsync()
{
  using (Telemetry.Operation(nameof(RunOperationAsync)))
  {
    // work here
  }
}

That doesn't look deeply nested to me :)

CyrusNajmabadi commented 6 years ago

Your "defer" example could be implemented as

Note: i would not want defer to cause allocations.

HaloFour commented 6 years ago

@CyrusNajmabadi

That doesn't look deeply nested to me :)

Add telemetry and a couple of resources which can't be all instantiated back to back, plus namespace, class and method indentation and you're already 20+ spaces over. If you don't think that's a lot then I'll let you borrow my 10" Surface Pro 2 which was my primary development machine for about 4 years.

Note: i would not want defer to cause allocations.

It wouldn't have to if you guys allowed for Dispose by convention like you do with so many other language features. Then you could have value disposables just like you have value enumerators. But you guys enjoy being inconsistent. 😉

Every language that has a defer feature recommends it specifically and explicitly for resource cleanup. It is considered bad practice to use it for anything else. C# has a mechanism for resource cleanup.

CyrusNajmabadi commented 6 years ago

But you guys enjoy being inconsistent.

I don't see why teh snark is necessary. It's not like people are going out of their way to do this. This is a language that has been around a long time and which always has work being done with it continuously. That means that not every issue can be addressed. Would you like it if your customers treated every rough edge in any product you made as being maliciously intentional? :-/

It wouldn't have to if you guys allowed for Dispose by convention like you do with so many other language features

I don't see how that would help here. The Deferable type had an private Action _action. Delegates cause allocations today. So you'd have to pay that price. I would prefer an alloc free way to have this feature, and i don't see how to do that without language support.

yaakov-h commented 6 years ago

Wasn't there a proposal somewhere for zero-allocation delegates?