fsharp / fsharp/fslang-suggestions

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

Open
#1,010 4 comments 0 reactions 0 assignees View on GitHub
area: diagnostics
Dominant language
No language data
Stars
373
Forks
21
PR merge metrics
No merged PRs in 30d

Description

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

The following code produces a warning that `elementName` is unused.

```fsharp
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()
}

[]
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

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

is equivalent to

```fsharp
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](https://github.com/fsharp/FsAutoComplete/issues/709)

## 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](http://stackoverflow.com)) and I have searched stackoverflow for discussions of this issue
* [x] I have [searched both open and closed suggestions on this site](http://github.com/fsharp/fslang-suggestions/issues) 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.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the reproducing F# snippet and the existing unused-identifier warning, then read the linked FsAutoComplete discussion for context. Done means the example receives a clearer diagnostic about shadowing an existing identifier in a pattern rather than suggesting the pattern performs an equality test.

Written by the indexing model from the issue text.

Assessment

Tech stack
fsharp
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.