Skip to content
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

Bugfix for DotLambda - (a, (b, c)) was incorrectly detected as a discard (a compiler generated one was there) #16334

Merged

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Nov 23, 2023

Fixes #16276 .

This change makes sure that only real "_" discards produce a warning.
Hidden compiler-generated discards that cover a different construct (e.g. a discard for a tuple which is not reference as a whole) are not going to produce a warning for ambiguity.

At the same time, this also changes the warning to be an optional one.

@T-Gro T-Gro self-assigned this Nov 23, 2023
@T-Gro T-Gro marked this pull request as ready for review November 23, 2023 14:26
@T-Gro T-Gro requested a review from a team as a code owner November 23, 2023 14:26
@auduchinok
Copy link
Member

auduchinok commented Nov 23, 2023

I wonder what is the rationale to disallow these lambdas in some of the cases? For example, these cases are allowed:

let getP = 
    let _ = () // this `_` is definitely in scope but is ignored
    [""] |> List.map _.Length
let getP x : (string -> int) = 
    match x with
    | null -> failwith ""
    | _ -> _.Length // this is allowed, despite the another `_` in the scope
type T() =
    member _.M() =
        [""] |> List.map _.Length

There are probably more, these are just from the top of my head.
At the same time, the following is not allowed (and significantly limits the possible usages of the feature):

let f a b _ =

  // many lines of (nested) code

  let x =

    // many lines of code

    xs |> Seq.map _.P // not allowed because of `_` somewhere else above (i.e. in `f`)

I think it should be consistent: we should either disallow it everywhere where _ pattern is present in scope, or allow it. Given how ubiquitous _ is in F# code, I think there's no reason to disallow it in some of the actually common scenarios.

@T-Gro
Copy link
Member Author

T-Gro commented Nov 23, 2023

I wonder what is the rationale to disallow these lambdas in some of the cases? For example, these cases are allowed:

Important: This is just a warning hinting that readers of the code might think the code is doing something with "the other underscore".
The code still works and runs as a lambda.

It is not a hard error.

@auduchinok
Copy link
Member

auduchinok commented Nov 23, 2023

Important: This is just a warning hinting that readers of the code might think the code is doing something with "the other underscore".

That's an important note, I agree. However, a lot of people use settings like WarnAsError, and a lot of people prefer having no warnings in their code. It's also important for analyzers and various editor features to not produce code that would have warnings where there were none prior to the changes. I think from the language design standpoint it would be better if all these cases were either disallowed consistently or no warnings were produced.

@T-Gro
Copy link
Member Author

T-Gro commented Nov 23, 2023

TBH I have no problem with changing that warning into being an optional one.

@vzarytovskii : Can we change an existing warning into optional one after net8 has been released?

@vzarytovskii
Copy link
Member

TBH I have no problem with changing that warning into being an optional one.

@vzarytovskii : Can we change an existing warning into optional one after net8 has been released?

We can, since it shouldn't break any existing code.

@T-Gro T-Gro merged commit 1c761ea into dotnet:main Nov 27, 2023
25 checks passed
@T-Gro T-Gro deleted the 16276-unexpected-ambiguity-warning-for-dotlambda branch November 27, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unexpected ambiguity warning for DotLambda
5 participants