-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Champion "Implicitly scoped using statement" (16.3, Core 3) #1174
Comments
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.
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;
}
}
|
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. |
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 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. |
Kotlin has 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);
}
}
} |
Oh, that would be nasty. That suddenly brings all the issues around 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
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. |
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 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. |
I do not understand your example and the C "issue" it is supposed to illustrate. |
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 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
} |
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. |
It sounds like you're arguing against the current syntax of 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, |
I had mistakenly thought that
|
sequence expressions + using (var connection = new SqlConnection(connectionString))
using (var command = (connection.Open(); connection.CreateCommand() with { CommandText = sql }))
using (var reader = command.ExecuteReader())
{ .. } oh no. |
could using be applied to assignment only?
if not, then declaration could be implied by one keyword so instead of |
Following code does not compile right now: goto asdf;
using (File.Open("test", FileMode.Open))
{
asdf:;
} With I guess goto asdf;
using File.Open("test");
asdf:; Should be disallowed too. |
goto asdf;
using var file = File.Open("test");
asdf:; If code after |
@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. |
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 Perhaps |
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 void Method()
{
logger.Log("Text");
using logger.Indent();
...
logger.Log("Indented text");
...
using logger.Indent();
...
logger.Log("Twice-indented text");
...
} Adding |
use normal syntax in that case. |
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 😁. |
@ghord I don't know what library you are using, but is there any method like 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. |
Re: I want to mention #218 which may want to use the exact same syntax (if ever happens). On the other hand, Though the result might be confusing, so perhaps |
if a new keyword 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.
} |
@gulshan: by piggybacking off of the |
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. |
@TonyValenti that isn't feasible/dependable: an |
And yet, Linq was still good to do. The presence of companies willing to ban over language features does not mean those language features should not be done. After all, i'm sure for every language feature, you could find some codebase that has banned it. But we obviously still work on new language features despite that. |
Indeed, i made this very mistake in Roslyn in the Sqlite layer. I made an async call while a disposable resource was being held. Should we ban async+disposal+usings? No. We just need to be careful when combining these things. |
That's exactly the point I was trying to make: It's too easy to combine |
So... the don't combine them. I mean, we do this all the time in other arenas. in places where allocation costs are problematic, we don't allow allocation-heavy features. In places where dispatch costs are problematic, we don't use virtual/interface dispatch. etc. etc. Not every language feature is appropriate in every codebase (or at any point in any particular codebase). We have rules all over hte place for things you should be careful of because they might cause a problem. As i pointed out, async+disposal is already a problem, and it doesn't seem to be any worse here. In both cases you have to think about what's going on and set things up properly. So nothing really changes. As i pointed out, not having "using var" didn't save me. The problem exists independently of the new language construct. |
Don't worry I won't do it myself. But I'll the one called to fix weird bugs because some dev has used a feature without understanding the semantics. But that's not a problem really. I'm a consultant, I make money when others screw up ... (That's why I'm skeptic about this feature, perhaps even biased) |
That's an issue with all language features :) Don't understand 'async'? Then you're going to have weird bugs. Don't understand lambdas, or generics, or 'ref', or operators, or structs, or inheritance, or stacks, or memory, or etc. etc. etc.? Then you're going to have weird bugs. We don't stop moving the language forward just because people there exist people who use the language without spending hte time to understand what's going on. :) Heck, if you don't understand 'using' today, you're going to have issues. But life moves on and the language still improves so that people who do learn and understand can have a better experience :) |
Yes. But in this case, there simply is a better option: |
@popcatalin81 Wrapping the whole function with |
The usual phrase is https://en.wikipedia.org/wiki/Pyramid_of_doom_(programming) |
@Thaina so you prefer a hidden Pyramid? opposed to a visible one? It's easier to understand an implied pyramid than an explicit one?
So let's see:
|
@popcatalin81 It just a perspective. If you call it hidden pyramid then In my perspective it not a hidden pyramid because you shove all responsible stack to the parent scope. In intuitively the same as It like you have array as stack. You don't need to really stack things in memory, it just abstraction to the LIFO process. And this make consistency for both code and memory Hidden pyramid is not pyramid. Flatten pyramid is not pyramid. Because what make pyramid is its shape, not because the order of operation it got |
@popcatalin81 And I think you misunderstand the most important point. That we don't deprecate or stop using the I would still using the In fact I think it would be common to have using(var something = LoadSomeThing())
{
// Do something with something
if(needAnotherThing)
{
using var anotherThing = LoadAnotherThing(); // can avoid pyramid
// Do anything that need both something and anotherThing
} // anotherThing dispose here
// do some more thing relate to something but don't need anotherThing
}
// do some more thing not relate to something or anotherThing Like this |
It seems to me that this discussion is starting to repeat the same points over and over again. I've seen other discussions in this repo where one of the parties has assumed that "disagreement" equals "not understanding", resulting in a tremendous amount of loud repetition. I fear this issue might be heading in the same direction. I fervently hope it's not, and I would like to derail that if possible. @popcatalin81 I'm just a random developer, participating here just because I find the process of language design endlessly fascinating. Please be assured that I have understood and appreciated the points you're making - and if I do, I'm pretty sure the LDM folks who make the decisions do as well. |
I hope that whatever syntax and semantics this proposal evolves into, will take into account the possibility of future evolution covering more than just local variables - and in particular, implicitly disposed fields / implicit implementation of IDispose (in the vein of C++/CLI destructors). Note, I'm not saying that it should be a part of this proposal. Only that if such a direction is later taken, whatever is done here would serve as a natural basis - so it's best to pick an accommodating syntax today, so as to not have regrets over it in a few years. With that in mind, and in the spirit of minor bikeshedding, how about Here's the code snippet from the public static void CopyFile(string from, string into)
{
owned var fromFile = File.Open(from, FileMode.Open, FileAccess.Read);
owned 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.
} (And then perhaps Such syntax would also extend naturally to ownership of objects referenced by another object via its fields, if some future proposal decides to go there, e.g.: owner class Image {
private owned FileStream stream;
...
} |
FWIW, we have had this feature in F# for years, since v3, I believe, happy to have it, and see no troubles from it at all! The syntax exactly follows that of the binding, only with a different keyword for scoped disposal (
I am not going to quote examples to keep flame low, but I cannot help remarking that I've read many objections here that sound no better than patronizing. Please. C# is not intended as language to teach kindergartners how to program. A language that has no features that can potentially be misused is either not Turing-complete, or Haskell. Cannot put it into words better than @CyrusNajmabadi did:
Amen to that. Once you've learnt that And "some devs" misuse the assignment operator too², but I still want it in the language, if you do not mind :-) ¹ confusingly, not to be confused with the |
|
But it's not legal to use inside of a method scope. In fact, the compiler doesn't even consider it being a namespace/type alias when used in a method scope, so there's no ambiguity here. |
I'm curious as to how that would interact with the scripting form of C#, where this doesn't necessarily apply. |
This is great feature, but is there anything similar planned for namespaces? Most if not all .cs files will have this wrapping namespace in them. namespace MyNameSpace
{
public class MyClass
{
}
} It seems very redundant because most of the files will have single namespace just wrapping everything. It would be great to able to do this this instead namespace MyNameSpace;
public class MyClass
{
} |
The documentation seems to imply that this (multi-declaration implicitly typed variables) is possible:
But it doesn't look to be possible with SharpLab. Is it supposed to be possible, and wouldn't it then clash with disallowing multi-declaration var generally? I'd file this as a bug on the documentation but the proposal says the same thing. |
@JesperTreetop |
Should #114 just mention this issue instead of both being put on the project board? |
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 theusing
statement; the resource is freed at the end of the block.See #114 and #114 (comment)
LDM history:
The text was updated successfully, but these errors were encountered: