dotnet / csharplang

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

Champion "Lambda discard parameters" (VS 16.8, .NET 5) #111

Open gafter opened 7 years ago

gafter commented 7 years ago

Examples: (_, _) => 1, (int _, string _) => 1, void local(int _, int _) ...

See

CyrusNajmabadi commented 6 years ago

Might I suggest also adding it to warning waves, if that's ever to happen?

Note: i love warning waves. But they don't mean that things can actually be removed. It just means the compiler can add new warnings in a way that is felt to eb safe (precisely because of hte back compat concerns). People can opt into new warning waves and get the new warnings/errors, but the language still needs to preserve all the old semantics.

This allows teh customer to be the one that opts into choosing the time when the value of the warning outweighs the drawbacks of any breaks. I'm hugely supportive of this as it puts the customer in control, while not preventing them from upgrading, instead of us deciding for everyone.

CyrusNajmabadi commented 6 years ago

Note: i'm going to bow out here (with regard to back-compat). I think this horse has been beaten to death. I think the teams' position on back compat is well understood, and i hope we can improve things here with tooling/warning-waves. Repeating myself on these points probably won't help anything.

jnm2 commented 6 years ago

I would definitely love a real world example of someone actually having a mutable field called '_', and then intending to have a discard, but having that discard write into the field...

Me too.

DavidArno commented 6 years ago

@CyrusNajmabadi, @jnm2,

I would definitely love a real world example of someone actually having a mutable field called '_', and then intending to have a discard, but having that discard write into the field...

Well, guess what? 😀

It so happens that I have a property, __. Now the thing is, it started life as _ as it's used in pattern matching in SuccincT to represent "anything" (as per F#'s use if _ in patterns). Then the team made _ a discard. Not wishing to cause grief to users of SuccincT, I renamed it __.

Of course, if I'd not been hanging around here, knowing about discards before they were released, then I could have unleashed this on folk. I think the one saving grace would have been that its not mutable. But it's pretty damned close to what you asked for...

CyrusNajmabadi commented 6 years ago

But it's pretty damned close to what you asked for...

And yet not what i asked for :D

DavidArno commented 6 years ago

@CyrusNajmabadi, you are a hard man to please! 😆

firelizzard18 commented 6 years ago

@CyrusNajmabadi Simple solution: make _ an invalid identifier but require projects to opt-in via <LanguageVersion>. Thus if you don't want to break, you don't.

CyrusNajmabadi commented 6 years ago

That is the warning wave proposal @firelizzard18 . Note we would not make someting invalid on an upgrade scenario. You should be able to move to newer versions of the langauge without breaking your code. Code warnings/breaks should only come through a separate opt-in mechanism (hence warnings waves).

CyrusNajmabadi commented 6 years ago

@CyrusNajmabadi, you are a hard man to please! 😆

I think it's very worthwhile when discussing these sorts of 'issues' to strongly understand if they reflect real problems the ecosystem is having, or if they're 'warts' that some people don't like and want to be excised in the name of 'purity'.

To make a breaking change it must be the case that this is a significant problem impacting a lot of people. That was one the case for 'foreach' capture. It kept biting people all over the place and it was one of those cases that people felt must assuredly be a bug. We ourselves could not justify the actual behavior of the language (beyond: well... we shipped that way). Indeed, it was unclear if there was an actual use case out there where the current bahavior was actually desirable (no one coudl find one). These are the 'perfect storm' circumstances that justify breaking changes.

bkoelman commented 6 years ago

/cc @dennisdoomen for AV1739: Use an underscore for irrelevant parameters

DavidArno commented 6 years ago

@bkoelman,

Those "standards" lost me on the first rule, "Use US English". I'll use the language appropriate to the audience, thanks very much.

jnm2 commented 6 years ago

@DavidArno Unless the code is proprietary, isn't the audience the entire world?

dennisdoomen commented 6 years ago

Those "standards" lost me on the first rule, "Use US English". I'll use the language appropriate to the audience, thanks very much.

In this context, the audience are the other developers.

DavidArno commented 6 years ago

@Jnm2, propriety code makes up a significant part of all code written. Having a standard that only applies to code with a public audience seems ... shortsighted.

jnm2 commented 6 years ago

@DavidArno I wouldn't want to put an additional barrier in the way of open-sourcing part of a proprietary codebase.

DavidArno commented 6 years ago

@jnm2,

So you feel folk should learn English and only use the US variation of that language in their code regardless of their first language, just in case they want to open source it is future? That seems more than a little arrogant.

CyrusNajmabadi commented 6 years ago

this conversation seems non-constructive.

jnm2 commented 6 years ago

@DavidArno No, I don't think I feel that way. @CyrusNajmabadi Sorry, that was me.

sirius0xff commented 6 years ago

One possible solution for the problem of breaking change is use it as discard parameter only if _ is/are not referenced in the body.

Which would allow both:

SomeEvent += (_,_,_,_)=>foo(); and SomeList.Select(_=>_.Foo).MoreLinq....

And this would result in an error: SomeEvent += (_,_,_,_)=>_.bar();

========================================================== If nested, take the closest one in scope:

SomeEvent += (_,_,_,_)=>SomeList.Select(_=>_.Foo).MoreLinq....
//same as:
SomeEvent += (_,_,_,_)=>SomeList.Select(__=>__.Foo).MoreLinq....

or simply forbid it: SomeEvent += (_,_,_,_)=>SomeList.Select(_=>_.Foo).MoreLinq.... //error

========================================================== It may cause confusion tough...

gapspt commented 6 years ago

If only a special character was chosen as the discard, other than a valid identifier... Something such as ? or * would work just fine...

gafter commented 6 years ago

Since discards were added in C# 7.0 (a few language versions ago) we are unlikely to change the character we use for them.

gapspt commented 6 years ago

It was more me venting than anything else... But it actually would be possible to allow for both and a new special character, where the new character would allow for more possibilities, but the would keep retro-compatibility. (And 7.x is still the latest major though)

matkoch commented 5 years ago

Probably it's not the main idea of using discards, but being able to access the discarded parameter is a huge potential for fluent APIs. For instance:

DoSomething(_ => _
   .SomethingNested(_ => _
       .AndAnotherNesting()));

// current alternative
DoSomething(s => s
   .SomethingNested(ss => ss // or different letters of course
       .AndAnotherNesting()));
YairHalberstadt commented 5 years ago

@matkoch That kind of goes against the point of discards though

matkoch commented 5 years ago

@YairHalberstadt that's why I started my statement like I did ;) The limitation though is that it can only be used as very first expression, i.e., not later.

YairHalberstadt commented 5 years ago

Fair enough. @davidarno would hate it though 😉.

I prefer not allowing this kind of hack, but TBH I don't really care that much.

alrz commented 5 years ago
DoSomething(_ => _
   .SomethingNested(_ => _
       .AndAnotherNesting()));

I think what you want is "parameter shadowing in lambdas" which is already being considered.

DoSomething(s => s
    .SomethingNested(s => s // shadows the previous `s`
        .AndAnotherNesting()));

afaik, for now it will be implemented only for local functions as a part of "static local functions" (https://github.com/dotnet/csharplang/issues/1565).

dadhi commented 5 years ago

Anyone knows if exist a similar proposal for Linq expressions from bindings?

jeroen-mostert commented 5 years ago

I like this not just for itself but also so code analyzers can meaningfully start to issue "lambda parameter unused" warnings about code like

this.Bar = collection.Sum(c => Bar);

where of course the intent was to write

this.Bar = collection.Sum(c => c.Bar);

(And yes, I came here from fixing just such a bug.) CA1801 or vanilla Roslyn don't complain about this, and indeed such a warning would not be very practical today since it'd have to be suppressed very often. (I don't know if Resharper can warn for it, I don't have that. Having discards available makes the warning more viable regardless of the analyzer.)

yaakov-h commented 5 years ago

@jeroen-mostert A single-parameter lambda couldn't become a discard for backwards compatibility. This proposal only treats multiple parameters named _ as discards.

jeroen-mostert commented 5 years ago

@yaakov-h: Understood, but if _ becomes idiomatic as a discard for all lambda parameters, then _ can be treated as a logical discard by analyzers even for single-parameter lambdas, leaving only the (unusual and exceptional) case of _ actually being used in the lambda body. (Which, realistically, analyzers can then also warn about as something you probably want to write differently.)

jcouv commented 4 years ago

I renamed the issue as it is being broadened from lambda parameters to any/all parameters.

Note that we'd like to avoid variables being named _ (underscore), so we're tracking an analyzer or warning (in warning wave) to avoid any ambiguities (https://github.com/dotnet/roslyn/issues/26594).

jcouv commented 4 years ago

From discussion today in LDM, we're going to keep the feature narrow for now (only support it for lambdas and anonymous methods as initially proposed).

YairHalberstadt commented 4 years ago

Can I ask why doing this for all methods is more complicated?

jcouv commented 4 years ago

@YairHalberstadt It's not a question of implementation cost, but trade-off between language complexity and benefit to users. Methods are part of an API (they can be invoked, whereas lambdas can only be converted to delegates, so the parameters names are never accessible to callers). It is possible that discards would be generalized to methods later on, depending on feedback. More notes will be posted for today's meeting.

atifaziz commented 4 years ago

lambdas can only be converted to delegates, so the parameters names are never accessible to callers

Aren't they available through reflection? I rely on that in a library.

jnm2 commented 4 years ago

@jcouv Can local functions be included? They are often converted to delegates.

DavidArno commented 4 years ago

@jcouv,

You raise a good point re methods that I hadn't thought of. Consider the following code example,

public interface I 
{
    public void M(int a, int b);
}

public class C : I
{
    public void M(int _, int _) {}
}

public static class P
{
    private static void Main()
    {
        I i = new C();
        C c = new C();

        i.M(a:1, b:1);
        c.M(_:1, _:1);
    }
}

In C, I've used discards for M as I don't use the parameters. This makes sense to me: why name a parameter that isn't used?

But if I'm a developer working in a team that insists on named parameters (not sure if such a team exists, but thinking worst case here), then that line c.M(_:1, _:1); can't compile as I've trying to name those discards.

My motivation for wanting discards in methods (and operators, though I'm just lumping them in with methods here) is for situations such as when defining event handlers, operators and the like. In these situations, I have to specify the parameters to meet the contract. But if I then don't use those parameters, the compiler IDE complains with an IDE0060 message. So I want to use discards there to keep the compiler IDE happy.

But maybe this is the wrong solution to the problem. If a method must have a parameter to meet the contract, but that parameter isn't used, the compiler IDE puts the developer in the impossible situation difficult position of having to ignore that message. So maybe the simpler solution is to just not warn on those unused parameters when they are mandatory?

Joe4evr commented 4 years ago

@DavidArno I think in the case you present, that should likely be a compiler error since C.M is an implicit implementation of I.M and there need to be unambiguous identifiers for the first and second parameter because M is a public API regardless of whether or not C happens to be followed by : I.

But, I think explicit implementations maybe could allow discarded parameters since those methods are only callable through the interface signature. A potential (but perhaps unlikely) addition to that would be to also allow it in method overrides provided that the overriding class is less visible than the base class? Idk, it's obviously quite a complicated scenario.

HaloFour commented 4 years ago

@DavidArno

So maybe the simpler solution is to just not warn on those unused parameters?

I agree, the better approach would seem to have the analyzers not generate diagnostics for unused parameters that are required to meet the contract, such as for implicit implementation of an interface member or for overloaded operators, etc.

In general I'm a little squeamish about extending _ to all parameters. Lambdas and local functions are at least internal implementation details, but public methods are more likely to be walked via reflection and I have concerns over how such parameters would be interpreted by tooling.

Logerfo commented 4 years ago

Including local functions sounds perfect. Even private methods have their reflection concerns.

louthy commented 4 years ago

Will this also work for LINQ (as LINQ gets compiles down to lambdas)? For those of us that use LINQ for more than just querying an enumerating this would be an absolute godsend.

It would reduce the calls for an extension to the LINQ grammar. For example, I'm just working on a new IO monad:

static SIO<Runtime, ConsoleKeyInfo> ShowError(Error err) =>
    from _1 in setColor(Red)
    from _2 in writeLine(err.Message)
    from _3 in writeLine()
    from _4 in writeLine(StackTrace(err))
    from _5 in setColor(White)
    from k in readKey
    select k;

I would prefer to do this:

static SIO<Runtime, ConsoleKeyInfo> ShowError(Error err) =>
    do setColor(Red)
    do writeLine(err.Message)
    do writeLine()
    do writeLine(StackTrace(err))
    do setColor(White)
    from k in readKey
    select k;

But would be fine with just this (as a stopgap until the day that LINQ gets some much needed love):

static SIO<Runtime, ConsoleKeyInfo> ShowError(Error err) =>
    from _ in setColor(Red)
    from _ in writeLine(err.Message)
    from _ in writeLine()
    from _ in writeLine(StackTrace(err))
    from _ in setColor(White)
    from k in readKey
    select k;
ChaseFlorell commented 4 years ago

I asked this question on Twitter, but figured I should bring it over here https://twitter.com/ChaseFlorell/status/1286159277451653121

Question, for those insane folks who use _ => {} and expect a value from it, is this a breaking change?

Is there a different character you can use that would have otherwise been invalid pre C# 9? . => {} perhaps?

jnm2 commented 4 years ago

@ChaseFlorell No, it is not a breaking change. This feature only kicks in when at least two parameters are discarded.

CyrusNajmabadi commented 3 years ago

@jcouv do we have an existing issue covering multiple discards for methods, or multiple discards for locals?

jcouv commented 3 years ago

@CyrusNajmabadi Tracked by https://github.com/dotnet/csharplang/issues/2180

CyrusNajmabadi commented 3 years ago

Thanks!