csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

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

Open RayKoopa opened this issue 5 years ago • 96 comments

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?

RayKoopa avatar Feb 16 '19 05:02 RayKoopa

@mavasani for the code fix bug

sharwell avatar Feb 16 '19 05:02 sharwell

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 avatar Feb 16 '19 07:02 mavasani

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 avatar Feb 16 '19 07:02 mavasani

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

mavasani avatar Feb 16 '19 08:02 mavasani

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.

mavasani avatar Feb 16 '19 15:02 mavasani

@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.

chsienki avatar Feb 16 '19 22:02 chsienki

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.

RayKoopa avatar Feb 16 '19 22:02 RayKoopa

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.

StephenCleary avatar Jul 06 '19 02:07 StephenCleary

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 ___ = ...
...

notanaverageman avatar Jul 17 '19 03:07 notanaverageman

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.

reflectronic avatar Oct 04 '19 20:10 reflectronic

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))
{
   // ...
}

jnm2 avatar Oct 07 '19 13:10 jnm2

What about this syntax for discarding the value?

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

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

TehPers avatar Oct 21 '19 08:10 TehPers

@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.

quinmars avatar Oct 21 '19 12:10 quinmars

up

yinyue200 avatar Jan 24 '20 10:01 yinyue200

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
}

jackalvrus avatar Apr 15 '20 14:04 jackalvrus

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

333fred avatar Apr 15 '20 15:04 333fred

@333fred I thought this was an open question, but @jcouv pointed out we already approved in LDM (https://github.com/dotnet/roslyn/issues/43292#issuecomment-613062739)

chsienki avatar Apr 15 '20 17:04 chsienki

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

333fred avatar Apr 15 '20 17:04 333fred

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

jcouv avatar Apr 15 '20 21:04 jcouv

This is still a language issue.

333fred avatar Apr 15 '20 22:04 333fred

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

jcouv avatar Apr 15 '20 22:04 jcouv

I guess I'm confused here. using var _ = should be valid and introduce a local called _ thats the behavior today, and applies wether its a using or not.

using _ = should be valid. From the LDM notes the wording suggests that was what was implied, but the actual written code used using var _

chsienki avatar Apr 15 '20 22:04 chsienki

My $0.02 on the syntax: to me it would make more sense as using _ = . This seems like it would be more consistent with the current discard behavior.

For example, this:

var _ = GetValue();
Console.WriteLine(_); // This works because _ is a declared variable

Versus this:

_ = GetValue();
Console.WriteLine(_); // This does not work because _ is a discard

jackalvrus avatar Apr 15 '20 22:04 jackalvrus

I thought var is required for out discards

daniel-white avatar Apr 15 '20 22:04 daniel-white

@chsienki using var _ = .. is a discard in C# 8 from language design perspective, but the compiler currently treats it as a local (compiler bug). Fred is keeping this issue open to propose supporting using _ = ... in a future version of C#.

@daniel-white out argument already allow short discards M(out _) (example). Deconstruction assignment do too (_, y) = (1, 2) (example).

jcouv avatar Apr 15 '20 22:04 jcouv

@jcouv great. I haven’t played much with them in 8. Love that it could make it shorter without var

daniel-white avatar Apr 15 '20 22:04 daniel-white

#2593 could also solve this issue.

andre-ss6 avatar Jun 04 '20 18:06 andre-ss6

I'm not sure if anyone is aware that using var _ = GetDisposable() resulted in my finalizers being called on the rvalue (they won't be if disposed properly).

/// <summary>
		/// Finalizes an instance of the <see cref="SemaphoreSlimContext"/> class.
		/// </summary>
#pragma warning disable CA1821 // Remove empty Finalizers //TODO remove this when https://github.com/dotnet/roslyn-analyzers/issues/1241 is fixed
		~SemaphoreSlimContext() => Dispose();
#pragma warning restore CA1821 // Remove empty Finalizers

		/// <summary>
		/// Release the lock on <see cref="lockedSemaphore"/>
		/// </summary>
		public void Dispose()
		{
			lock (disposeLock)
			{
				if (disposed)
					return;
				disposed = true;
			}

			GC.SuppressFinalize(this);
			lockedSemaphore.Release();
		}

Cyberboss avatar Jul 13 '20 23:07 Cyberboss

@Cyberboss

I'm not sure if anyone is aware that using var _ = GetDisposable() resulted in my finalizers being called on the rvalue (they won't be if disposed properly).

Can you provide a repro? using var _ = GetDisposable() produces identical IL to a using block with a non-discard so the behavior should be identical:

https://sharplab.io/#v2:D4AQTAjAsAULIGYAE4kGEkG9ZNy5AkgCICWAzgA4D2ZAhgEYA2ApkgOLMAuplNDLACgCUSALwA+JADsAro0YBuHHmW5EKACxIAshGFZVeNQAYkAN1oAnJAH0x7Lj2p0mzYUphGAvocPqQWtpg+tieRnggpgIW1gBm9hzc5M78bkIioeF4PmG4OV5AA==

HaloFour avatar Jul 13 '20 23:07 HaloFour

Hmm, pardon me, it seems something else is triggering this finalizer when it shouldn't. (Though I'm having a hard time figuring out what as all references to this class are using using)

Cyberboss avatar Jul 14 '20 00:07 Cyberboss