Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Can't use C# 8.0 using declaration with a discard #2235

Closed
RayKoopa opened this issue Feb 16, 2019 · 96 comments
Closed

Can't use C# 8.0 using declaration with a discard #2235

RayKoopa opened this issue Feb 16, 2019 · 96 comments
Assignees
Milestone

Comments

@RayKoopa
Copy link

RayKoopa commented Feb 16, 2019

C# 8.0 adds the possibility to declare using variables without a corresponding using block, discarding the variable as soon as the scope in which it was declared is left.

The official example shows it with assigning the using variable to an actually named variable as you would typically do:

// Pre C# 8.0 code:
static void Main(string[] args)
{
    using (var options = Parse(args))
    {
        if (options["verbose"]) { WriteLine("Logging..."); }
        ...
    } // options disposed here
}

// Becomes:
static void Main(string[] args)
{
    using var options = Parse(args);
    if (options["verbose"]) { WriteLine("Logging..."); }
    ...
} // options disposed here

However, this does not seem to work if I don't actually need the variable being disposed. My use case is a temporary Seek object which remembers the current position in a stream when created, and reverts to that position upon being disposed. It is used like this:

byte[] ReadDataAtFollowingOffset(ExtendedBinaryReader reader)
{
    int offset = reader.ReadInt32();
    int length = reader.ReadInt32();
    using (reader.TemporarySeek(offset, SeekOrigin.Begin))
    {
        return reader.ReadBytes(length);
    }
}

I tried to rewrite this in C# 8.0:

byte[] ReadDataAtFollowingOffset(ExtendedBinaryReader reader)
{
    int offset = reader.ReadInt32();
    int length = reader.ReadInt32();
    using reader.TemporarySeek(offset, SeekOrigin.Begin);
    return reader.ReadBytes(length);
}

However, this yields CS0118 'reader' is a variable but is used like a type. A bit stumped here already, I then tried storing the Seek instance in a discard variable at least:

byte[] ReadDataAtFollowingOffset(ExtendedBinaryReader reader)
{
    int offset = reader.ReadInt32();
    int length = reader.ReadInt32();
    using _ = reader.TemporarySeek(offset, SeekOrigin.Begin);
    return reader.ReadBytes(length);
}

But this now yields CS0246 The type or namespace name '_' could not be found (are you missing a blahblahblah...?). More stumped, I then became even more explicit:

byte[] ReadDataAtFollowingOffset(ExtendedBinaryReader reader)
{
    int offset = reader.ReadInt32();
    int length = reader.ReadInt32();
    using Seek _ = reader.TemporarySeek(offset, SeekOrigin.Begin);
    return reader.ReadBytes(length);
}

While this compiles, it feels very odd to me. Jokes on Visual Studio 2019 trying to break my code now too and forgetting to use using at all:

image

Is this a known issue, done on purpose, or an oversight in the feature?

@sharwell
Copy link
Member

@mavasani for the code fix bug

@mavasani
Copy link

We should probably just consider a using variable declaration as an effective read of the underlying variable, so the assigned value is not deemed as unused. I can send a PR for it tomorrow.

@mavasani
Copy link

Tagging @chsienki @AlekseyTs and @333fred. The false positive from the IDE unused value assignment analyzer is due to missing IOperation support and hence missing ControlFlowGraph support for using variable declarations #dotnet/roslyn/issues/32100. Analyzer is performing flow analysis on top of CFG to flag assignments whose target is never read or referenced. CFG for this code has a local variable declaration wrapped with an Operation.None and there is no subsequent local reference operation in the CFG. Implementing CFG support for this node would ensure there is an implicitly generated ILocalReferenceOperation for the compiler generated dispose invocation, which would remove the false positive from this analyzer and hence no suggestion to use discard would be offered.

@mavasani
Copy link

Actually, the analyzer/code fix to recommend using discard should be fixed in VS2019 Preview4 with
dotnet/roslyn#33036 - the first commit in that PR avoids flagging this case. I will verify it with latest dogfood build tomorrow.

@mavasani
Copy link

Confirmed that unused value diagnostic + use discard code fix is offered in VS2019 Preview2 but not in latest VS2019 dogfood builds, and should be fixed in upcoming Preview4 release.

@chsienki
Copy link
Member

@RayKoopa the new using declaration feature only works for explicit variable declarations, you can't apply using to arbitrary expressions, even if that expression results in an IDisposable. It is when the declared variable goes out of scope that the object it's referencing is disposed.

Essentially

using (var x = ...) 
{
  x.DoSomething();
}

can be re-written as

using var x = ...
x.DoSomething();

but

using(SomeExpression())
{
}

Can't be directly translated, as there's no declaration in the statement. You can however, assign it to a variable and it'll work, even if you don't use the variable for anything else. As you saw some of the tooling in the previews is a little out of sync with the newest language features at the moment, hence the unused diagnostic in VS, but the compiler will correctly track that it's actually used.

@RayKoopa
Copy link
Author

Thanks for letting me know @chsienki! I noticed I can even go as far as

using var _ = ...

to "pseudo" discard the variable here, and it would still compile. That's really sufficient, just wanted to make sure it was done on purpose like this and is not something like an oversight.

@StephenCleary
Copy link

Can't be directly translated, as there's no declaration in the statement.

This would be quite useful. I'd love to be able to use using declarations with discards.

@notanaverageman
Copy link

There is also the problem of multiple discards. For now the only way is to add more underscores (actually giving different names to the variables as they are not really discards):

using var _ = ...
using var __ = ...
using var ___ = ...
...

@reflectronic
Copy link

This could be useful for RAII-style lock acquisition; for example, one would be able to do

void M()
{
	using _ = myLock.Acquire() // rest of scope is protected by lock
    // ...
}

instead of having to implement something like #2451.

@jnm2
Copy link
Contributor

jnm2 commented Oct 7, 2019

This also makes it painful to use .ConfigureAwait(false) on IAsyncDisposable:

var foo = new Foo(); // Foo : IAsyncDisposable
await using var unused1 = foo.ConfigureAwait(false);

// ...

In order to avoid declaring unused variables, you must go back to a using statement:

var foo = new Foo(); // Foo : IAsyncDisposable
await using (foo.ConfigureAwait(false))
{
   // ...
}

@TehPers
Copy link

TehPers commented Oct 21, 2019

What about this syntax for discarding the value?

using myLock.Acquire();
// lock-protected code

// Also, don't forget about IAsyncDisposable!
await using myLock.AcquireAsync();

@quinmars
Copy link

@TehPers, the problem is that:

using (myLock.Acquire());

is already valid C# code and means something slightly different. Namely dispose the resource right now and not at the end of the outer block.

@yinyue200
Copy link

up

@jackalvrus
Copy link

jackalvrus commented Apr 15, 2020

Our use case for this is Serilog's PushProperty method. It would be nice if we could write something like:

using _ = LogContext.PushProperty("a", a);
using _ = LogContext.PushProperty("b", b);
using _ = LogContext.PushProperty("c", c);

// the function body

instead of:

using (LogContext.PushProperty("a", a))
using (LogContext.PushProperty("b", b))
using (LogContext.PushProperty("c", c))
{
    // the function body
}

@333fred
Copy link
Member

333fred commented Apr 15, 2020

I'm interested in championing this. It's an annoying inconsistency that I've wanted to fix for a while now.

@chsienki
Copy link
Member

chsienki commented Apr 15, 2020

@333fred I thought this was an open question, but @jcouv pointed out we already approved in LDM (dotnet/roslyn#43292 (comment))

@333fred
Copy link
Member

333fred commented Apr 15, 2020

Well, we still need to triage it. I imagine we'll put it in any time, but we need to decide.

@jcouv
Copy link
Member

jcouv commented Apr 15, 2020

Since there is no language issue, I'll go ahead and close the csharplang issue. We'll track the bug fix with roslyn issue (dotnet/roslyn#43292). Thanks

@jcouv jcouv closed this as completed Apr 15, 2020
@333fred
Copy link
Member

333fred commented Apr 15, 2020

This is still a language issue.

@333fred 333fred reopened this Apr 15, 2020
@jcouv
Copy link
Member

jcouv commented Apr 15, 2020

Sorry, confused this with using var _ = ... scenario. Makes sense to keep this open.

@sharwell
Copy link
Member

sharwell commented Jan 4, 2022

Technically it is possible to allow this:

using var _ = ...;
using var _ = ...;

The result would be similar to how the lambda function (_) => _.Value is fine (single parameter named _), but you can also define a lambda function (_, _) => false with two discards and no named parameters. Since two using var _ statements were never allowed in the same block before, we're converting erroneous code to valid code and not actually changing the meaning of previously valid code.

That said, the following would be more consistent with discards in general:

using _ = ...;
using _ = ...;

This was also not valid code in the past, but could become valid code in the future.

@CyrusNajmabadi
Copy link
Member

so we can't really change this, either.

We can change this. Code that is in error today can be made legal in the future :-)

@andre-ss6
Copy link

I took you at your word but having been spurred by the commenters above and having checked this just now, it's not an error - it creates a variable _; so we can't really change this, either.

@TahirAhmadov They were talking about two using var _, which is an error (two variables with the same name).

That said, the following would be more consistent with discards in general:

using _ = ...;
using _ = ...;

This was also not valid code in the past, but could become valid code in the future.

@sharwell But this is already valid code today (using alias) and would be ambiguous with the current proposal because of top-level statements.

@sab39
Copy link

sab39 commented Jan 4, 2022

Doesn't the compiler know that it's inside a method and is therefore a dispose using, as opposed to outside a method in which case it's a namespace using?

That would make the compiler context sensitive.

However, even ignoring that, statements are legal at the top level in c#. So it's a true ambiguity there period.

Surely there are plenty of situations in the language already where what's legal or not depends on the context, including quite a lot of special cases about when _ is an identifier versus being a discard, so adding another doesn't seem like a huge stretch.

Would it be so terrible if the standalone version of disposable-using statements were allowed in general, but disallowed in top level statement contexts? After all, in that context the behavior might be counterintuitive or undesirable anyway - one scenario for top level statements would be something like a REPL, which doesn't have a convenient way to close the scope to dispose the object at all.

@CyrusNajmabadi
Copy link
Member

Would it be so terrible if the standalone version of disposable-using statements were allowed in general, but disallowed in top level statement contexts?

I would consider that terrible.

@sab39
Copy link

sab39 commented Jan 4, 2022

Would it be so terrible if the standalone version of disposable-using statements were allowed in general, but disallowed in top level statement contexts?

I would consider that terrible.

Care to elaborate? I can't think of a good reason why you'd want one in a top-level statement context.

Edited to add: to me, a rule that "disposable-using statements are invalid if you aren't inside a block" is no different than "continue statements are invalid if you aren't inside a loop". continue means "jump to the next iteration of the containing loop"; using means "dispose this at the end of the containing block". Even without the syntactic ambiguity issue, I'd argue that it would be confusing not to forbid this kind of using in top level statements!

@cytoph
Copy link

cytoph commented Jul 28, 2022

I think using myClass.GetDisposable(); would be nice, but if it means that much consideration (and supposedly problems), than in my opinion, it should just be left out of the discussion for now and be re-iterated at a later time.
Just implement the explicit discard syntax (using _ = myClass.GetDisposable();) for now (because that should not cause any ambiguities or problems with existing code), so there's at least no reason anymore we have to use explicit variable names or something like _, __, ___, ____ on multiple uses.
Isn't that something we could all agree upon? 🤔

@CyrusNajmabadi
Copy link
Member

using _ = A.B; is already legal syntax for a using alias. So we rush breaking code by now interpreting that to be a using + discard. Something we'll have to be very careful about.

@airbreather
Copy link

using _ = A.B; is already legal syntax for a using alias

oof... completely out of curiosity (because I know there's no going back), how much easier would this be to resolve if we didn't have top-level statements?

@cytoph
Copy link

cytoph commented Jul 28, 2022

using _ = A.B; is already legal syntax for a using alias. So we rush breaking code by now interpreting that to be a using + discard. Something we'll have to be very careful about.

Oh god, you're right....... 🙈 Why did anyone ever think it would be a good idea to allow a single underscore as a variable name? 🙈😅 But yeah, that's of course enough of reason, that it won't work.

using _ = A.B; is already legal syntax for a using alias

oof... completely out of curiosity (because I know there's no going back), how much easier would this be to resolve if we didn't have top-level statements?

I guess it'd be so easy that it even would have been implemented by now. Because usings inside methods (or similar scoped members) do not pose any ambiguity as far as I know. Considering this topic I'm kind of surprised there aren't more problems with top-level statements.

Nonetheless:
Would simply disallowing using declarations with discard in top-level statements be something that could be achieved?

@andre-ss6
Copy link

My humble based on no data whatsoever opinion: using var ____ = FourthUsingPleaseEndMySuffering(); is enough a big of an issue to break whoever uses a single underscore as an alias to a type.

@CyrusNajmabadi
Copy link
Member

I simply to _1, _2, _3` and so on. It's still clearly discarded, and there's no variadic width to the identifier names.

@CyrusNajmabadi
Copy link
Member

how much easier would this be to resolve if we didn't have top-level statements?

There would be no conflict at that point. But it would also prevent local using aliases if we ever wanted that.

@jackalvrus
Copy link

I think the concern over top-level statements is not justified.

First off, using clauses must be at the top of the file. The following fails with CS1529:

Console.WriteLine("Hello, World!");
using _ = System;

So the only time there is any ambiguity is when a using expression with discard is desired as the very first line of the top-level code (after the usings). C# already handles situations where the code is ambiguous. This should be no different. I would be perfectly happy with being required to clarify the code by using the old-school using (A.B) {} syntax in that one case.

That said, I haven't been able to figure out any way to make it so that an identifier in the form A.B can exist as both a namespace and an expression that returns an IDisposable without triggering other errors. The only way I can think of to create that scenario is to create class A with static property B in the global namespace, then create namespace A with class B. That code fails with CS0101.

So there is potentially no ambiguity at all.

Can anyone think of a way to get something like A.B to exist simultaneously as both a namespace and an expression in the same context?

@airbreather
Copy link

airbreather commented Jul 29, 2022

So there is potentially no ambiguity at all.

Can anyone think of a way to get something like A.B to exist simultaneously as both a namespace and an expression in the same context?

From my understanding, it's not that it's impossible to resolve the apparent ambiguity (and even if a usage does happen to be ambiguous somehow, I think there's a general understanding that an error would be fine).

Rather, as I understand it, the issue here is that because a statement at the top of a file of the form using _ = A.B; could be interpreted either way depending on semantic context, parsing it becomes a lot more complex because you can no longer distinguish between the two extremely different possible meanings of this line at the syntax level.

So (again, as I understand it) if this were the kind of feature that would REALLY add a lot of value, then I'm sure the team would be able to overcome the issues and get something that works just perfectly (which is why I imagine this hasn't been outright closed as "rejected"). As the problem it solves is really just a minor inconvenience in the grand scheme of things, however, I'm not surprised that it has taken this long without a resolution.

@jackalvrus
Copy link

While I understand and applaud the push to reduce unnecessary ceremony, fixing this would improve my developer experience far more than top-level statements ever will.

@TahirAhmadov
Copy link

Probably a non starter, but changing the namespace using to import and alias using to alias, and leaving using to just do IDisposable, would alleviate all these ambiguities, in addition to being somewhat easier to read.

@CyrusNajmabadi
Copy link
Member

Yes. That would be a non-starter. That would also introduce its own set of issues as things like import x.y.z; and alias x = y; are already legal in c# today :)

@crozone
Copy link

crozone commented Feb 21, 2023

While I understand and applaud the push to reduce unnecessary ceremony, fixing this would improve my developer experience far more than top-level statements ever will.

Agreed, besides being next to useless for experienced developers, top level statements are already an ugly hack and come with a huge list of caveats which need to be considered when programming in that context. I don't think adding yet another caveat to that list of pitfalls will be much of an issue, for a feature that is hugely useful in actual real-life code.

@andre-ss6
Copy link

Let's introduce the using_ keyword. Simple 😅

@Charlieface
Copy link

I agree, allowing using _ = GetIDisposable(); would be hugely useful.

One option for the top-level statement problem might be to simply disallow them. It's not like anyone uses them in any realistic scenario 😀

Realiztically, we can just disallow _ as a using alias, with a breaking change. It's going to be a very rare person who has an underscore as an alias, whereas our current issue seems to be far more widespread.

The breaking change itself is not going to change the meaning of anyone's code apart from preventing it from compliing, unless they both used top-levels and an underscore alias, presumably vanishingly rare.

@MelGrubb
Copy link

Let's introduce the using_ keyword. Simple 😅

Nah, the new keyword should clearly be "notusing"

@aradalvand
Copy link
Contributor

aradalvand commented Oct 25, 2023

Hopefully the new breaking change warning proposal could allow us to sort this out once and for all. We should either enable:

using GetSomeDisposable();

(which currently, if there are parentheses around the expression, could mean this)
or

using _ = GetSomeDisposable();

(which currently could be a type alias)

The former is actually the ideal one; it would be perfect.

@AlanHauge-Interacoustics
Copy link

AlanHauge-Interacoustics commented Dec 12, 2023

The former is actually the ideal one; it would be perfect.

I think both should be available, in case the local rules allow unassigned object, and do not force the discard notation for non-used return values.
Perhaps the first one is regarded as a used return value.

@DavidArno
Copy link

Perhaps the first one is regarded as a used return value.

I like your thinking. Technically it is used as you say. I prefer the using _ = ... notation, but would be happy with either as both are an improvement on the using junk = ... that I currently have to use.

Agreed, besides being next to useless for experienced developers, top level statements are already an ugly hack...

After 10 months, I've finally stopped laughing at this statement. I love top level statements: they are one of the best features added to the language in recent years. But there again I wrote my first line of code only 42 years ago, so I guess I don't yet qualify as experienced 🤣

@aradalvand
Copy link
Contributor

Perhaps the first one is regarded as a used return value.

What does a "used return value" mean?

@AlanHauge-Interacoustics

Perhaps the first one is regarded as a used return value.

What does a "used return value" mean?

I mean, it is a warning (maybe from resharper), when you call a method, but do not use the return value for anything
This gives a warning:

something.ToString();

this does not:

string someText = something.ToString();

@Rekkonnect
Copy link
Contributor

Is it really a problem that we have an ambiguity in top-level statements? using having this many meanings is already confusing enough, and seeing a lot of them stacked up in the same indentation level will not be very straight-forward to understand:

using System;
using System.Collections.Generic;

Console.WriteLine("Beginning using");
using MyResource();

Console.WriteLine("Terminating using");

IDisposable MyResource()
{
    return null; // maybe something not null
}

Using the block using statement will surely solve this problem. It will be clearer that the using statement is about consuming and disposing an IDisposable that we do not intend to store anywhere.

Alternatively, if we want a universal solution that also covers top-level statements, I think that indicating that this is a using statement on an expression, we could adopt something like using var. using var will be a special kind of syntax that will be resolved as an unused variable of a disposable resource that we will dispose upon exiting the current scope. The syntax will be something like this:

using var MyResource();
using var x = AnotherResource();

The justification is that uninitialized variable declarations are not allowed in a scope-wide using statement. This using var clearly denotes that an IDisposable expression is being returned and used for disposal, but not stored in any variable.

using var expr; will be an alternative to the hypothetical using _ = expr; which I personally find harder to type and comprehend.

@dotnet dotnet locked and limited conversation to collaborators Nov 18, 2024
@333fred 333fred converted this issue into discussion #8605 Nov 18, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests