LiteracyFanatic commented 3 years ago

Show better error message when shadowing an existing identifier in a pattern

The following code produces a warning that elementName is unused.

open System.Xml
open System.Xml.Linq

let streamXmlElements (elementName: string) (path: string) =
    let settings = XmlReaderSettings(DtdProcessing = DtdProcessing.Parse)
    let reader = XmlReader.Create(path, settings)
    reader.MoveToContent() |> ignore
    seq {
        try
            while (reader.Read()) do
                match reader.NodeType, reader.Name with
                | XmlNodeType.Element, elementName ->
                    yield XElement.ReadFrom(reader) :?> XElement
                | _ -> ()
        finally
            reader.Dispose()
    }

[<EntryPoint>]
let main argv =
    streamXmlElements "entry" "data/JMdict.xml"
    |> Seq.iter (printfn "%A")
    0

This is true, but the error message is confusing because one might mistakenly assume that

match reader.NodeType, reader.Name with
| XmlNodeType.Element, elementName ->
    yield XElement.ReadFrom(reader) :?> XElement
| _ -> ()

is equivalent to

if reader.NodeType = XmlNodeType.Element && reader.Name = elementName then
    yield XElement.ReadFrom(reader) :?> XElement

when in reality the pattern is binding reader.Name to a new identifier that shadows an existing one.

I propose we detect the case where you simultaneously shadow an existing identifier in a pattern and fail to use it for anything and produce a clearer error message.

The existing way of approaching this problem in F# is just relying on the existing error message.

Pros and Cons

The advantages of making this adjustment to F# are making it easier to understand and fix an error.

The disadvantages of making this adjustment to F# are none that I can think of.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: This issue was originally raised in the FsAutoComplete repository. See the original discussion here

Affidavit (please submit!)

Please tick this by placing a cross in the box:

[x] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
[x] I have searched both open and closed suggestions on this site and believe this is not a duplicate
[x] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

[x] This is not a breaking change to the F# language design
[ ] I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

BentTranberg commented 3 years ago

I think this issue report lacks some details. Which one elementName triggers the error message, or is it both of them? Is there possibly another error message for the other one? What exactly is the error message? Include the FSxxxx number. Finally, I suspect the source demonstrating the issue can be minimized enormously.

LiteracyFanatic commented 3 years ago

In VS Code with Ionide, both occurences of elementName are grayed out and display the message This value is unused FSAC when hovered over. I'm not sure how to retrieve an FSxxxx number for this. I referred to this as a warning, but perhaps that is not the correct terminology as it does not show up in the output of dotnet build but only in the quick fix suggestions provided by LSP.

This is the line produced by LSP in the logs.

[02:03:06.061 INF] [LSP] TextDocumentCodeAction Request: {"TextDocument": {"Uri": "file:///home/jordan/src/kensaku/src/DataParsing.fs", "$type": "TextDocumentIdentifier"}, "Range": {"Start": {"Line": 42, "Character": 23, "$type": "Position"}, "End": {"Line": 42, "Character": 34, "$type": "Position"}, "$type": "Range"}, "Context": {"Diagnostics": [{"Range": {"Start": {"Line": 42, "Character": 23, "$type": "Position"}, "End": {"Line": 42, "Character": 34, "$type": "Position"}, "$type": "Range"}, "Severity": {"Value": "Hint", "$type": "Some"}, "Code": null, "Source": "FSAC", "Message": "This value is unused", "RelatedInformation": {"Value": [], "$type": "Some"}, "Tags": {"Value": ["Unnecessary"], "$type": "Some"}, "$type": "Diagnostic"}], "$type": "CodeActionContext"}, "$type": "CodeActionParams"}

The message can certainly be reproduced in fewer lines - let f x = 1 is sufficient - but that's sort of my point. The same error is displayed in the more complicated snippet I posted and while not incorrect as neither instance of elementName is used, it's highly confusing.

When discussing the clarity of error or warning messages, I think the context of how a user might come to write the offending code and how their mental model of what it does differs from the compiler's interpretation is really important. It seems as though there is a single error being referenced in two different places which is a bit paradoxical for an error about an unused value. If one isn't aware that you can't pattern match against non-literals, the error message doesn't help at all because reading the code it looks as though the argument is being used in the body of the function. An additional warning about shadowing makes it much easier to understand the root cause of the problem.

Note that I don't think we should produce a warning about shadowing in general as it can frequently be a useful tool to avoid coming up with convoluted new names for things when done intentionally. The criteria I am suggesting would be a more descriptive warning when a value is shadowed AND unused.

I hope that serves to clarify.

dsyme commented 2 years ago

This is a perfectly reasonable diagnostic suggestion. Normally we track those in dotnet/fsharp, but I'll leave this open to track it here

vzarytovskii commented 2 years ago

This is a perfectly reasonable diagnostic suggestion. Normally we track those in dotnet/fsharp, but I'll leave this open to track it here

Yeah, agree, we probably want to create a tracking issue int dotnet/fsharp in the Simple-F# bucket.

fsharp / fslang-suggestions