fslang-suggestions icon indicating copy to clipboard operation
fslang-suggestions copied to clipboard

Prohibit duplicate identifiers at the same level

Open pbachmann opened this issue 1 year ago • 123 comments

I propose we... Issue a warning when an identifier is declared twice in this same indent level with a view to making it an error in later versions of the language.

The existing way of approaching this problem in F# is... To either issue errors or do nothing, depending on whether the duplicates are at the module level or not. Eg. at the module level:

let invoice = 5
let invoice = GetInvoiceFromDatabase invoice

.. emits error FS0037: Duplicate definition of value 'invoice'

On the other hand, in every other circumstance, F# allows the creation of duplicate identifiers:

let LoadInvoiceAndPrintCustomer () =
    let invoice = 3
    let invoice = GetInvoiceFromDatabase invoice
    printfn "cust %s" invoice.CustomerName

Pros and Cons

The advantages of making this adjustment to F# are ...

  • Consolidate F#'s credentials as a strongly-typed language.
  • Allows F# to set itself apart from other mainstream languages by making it the only language that has a sensible default for identifier assignment (see 'Origins of this idea' below).
  • Newcomers are not taunted by language inconsistency.
  • Developers do not waste time pondering the benefits of giving different identifiers the same name.
  • Promotes the idea that F# language design is driven by sensible defaults and not by eager coders demanding more toys or the need to accommodate historical anomalies.
  • Encourages developers to focus on their key responsibilities which includes either (a) naming identifiers meaningfully or (b) avoiding the need to create temporary names by using the |> operator.

The disadvantages of making this adjustment to F# are...

  • Some existing code will see warnings and, in future versions, errors.

(in one sense, this is not a disadvantage because in fact what is being delivered to developers here is little more than good advice).

Extra information

Estimated cost (XS, S, M, L, XL, XXL):
I am in no position to estimate the cost.

Related suggestions: I have scanned/searched the 1200 or so issues and can’t find anything related.

There is a stackoverflow complaint - Error FS0037 sometimes, very confusing to which @tpetricek responds:

This is a bit confusing... It makes sense when you know how things are compiled though.

I think @tpetricek mistyped when he said 'a bit' - really he meant 'utterly'. As indicated elsewhere, I suggest F# developers should not be thinking about "how things are compiled" but instead be thinking about great names for their identifiers.

Another person wrote RTFM:

At any level of scope other than module scope, it is not an error to reuse a value or function name. If you reuse a name, the name declared later shadows the name declared earlier. However, at the top level scope in a module, names must be unique.

Which I think just begs the question.

Affidavit (please submit!)

  • [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] This is a language change and not purely a tooling change (e.g. compiler bug, editor support, warning/error messages, new warning, non-breaking optimisation) belonging to the compiler and tooling repository
  • [X] This is not something which has obviously "already been decided" in previous versions of F#.
  • [X] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [ ] This is not a breaking change to the F# language design
  • [X] I or my company would be willing to help implement and/or test this

For Readers

Please click the :+1: emoji on this issue, even if you don’t like the idea or see the need for it. These counts are used to generally order the suggestions by engagement.

An artificially inflated count might be necessary to counter the average developer’s preference for new toys over solid code. See Don Syme 12:43 – 13:29.

Origins of this idea

This idea arose from an attempt I made to explain why F# is a better programming language than others. Unfortunately, this article was based on the misplaced assumption (based on testing at the module level), that F# did not permit duplicate identifiers at any indent level.

pbachmann avatar Aug 02 '23 05:08 pbachmann

Shadowing is a valid F# feature and it will be a breaking change. However, it's a valid use for the F# analyzer, and should be tracked in F# repo.

vzarytovskii avatar Aug 02 '23 06:08 vzarytovskii

Can you show an example of where this "valid F# feature" is useful?

pbachmann avatar Aug 02 '23 06:08 pbachmann

A classic example that comes to mind.

let x item =
    match item with
    | Some item -> ()
    | _ -> ()

But I get how shadowing values of the same type can be dangerous.

kerams avatar Aug 02 '23 07:08 kerams

let x item =
    match item with
    | Some item -> ()
    | _ -> ()

... seem to be two different things,

  • first 'item' is a parameter to function x which is then passed to the match expression.
  • second 'item' is a placeholder into which the result of the deconstructed "Some item" is placed.

They can have the same name. Everyone knows that in different contexts, something with the same name can have different meanings. Fred from Accounts is not the same as Fred from Marketing.

I thought I made it clear in my proposal that I was commenting on two identifiers at the same indent level eg.

let x item =
    let myName = "Sam"
    let myName = "No longer Sam"

pbachmann avatar Aug 02 '23 07:08 pbachmann

I missed 'same indent level' in your original comment, but truth be told, I find it less of an issue than shadowing across scopes. You can filter some data in an inner scope and shadow a value, and then try to refer to it later in an outer scope, thinking you're accessing filtered data. Or you might be using the filtered data in the inner scope, but remove the filtering at some point in the future.

kerams avatar Aug 02 '23 07:08 kerams

I find it less of an issue than shadowing across scopes. You can filter some data in an inner scope and shadow a value, and then try to refer to it later in an outer scope, thinking you're accessing filtered data. Or you might be using the filtered data in the inner scope, but remove the filtering at some point in the future.

I think I agree with you (in one sense), but would like to be clear about what you mean. There was a proposal for C# called static local functions, which they had a meeting about, though I don't know whether it ever made it into the language. Is that an attempt to solve the problem you're talking about? By making the inner stuff not capture the outer stuff? If so, I sympathise but tend to solve that problem by saying to myself, "Well if the inner code is genuinely independent of the outer context, stick the code into its own context (a separate function) so there's no need to ask for a static keyword."

Returning to my proposal, it might seem like a "lesser issue" to you, but I wonder what programmers from other languages think? Is there a Java programmer who doesn't think it is perfectly reasonable for the following code...

class HelloWorld {
    public static void main(String[] args) {
        final var args  = "one";
        System.out.println("Hello, World!");
    }
}

to issue an error:

HelloWorld.java:6: error: variable args is already defined in method main(String[])
final var args  = "one";
          ^

?

pbachmann avatar Aug 02 '23 07:08 pbachmann

Can you show an example of where this "valid F# feature" is useful?

let results = getResults ()

Logger.log results

let results = results |> List.map ((*) 2)

...

We use it all the time in all the places.

vzarytovskii avatar Aug 02 '23 07:08 vzarytovskii

@pbachmann Any good IDE would highlight where the same variable is used. All variables are still typed strongly, the point of "Consolidate F#'s credentials as a strongly-typed language." is invalid. image

Also, indent levels become confusing when you consider that

let x item =
    let myName = "Sam"
    let myName = "No longer Sam"
    myName

can be written as

let x item =
    let myName = "Sam" in
        let myName = "No longer Sam"
        myName

if these two are considered differently then you have a new inconsistency.

Happypig375 avatar Aug 02 '23 08:08 Happypig375

@vzarytovskii If I understand the purpose of the logging example, you are saying "I've got some results, I want to log the results, then I want to manipulate the results (and maybe log them again) and I can't be bothered thinking of new names all the time - 'results' is good enough for me."

I apologise if I've misunderstood.

If "not having to think of new names all the time" is the use of shadowing, I understand but I just happen to have a view, expressed in my proposal, that developers should think of new names all the time or use a series of pipes. (The goal being to to create more readable code). This was explained in my essay to non-programmers.

Of course, maybe an experienced programmer like you may not have much interest in what non-programmers think.

pbachmann avatar Aug 02 '23 08:08 pbachmann

@vzarytovskii

Maybe there is a new philosophy in the F# camp? I happen to be in full agreement with Don Syme when he railed against the use of point free code in this video. On the other hand, you think

let results = results |> List.map ((*) 2)

is a perfectly reasonable "simple example" to provide to a possible newcomer to the language?

pbachmann avatar Aug 02 '23 08:08 pbachmann

@vzarytovskii

Maybe there is a new philosophy in the F# camp? I happen to be in full agreement with Don Syme when he railed against the use of point free code in this video. On the other hand, you think

let results = results |> List.map ((*) 2)

is a perfectly reasonable "simple example" to provide to a possible newcomer to the language?

It's was just the easiest example, which captures the intent, to type from my phone. It can be any lambda there. We do data transformation like this all the time, and shadowing is usually a way to go, to not pollute local scope with bindings which are essentially discarded.

vzarytovskii avatar Aug 02 '23 08:08 vzarytovskii

@Happypig375 Yes I agree that an IDE that highlights where an identifier is used and where it originates from mitigates the problem. I might sadden the original designers of the language to think that a good IDE is required to alleviate some of the problems in the language's design.

All variables are still typed strongly, the point of "Consolidate F#'s credentials as a strongly-typed language." is invalid.

Yes F# variables are strongly typed. I was making fun of the fact that in F# you can write code like:

let myName = "Sam"
let myName = 2 + 2

which in most languages would seem ridiculous.

Regarding:

let x item =
    let myName = "Sam" in
        let myName = "No longer Sam"
        myName

I am not familiar with "let ... in". Is this verbose syntax? Do I need to learn verbose syntax to understand your point?

pbachmann avatar Aug 02 '23 09:08 pbachmann

@vzarytovskii

OK, so it seems I have misunderstood you. When you say:

We do data transformation like this all the time, and shadowing is usually a way to go, to not pollute local scope with bindings which are essentially discarded.

.. you are not avoiding the generating a lot of names because of the difficulty of thinking of all those names, but because you want to assist the person trying to read your code by not giving him a lot of meaningless names to wade through?

pbachmann avatar Aug 02 '23 09:08 pbachmann

@pbachmann

I might sadden the original designers of the language to think that a good IDE is required to alleviate some of the problems in the language's design.

F# with all its type inference is already an IDE-dependent language. This is simply how the language is used.

Name shadowing is less ridiculous if you consider

let myName = "Sam"
match String50.validate myName with
| Some (myName: String50) -> // Wow! Different type!
    addMyNameToDatabase myName
| None -> Error $"My Name {myName} is longer than 50 characters."

Any other name would fall into the trap of adding unnecessary type information to the variable name.

I am not familiar with "let ... in". Is this verbose syntax? Do I need to learn verbose syntax to understand your point?

Yes, but it is not hard to understand. Any

let x = y
f1 x
f2 x

in local scope (NOT type/module scope) is equivalent to

let x = y in f1 x; f2 x

and it's similar to how MIT App Inventor (easy language for beginners) thinks about local variables. image

Happypig375 avatar Aug 02 '23 09:08 Happypig375

@vzarytovskii

OK, so it seems I have misunderstood you. When you say:

We do data transformation like this all the time, and shadowing is usually a way to go, to not pollute local scope with bindings which are essentially discarded.

.. you are not avoiding the generating a lot of names because of the difficulty of thinking of all those names, but because you want to assist the person trying to read your code by not giving him a lot of meaningless names to wade through?

Yeah, pretty much. But I'm biased, since I'm used to this style of code, and I account for possible shadowing, and use it a lot myself.

In my opinon, lots of different bindings can be harmful in different ways, can be unwilingly captured into closures, confusing for devs, regarding which one to use in the end (e.g. which one was last filtered, or mapped, or processed in a different way).

We cannot make this a warning which is emitted by default, since it will break a lot of existing code (people tend to make warnings threated as errors by defualt).

Two this can be considered are: a. Make it an informational warning, or off-by-default opt-in warning. b. Make it an another use-case for Analyzsers SDK which we're planning to work on around F# 9 / .NET 9.

P.S. one example from our VS tooling:

 let! declarationSpans =
    async {
        match declarationRange with
        | Some range -> return! rangeToDocumentSpans (document.Project.Solution, range)
        | None -> return! async.Return []
    }
    |> liftAsync

let declarationSpans =
    declarationSpans
    |> List.distinctBy (fun x -> x.Document.FilePath, x.Document.Project.FilePath)

It kinda makes it easier to read distinc logical blocks: first we fetch the data from the solution, an dthen we distinct it by the paths, without the need for separate binding.

vzarytovskii avatar Aug 02 '23 09:08 vzarytovskii

@Happypig375 Re: String50 validation, I have already made it clear that my proposal applies to variables at the same indent level (see comments above)

Some (myName: String50) -> // Wow! Different type!

is a different type and should be a different type, we are all in agreement on that. btw I would have thought that String50.validate would supply the type and there's no need for :String50 annotation (though you might have provided that for my benefit).

Re: understanding verbose syntax, not for me tonight. I'm too tired :)

pbachmann avatar Aug 02 '23 09:08 pbachmann

@pbachmann Consider computation expressions

let result = option {
    let myName = "Sam"
    let! myName = String50.validate myName
    addMyNameToDatabase myName
}

Happypig375 avatar Aug 02 '23 09:08 Happypig375

let result = option {
    let myName = "Sam"
    let! myName = String50.validate myName
    addMyNameToDatabase myName
}

I've done a lot of this in actor based code, where there might be multiple asynchronous steps in handling a message to the actor and each one transforms a state record. I've tried switching it to unique names for example:

let! state' = performStep1 state
...
let! state'2 = performStep2 state'
...
return Some (NextActorState, state'2)

But this just creates new problems, I could easily have used the wrong state[', '2] in the return. Or I could easily forget to update one of the steps when inserting new ones (this of course did happen).

By reusing the same name 'state' at each step I'm actually making it more like other languages where 'state' would be mutable and you'd just keep updating it. Except with F# I get the benefit that it's not a single mutating value and the tooling will nicely show me where a name is bound and used.

Another place I'd frequently use it is transforming function arguments on function entry e.g

let DoSomething (arg1: string) (arg2: string option) =
    let arg2 = defaultArg arg2 (arg1 + "magic")
    $"first was {arg1} and second was {arg2}"

realparadyne avatar Aug 02 '23 10:08 realparadyne

@pbachmann

Re: understanding verbose syntax, not for me tonight. I'm too tired :)

It's worth looking into this tomorrow. I feel it's something that should be highlighted more because it's key to really understanding that these types of languages truly are expression based. They are not a sequence of statements but a function is just one big expression. the let .. in .. form of binding reveals how that really works. The non-verbose form layered on top is generally simpler to read but it's also giving you an illusion of a sequence of statements/expressions by hiding the truth. That's why you can't consider language changes without considering both forms. Also the verbose form comes in very handy sometimes.

I really struggled to get to grips with functional and expression based programming for a long time (and still do). Especially with only written descriptions of it that didn't mesh together and didn't give me the visual mental model I need to understand things. I keep thinking if only I could make drawing and animations that explain it that it could be so much clearer!

realparadyne avatar Aug 02 '23 10:08 realparadyne

I think a factor here, touched on by @realparadyne is that F# (and immutable/transformation style languages) places less responsibility on bindings than you would expect in something like Java.

There, you have lots of mutable state managed by nominal classes, and identifiers have a sort of ‘authority’ over them, just mutating them into something totally different would indeed be peculiar and confusing.

But here, bindings don’t really own the data they refer to, they are just transitory tags into the transformational state as it moves along.

Also, I actually disagree that a developer should be spending time coming up with highly specific names for everything. Apart from it being time consuming, it actually decreases readability because you just have 10 things to track instead of 5 etc, and when you get used to reading this style, its not a problem at all to update the bindings in your head as you go.

robitar avatar Aug 02 '23 23:08 robitar

FWIW I wouldn't really support this feature. As an off by default warning maybe, but I don't know if it's worth the maintenance cost to keep it going long term.

cartermp avatar Aug 03 '23 00:08 cartermp

We use shadowing all over the place to redefine a value so the previous one can’t be used by mistake, a feature I love in F #

davidglassborow avatar Aug 06 '23 08:08 davidglassborow

Thanks @davidglassborow, others have made similar statements and I am still formulating a proper reply. The delay is because I feel I need not only to reconcile my original view with the views of those who have commented, but to equally factor in the views of those who are not in a position to comment (eg. high school students who are keen to learn coding).

pbachmann avatar Aug 06 '23 08:08 pbachmann

Yeah understand, I know we want to support new users, but it would be a shame to dumb the language down too much, if we did that I would definitely leave the community

davidglassborow avatar Aug 06 '23 10:08 davidglassborow

Me too - I'd be out of here in a flash. Seriously, this is something which has obviously "already been decided" in previous versions of F#. We don't warn about using a highly useful feature that is described in the language spec, and lots of us use all the time in order to gain readability and safety. There is nothing to warn about! Put it in an analyzer if you want, but not in the language, not even as a warning. It would greatly add to a situation where we'd have even more variations of the F# language. It's bad enough with the possibilities we already have to sort of redefine the language by specifying which warning to turn on or off or into an error.

BentTranberg avatar Aug 07 '23 07:08 BentTranberg

@BentTranberg I am aware that it is not permitted to suggest something which has obviously "already been decided", but can't find any declined suggestions searching for "shadow", or by scanning the first 200 or so declined suggestions. I was labouring under the apprehension that shadow labelling just kind of drifted into the language from OCaml and perhaps (just saying perhaps), now might be the time to revisit it. When fully articulated, I imagine this suggestion would fall under option 4 of the readme being, "things we never thought of before".

I acknowledge there was a suggestion that was declined which might be related inasmuch as it might have provided a fix for the problem noted by @realparadyne in his comment above (regarding actor-based code): https://github.com/fsharp/fslang-suggestions/issues/446 .. but it is not clear to me why it was declined.

Shadowing is in the spec? Well, presumably most features being reviewed would be in the spec.

pbachmann avatar Aug 07 '23 12:08 pbachmann

@pbachmann The relevant discussion of #446 is here https://github.com/fsharp/fslang-suggestions/blob/d48c35ce216e2bff148937ec028ad61e5c273fdf/archive/suggestion-8151819-add-support-for-forward-piping-the-result-of-an-a.md

I received this suggested operator... let (|>>) xA x2y = async.Bind (xA, x2y >> async.Return) ...which works very nicely.... async { return! downloadData() |>> Seq.where isUseful |>> Seq.sortBy sortSelector |>> Seq.toList } Based on this, I'm considering deleting this suggestion. Thoughts?

Happypig375 avatar Aug 07 '23 12:08 Happypig375

But still, piping necessarily assumes the argument to be piped is at the end, but we might use the intermediate result elsewhere or even multiple times. The operator is at most a workaround for a specific case.

Happypig375 avatar Aug 07 '23 13:08 Happypig375

@Happypig375

Your explanation of why it was "declined" was very helpful, thank you.

Re:

But still, piping necessarily assumes the argument to be piped is at the end, but we might use the intermediate result elsewhere or even multiple times. The operator is at most a workaround for a specific case.

I thought this was already covered in the apple pie story which I know you've read and commented on. (In the F# section, Grandma and the kid negotiate matters to come up with a combined pipe/intermediate result solution that they are both happy with).

pbachmann avatar Aug 07 '23 13:08 pbachmann

Ops, sorry, shadowing is perhaps not in the spec. I was so sure I saw it appear in a Google hit for a spec paper, but it looks like what I saw was this, from the language reference:

https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/functions/#scope

According to ChatGPT this feature also exists in OCaml and ML, and other functional languages. It pairs perfectly with the immutable-first nature of these languages.

In imperative languages, the situation is the opposite, and the reuse of variable names easily causes havoc. I know that very well because I used to fix other people's mistakes. In my experience, problems with shadowing in F# usually arise when it's mixed with mutability, which is precisely the situation you have in imperative languages.

BentTranberg avatar Aug 07 '23 15:08 BentTranberg