Roslyn: C# compiler does not provide a warning about unused lambda variables

[ivanbasov]

For the code:

    void SomeMethod()
    {
        void localFunction()
        {
        }

        Func<int> lambda = () =>
        {
            return 1;
        };
    }

compiler provides a warning for the unused local function but does not for the unused lambda. I expect they should be consistent.
Those compiler diagnostics is used in roslyn-analyzers.

[jcouv]

Tagging @agocke

[agocke]

The lambda is used -- it is converted to a delegate. If you converted the local function to a delegate it would also be considered used. This is by-design.

[jcouv]

I think he means the local called "lambda". It is assigned to but never used. Am I missing something?

[ivanbasov]

Yes, the local "lambda" was created but never used, and there is no warning about this.

[jcouv]

It's the same when the local is initialized from a parameter (rather than a lambda). It does seem unexpected.

```C#
using System;
class C
{
void SomeMethod(Func parameter)
{
void localFunction()
{
}

    Func<int> lambda = () => // no warning on unused local lambda
    {
        return 1;
    };

    Func<int> lambda2 = parameter; // no warning on unused local lambda2 :-(

}

}
```

[VSadov]

C# // A local variable that is written to is considered to also be read, // unless the written value is always a constant. The reasons for this // unusual behavior are: // // * The debugger does not make it easy to see the returned value of // a method. Often a call whose returned value would normally be // discarded is written into a local variable so that it can be // easily inspected in the debugger. // // * An otherwise unread local variable that contains a reference to an // object can keep the object alive longer, particularly if the jitter // is not optimizing the lifetimes of locals. (Because, for example, // the debugger is running.) Again, this can be useful when debugging // because an otherwise unused object might be finalized later, allowing // the developer to more easily examine its state. // // * A developer who wishes to deliberately discard a value returned by // a method can do so in a self-documenting manner via // "var unread = M();" // // We suppress the "written but not read" message on locals unless what is // written is a constant, a null, a default(T) expression, a default constructor // of a value type, or a built-in conversion operating on a constant, etc.

[VSadov]

In any case, making this a warning now could potentially break somebody's code, so IMO, the bar for making suggested change would be fairly high

[sharwell]

@ivanbasov I would encourage you to provide feedback in support of dotnet/roslyn-analyzers#459 and/or dotnet/roslyn-analyzers#461.

[VSadov]

Right - "unused parameters" analyzer is one of my favorite from the old FxCop perf subset. Especially the special case about unused "this" - "why is this an instance method?".

Also, since we now have "_" discard, it could be easier to argue that unused locals are generally useless (except for debugging, maybe) and thus are likely to be mistakes/typos/bugs.

It is just because in either case there could be legitimate reasons, compiler must be conservative.
Analyzer could be more flexible and thus more useful in these scenarios.

[ivanbasov]

Thank you, @VSadov! For https://github.com/dotnet/roslyn-analyzers/issues/461, it was decided some months ago to use just compiler warnings as an input. Do you mean we should not rely on compiler warnings and should have own code for the purpose?

FYI: @mavasani, @333fred

[ivanbasov]

@VSadov, could you please consider adding a new warning id for those cases?
@mavasani, if there are other approaches to provide diagnostics not as warnings or errors, please let us know.

[sharwell]

@ivanbasov As long as the analyzer uses the compiler warnings as input, it will suffer from the same limitations as the compiler warnings. If you want to expand the set of supported cases, it would need to be implemented as part of the analyzer code.

[ivanbasov]

Thank you, @sharwell! We definitely can have the whole or partial implementation of on the analyzer side. However, implementation for the rule was intentionally replaced with compiler warnings some months ago. Since compiler performs parsing for the purpose, we would like to understand if we can/should it and extend its diagnostics. If we can get it, it would be great. If it is impossible, we will consider other options.

[ivanbasov]

@VSadov, we synced offline regarding the issue. There can be an option to solve it on the compiler side and use a new diagnostics code for unused lambda. I'm re-opening the issue, in case if it can be proceeded on the compiler side. Could you please review it once again?

[agocke]

Could someone detail what the compiler action would be here? Providing a warning here would have to meet a very high compat bar.

[jcouv]

@agocke I think there should be a warning here.
If you agree and the only concern is compat, then we should keep the issue open and attach it to warning waves issue.

[agocke]

@jcouv I suppose, although I'd prefer that we have a commitment to warning waves for a specific release rather than pushing this down the line.

[jcouv]

@agocke We're targeting warning waves to release along with C# 8.0.

[alrz]

Probably a duplicate of https://github.com/dotnet/roslyn/issues/15695