dotnet / csharplang

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

Champion "Implicitly scoped using statement" (16.3, Core 3) #1174

Open gafter opened 6 years ago

gafter commented 6 years ago

This is for some form of using statement, where the statements executed while the resource is held are the following statements in the same block that contains the using statement; the resource is freed at the end of the block.

See https://github.com/dotnet/csharplang/issues/114 and https://github.com/dotnet/csharplang/issues/114#issuecomment-349773994

LDM history:

DavidArno commented 6 years ago

Downvote retracted. It was based on a misunderstanding on my part, over both how the current syntax works and a misreading of the new syntax.

Downvoting this. I didn't see the need for it when @HaloFour first proposed it, and I don't see a need now.

This is a "saving a few characters and a bit of indentation" proposal that makes the code harder to read as the usings have no obvious scope and just "magically" disappear at the end of the method. And since we now have local functions, there's even less need for it. Taken the original code example, it can already be expressed as:

public void Foo()
{
    using (var connection = OpenConnection(new SqlConnection(connectionString)))
    using (var command = SetupCommand(connection.CreateCommand()))
    using (var reader = command.ExecuteReader())
    {
        while (reader.Read())
        {
            ProcessRecord(reader);
        }
    }

    SqlConnection OpenConnection(SqlConnection connection)
    {
        connection.Open();
        return connection;
    }

    SqlCommand SetupCommand(SqlCommand command)
    {
        command.CommandText = "SELECT FOO FROM BAR";
        return command;
    }
}

In my view, in the above code it is immediately apparent as to the scope of the using blocks, which the proposal in #114 lacks.

HaloFour commented 6 years ago

@DavidArno

That instantly doubles the length of the method. Sure, it works, and sure, it avoids indentation, but at the cost of much unnecessary verbosity.

The scope of variables within a block is already a well defined concept in C#. There's no more "magick" here than should be obvious and expected.

mikedn commented 6 years ago

the usings have no obvious scope and just "magically" disappear at the end of the method

They do not disappear at the end of the method, they disappear at the end of the scope the variable was declared in. That's natural, there's nothing magic about it.

In fact it can be argued that the current using makes no sense because it forces you to create a new scope for no reason. Though it's sometimes useful when you need the using scope to be smaller than an already existing scope. When this happens in C++ what people do is create e block just for that:

void foo()
{
    {
        ObjX x;
       // use x...
    } // destroy x

    // some other code which does not need x
}

It's a bit weird but in my experience this need does arise too often.

gulshan commented 6 years ago

Kotlin has apply and also functions in its stdlib, which perform an action on a object and returns it. With them the code can be like-

public void Foo()
{
    using (var connection = new SqlConnection(connectionString).Apply(c => { c.Open; }))
    using (var command = connection.CreateCommand()
                          .Apply(c => { c.CommandText = "SELECT FOO FROM BAR"; }))
    using (var reader = command.ExecuteReader())
    {
        while (reader.Read())
        {
            ProcessRecord(reader);
        }
    }
}
DavidArno commented 6 years ago

@mikedn,

They do not disappear at the end of the method, they disappear at the end of the scope the variable was declared in.

Oh, that would be nasty. That suddenly brings all the issues around ; that C suffers from with while etc when a ; is accidentally placed after it:

using (var connection = OpenConnection(new SqlConnection(connectionString)))
using (var command = SetupCommand(connection.CreateCommand()));  // <- semicolon here
using (var reader = command.ExecuteReader())    // won't compile;command is now out of scope

@HaloFour,

... but at the cost of much unnecessary verbosity.

Clarity is never unnecessary verbosity. As you have pointed out to me on a number of occasions, it's never worth saving a few lines of code if the result is less clarity.

HaloFour commented 6 years ago

@DavidArno

Oh, that would be nasty. That suddenly brings all the issues around ; that C suffers from with while etc when a ; is accidentally placed after it:

Finding a syntax which better differentiates between the two forms would be a part of the design process. I agree that the feature should be made resilient to those kinds of issues. The syntax I've suggested is illustrative only.

Clarity is never unnecessary verbosity. As you have pointed out to me on a number of occasions, it's never worth saving a few lines of code if the result is less clarity.

Clarity is very subjective. The C# language does not require defining scope for every declared variable and it's pretty clear that the scope is inherited from the current block. In many non-GC languages a deterministic destruction/disposal when a variable goes out of scope is very common.

mikedn commented 6 years ago

Oh, that would be nasty. That suddenly brings all the issues around ; that C suffers from with while etc when a ; is accidentally placed after it:

I do not understand your example and the C "issue" it is supposed to illustrate.

DavidArno commented 6 years ago

@mikedn,

using (var connection = OpenConnection(new SqlConnection(connectionString)))
using (var command = SetupCommand(connection.CreateCommand()))
using (var reader = command.ExecuteReader())
DoSomething(reader);

is equivalent to:

using (var connection = OpenConnection(new SqlConnection(connectionString)))
{
    using (var command = SetupCommand(connection.CreateCommand()))
    {
        using (var reader = command.ExecuteReader())
        {
            DoSomething(reader);
        }
    }
}

So if changed to:

using (var connection = OpenConnection(new SqlConnection(connectionString)))
using (var command = SetupCommand(connection.CreateCommand())); // <- add semicolon
using (var reader = command.ExecuteReader())
DoSomething(reader);

It becomes equivalent to:

using (var connection = OpenConnection(new SqlConnection(connectionString)))
{
    using (var command = SetupCommand(connection.CreateCommand())) {}
}
using (var reader = command.ExecuteReader()) // command is out of scope here
{
    DoSomething(reader);
}

Adding or removing a ; would completely change the implied block structure of the code, leading to at least hard to figure out compiler errors, if not runtime errors.

This same problem used to plague me when writing C. Accidentally put a semicolon in the wrong place and things would go wrong:

while (true); // <- semicolon here
{
    // this block is unrelated to the while and is unreachable
}
mikedn commented 6 years ago

Hmm?!

using (var connection = OpenConnection(new SqlConnection(connectionString)))
using (var command = SetupCommand(connection.CreateCommand())); // <- add semicolon
using (var reader = command.ExecuteReader())
DoSomething(reader);

That's not how it's supposed to look like if this proposal is implemented. It's supposed to look like

using var connection = OpenConnection(new SqlConnection(connectionString));
using var command = SetupCommand(connection.CreateCommand());
using var reader = command.ExecuteReader();
DoSomething(reader);

The semicolon is required, like for every other variable declaration.

HaloFour commented 6 years ago

@DavidArno

It sounds like you're arguing against the current syntax of using, for which I'm afraid you're a little too late.

This proposal wouldn't have the same issue because the scope would not be tied to an embedded statement immediately following the statement. However, there is valid concern that the syntax I've suggested is too similar to the existing syntax since the only difference is the lack of parenthesis and the requirement of a semicolon. That's why @TyOverby suggested a different syntax employing a new keyword, using scope.

DavidArno commented 6 years ago

@HaloFour,

I had mistakenly thought that using ((var x = new Disposable())) was valid and thus that () could still be wrapped around your new syntax. This isn't the case. So that removes my fear over random ; affecting things. 😊

using scope or some such would help to distinguish them better though.

alrz commented 6 years ago

sequence expressions + with clause?

using (var connection = new SqlConnection(connectionString))
using (var command = (connection.Open(); connection.CreateCommand() with { CommandText = sql }))
using (var reader = command.ExecuteReader())
{ .. }

oh no.

MkazemAkhgary commented 6 years ago

could using be applied to assignment only?

SqlConnection connection;
using connection = new ...; // connection becomes readonly afterwards

if not, then declaration could be implied by one keyword so instead of using var you type using and that declares new variable. so using x = ... would actually declare read only x. I'm not proposing this though.

ygc369 commented 6 years ago

@HaloFour @gafter Is this similar to https://github.com/dotnet/csharplang/issues/121?

ghord commented 6 years ago

Following code does not compile right now:

    goto asdf;
    using (File.Open("test", FileMode.Open))
    {
        asdf:;
    }

With CS0159: No such label 'asdf' within the scope of the goto statement.

I guess

    goto asdf;
    using File.Open("test");
    asdf:;

Should be disallowed too.

mikedn commented 6 years ago

using File.Open("test"); doesn't make sense in this context, it should be a variable declaration. So the question is what to do about:

goto asdf;
using var file = File.Open("test");
asdf:;

If code after asdf is trying to use file then you'd get a Use of unassigned variable error so it's all good probably.

ghord commented 6 years ago

@mikedn Yet there are cases where using statement without variable declaration makes sense. I don't see why single-line using without variable declaration should be disallowed.

mikedn commented 6 years ago

Yet there are cases where using statement without variable declaration makes sense. I don't see why single-line using without variable declaration should be disallowed.

Well, it is useful sometimes but IMO not often enough to warrant supporting this syntax. Besides, it seems natural to extend scoping so that "at the end of a scope objects stored in "using" variables declared in that scope are disposed". It's not so clear what's going on with using File.Open as there's no variable associated with it and thus it doesn't have anything obvious to do with scoping.

Perhaps using var _ = File.Open("test"); would be sufficient?

ghord commented 6 years ago

Personally, code like this makes this feature most appealing to me. Previously, you had to introduce complicated scopes. Don't even get me started on mixing try and catch here - that would be another scope.

   void Method()
   {
       logger.Log("Text");
       using logger.Indent();
       ...
       logger.Log("Indented text");
       ...
       using logger.Indent();
       ...
       logger.Log("Twice-indented text");
       ...
    }

Adding var _ everywhere would look even worse than normal scoped using.

MkazemAkhgary commented 6 years ago

Yet there are cases where using statement without variable declaration makes sense

use normal syntax in that case. using var _ = is very agonizing.

mikedn commented 6 years ago

Personally, code like this makes this feature most appealing to me. Previously, you had to introduce complicated scopes. Don't even get me started on mixing try and catch here - that would be another scope.

Ha ha, it makes sense in your example. Though personally I'm still not convinced about its usefulness. Perhaps that's because last time I encountered code with so much logging ceremony I did something that I considered to be immensely useful in making the code more readable - I deleted it all 😁.

MkazemAkhgary commented 6 years ago

@ghord I don't know what library you are using, but is there any method like Indent(int count) ? in that case you can prevent nested blocks.

by the way designers had in mind to make indentation implicit using this trick. I'm pretty sure if this syntax (single line using) was possible on time they developed it, they would've thought of something better to overcome with that issue.

so really they wanted to take advantage of blocks and make indentation implicit, but you want to also omit blocks.

alrz commented 6 years ago

Re: using x = ... syntax, and generally using expression;

I want to mention #218 which may want to use the exact same syntax (if ever happens). On the other hand, using var x = ... (or using scoped for that matter), is not ambiguous with that syntax.

Though the result might be confusing, so perhaps using scoped is a better choice here.

gulshan commented 6 years ago

if a new keyword scoped is introduced, why not just use that without using? That will reduce confusion I think.

public static void CopyFile(string from, string into) 
{
    scoped var fromFile = File.Open(from, FileMode.Open, FileAccess.Read);
    scoped var toFile = File.Open(into, FileMode.Create, FileAccess.Write);
    // ... Perform actual file copy

    // Both FileStream objects are disposed at the end of the enclosing scope, 
    // in this case, the method declaration.
}
TyOverby commented 6 years ago

@gulshan: by piggybacking off of the using keyword, it is way easier to amend the grammar.

TonyValenti commented 6 years ago

I think I would rather see C# keep track of whether a disposable object has leaked its scope and if it hasn't, have it automatically dispose it.

VisualMelon commented 6 years ago

@TonyValenti that isn't feasible/dependable: an IDisposable may be passed to another method which can do anything it likes with it. Some concept of 'ownership' would be required for that kind of analysis to work at compile time. (In a way, using says I own this, I'll deal with it, but it doesn't stop others trying to deal with it first)

HaloFour commented 6 years ago

@TonyValenti @VisualMelon

Not to mention there are valid reasons for not wanting to dispose a resource, so having the language automatically do it would prevent valid scenarios and potentially break a lot of existing code.

Richiban commented 6 years ago

I really like this feature; it means that disposables can work more how they do in other languages, including F#.

One thing that's worth pointing out is that it seems this new syntax will functionally completely replace the old syntax. Not a bad thing, but worth mentioning.

In old-style using blocks the advantage over this feature is that the lifetime of the disposable object can be different from, say, the method scope:

void Main()
{
    using (var a = new MyDisposable())
    using (var b = new MyDisposable())
    using (var c = new MyDisposable())
    {
        // Some code
    }

    // Some other code where a, b and c have been disposed
}

But that will still be possible with the new syntax, simply by opening a new scope and putting using variable declarations inside it:

void Main()
{
    {
        using var a = new MyDisposable();
        using var b = new MyDisposable();
        using var c = new MyDisposable();   

        // Some code     
    }

    // Some other code where a, b and c have been disposed
}

Granted, opening custom scopes like that is not very common in C# today, but it's perfectly legal.

gafter commented 5 years ago

There is a question about what happens with explicit control-flow (e.g. goto) into or out of the "scope" of this construct. I propose...

Neme12 commented 5 years ago

I'm not really a fan of this. Most of the time in practice, the disposal can happen much earlier than at the end of the scope and I feel like this will just turn people on the wrong path of declaring a "using" local and having its lifetime extended to the end of the scope (that could potentially also include awaits) just to get the nicer syntax. At least most of the using statements I've written only had one statement inside - often with a few other statement after that.

Even if you start in this perfect sweetspot of needing the disposal at the end of the scope, let's say you later want to add more statements at the end (which don't need the resource and prevent disposal). Are you going to change the using variable into a using statement (or wrap the existing code in a new block)? No, most people would not.

Here's an example of the sort of C++ I've had to write way too many times:

statement();

{
    std::lock_guard<std::mutex> lock(mutex);
    statement();
    statement();
}

statement();

Is this really so nice? I essentially have to introduce a block just for this variable and this is neither pretty or easy to understand at first glance. It's also too easy to see these braces as redundant. I always squirm when I have to write code like this in C++ and miss the explicitness of a using scope in C#.

Now maybe I'm missing the point since a using statement will still be available. But as I've said, I think it will be too convenient to simply bolt using onto a variable and forget thinking about the scope where you actually need the resource and reducing it as much as possible.

There is also the fact that many times you do not actually need a variable for the resource, and in that case this feature would not help you either. All combined, I don't like how this introduces syntax for just 1 special case - 1. there is a variable and 2. the lifetime should exactly match the rest of the scope.

hoensr commented 5 years ago

(Reposting my comment on that other thread; many thanks to HaloFour for pointing me here!)

This is what I was looking for today. My code is something like this:

List<MyResults> results;
using (var sheet = new MyExcelSheet(excelFilePath))
{
    results = myExcelReader.ReadResults(sheet);
}

I would like to be able to shorten it to

var results = myExcelReader.ReadResults(using new MyExcelSheet(excelFilePath));

(MyExcelSheet is a thin facade around EPPlus, implementing an interface so I can unit-test the Excel reader.) Note that in the long code, I need to give the explicit type of the variable results, whereas in the short code, I can use var.

PS: Since I use an anonymous variable, I would expect the object to be disposed at the ;.

MouseProducedGames commented 5 years ago

I would argue that "using (var disposable new MyDisposable());" should be flagged as an error, if the original syntax is used. If what you mean is a one-line using, you should instead use "new MyDisposable().Dispose()" or "using (var disposable new MyDisposable) { disposable.DoTheThing(); }" (IMO).

scalablecory commented 5 years ago

Been quite a while since this issue was made back on the Rosyln project. Would love to know where this is with the official team, given the number of C# versions we've seen since then.

gafter commented 5 years ago

This has been implemented and is quite likely to be included in C# 8.

HaloFour commented 5 years ago

@gafter

Is this implemented in a particular branch? I'd love to play with it on SharpLab.

jnm2 commented 5 years ago

@HaloFour Fyi https://github.com/ashmind/SharpLab/issues/344

ashmind commented 5 years ago

@HaloFour https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQQHYAsoYMYQAmqcyAlhgOYA+AAgAwAEtAjANwCwAULQMzMAmRgGFGAb26MpzfrQAsjALIAKAJTjpmxpK1SGjAG5QEjKIwC8jDBADujACJlkABwD2yKMAA2ENZy6aAL46UiGMzghkRjAQgg5Obh7eECCMAJKOLu6ePuJhmnzMCpmJvupiwQHSlYFAA===

popcatalin81 commented 5 years ago

Uh, the invasion of these C++ idioms is making C# uglier and uglier by day ...

DavidArno commented 5 years ago

@ashmind,

Thanks for the link. Looks like it has bugs still, though (the compiler that is, not your super useful website).

popcatalin81 commented 5 years ago

I really dislike this feature due to the potential for subtle bugs in code with multiple disposables, loops and possibly async. I don't think the readability improvements actually are readability improvements. The code is more indented but not more readable. It does not convey the semantics more easily.

This change brings less indentation but also less "semantic" readability.

Thaina commented 5 years ago

@popcatalin81 If you intentionally drop parentheses around using then you intentionally use this feature. You just intentionally rely on parent block scope to dispose your things

It was readability improvements that make anyone immediately know that "oh so it's not be disposed somewhere in the middle, it just dispose at the end of this scope" unlike the normal syntax that can potentially end before the end of scope so it take more attention to know that where it would end, at the end or prematurely somewhere along the lines of code. Not to mention the pyramid of indentation block stack

popcatalin81 commented 5 years ago

@Thaina

ii f you intentionally drop parentheses around using then you intentionally use this feature.

No, not for all cases see below.

unlike the normal syntax that can potentially end before the end of scope so it takes more attention to know that where it would end

Yes, subtle bugs potentially arise from:

Thaina commented 5 years ago

@popcatalin81 Yes that is the case

I said

If you intentionally drop parentheses around using then you intentionally use this feature.

And what you have done is, you have one line block of using that work fine and as intended. But you then suddenly intentionally drop parentheses to have it live longer in that scope. This is not subtle. It is obvious bug

This feature does not try to replace the old feature. Each of both has it's own purpose and usage. You make a block of using to have it live in specific scope. You use implicit using to have it live on parent scope. Replacing it surely introduce a bug. In the same sense that you can't just replace do{}while(); with while(){} syntax directly

popcatalin81 commented 5 years ago

@Thaina Thank you for the lecture. but your points were not debated by me.

My points are:

Thaina commented 5 years ago

all in all

This feature is not replacement to using block

jaredpar commented 5 years ago

@popcatalin81

Relacing this with using var will change dispose order c, b, a (untimely after all calls)

Sure. If you use this feature incorrectly it will behave incorrectly. That is true of literally every feature though.

popcatalin81 commented 5 years ago

@jaredpar Most developers (I'd say 98%+) have never read and never will read the C# spec, they will just do things like using async void everywhere (seen it happen) without really getting the semantics. .

Sure. If you use this feature incorrectly it will behave incorrectly. That is true of literally every feature though.

Is multiple inheritance good or were just people using it wrong? Is fallthrough case statement in C good or were people just using incorrectly and that's why its' the source of so many bugs?

My argument is that people will use this feature wrong but in "good faith" ... semantics difference is not enough of a difference for most users: IE: async Task vs async void.

I distinctly remember reading or hearing Anders talk about using statement in .Net 1.1 timeframe an why RAII was considered for exclusion together with other C++ derived features. If only I could find it ... after almost 15 years buried in the internet ....

svick commented 5 years ago

@popcatalin81 I agree with the general argument that features that are easy to misuse are bad.

What I don't quite follow is: What exactly makes you think this particular feature would be easy to misuse? Why do you think many people will incorrectly manually "prettify" their code like this? (And I say "manually", because I'm fairly confident tools like VS or ReSharper won't offer refactorings that have a decent chance of being incorrect.)

iam3yal commented 5 years ago

@popcatalin81

Most developers (I'd say 98%+) have never read and never will read the C# spec, they will just do things like using async void everywhere (seen it happen) without really getting the semantics. .

Most developers won't read the spec but most professionalists, educators and such that responsible for creating the content such as docs, articles, books, videos and courses which the majority of developers are guided by probably do and I think and tend to believe that most sources would instruct developers to do the right thing in most cases as opposed to the wrong thing so I wonder how you reached the conclusion that most developers do it wrong, more importantly, I have hard time to believe that most developers use arbitrary sequence of keywords for no reason without questioning what things do and go with "oh! it just works, I'll use it.".