search

fsharp / fslang-suggestions

The place to make suggestions, discuss and vote on F# language and core library features
345 stars 21 forks source link

Prohibit duplicate identifiers at the same level #1299

Open pbachmann opened 1 year ago

pbachmann commented 1 year ago

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 ...

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

(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!)

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.

smoothdeveloper commented 1 year ago

https://learn.microsoft.com/en-us/dotnet/fsharp/tour#functions-and-modules second sample

Tarmil commented 1 year ago

@pbachmann Yes, that's why the warning I mention is off by default. But labeling a value without using it is still somewhat dubious (why not ignore it instead?) which is why the warning exists at all, and many people enable it. Then, if you really want to label without using, you can use a name that starts with an underscore: it will not be greyed out in the IDE nor trigger this warning.

BentTranberg commented 1 year ago

Can you point me to the specific F# documentation the issue alludes to?

Just look for the link (blue text?) "documentation" in the issue. It's almost at the start.

A shortcut: If you hover over your own link to the issue in your question a few posts above here, a box with a view into the issue will probably appear. In that box, the link "documentation" can be seen. Just click on that.

On the documentation page, search for "shadow". The issue also shows part of the text in question.

pbachmann commented 1 year ago

@Tarmil

But labelling a value without using it is still somewhat dubious (why not ignore it instead?)

As I suggested earlier, I guess coders like to label values they haven't used yet because:

if you really want to label without using, you can use a name that starts with an underscore

I didn't know that labels starting with underscores had special properties in F# and initially assumed you were proposing it as an idea. I spent some time considering whether I should support your proposal or not lol. Now that I see it is already in the language, I can see my deliberations are not relevant, but anyway I had fun thinking about whether such a feature would be consistent with what I love about F#.

pbachmann commented 1 year ago

@BentTranberg

Thanks for your follow-up, though you were pipped at the post by @SmoothDeveloper :)

I think your link to a related issue is an important one because it provides a real-life example of many of the issues discussed in this thread. Resolving them may resolve this issue.

While people are free, of course, to take Happypig's advice to just propose edits directly to, for example, the page discussed in that thread, I would welcome relevant links/comments to be posted here so I can close this issue if appropriate.

While I have much more I could say on all this, I would welcome others' views on key issues like:

BentTranberg commented 1 year ago

@pbachmann - I know you're new here - I'm not a maintainer of this repo, but from what I can understand, normally:

The way it normally works:

The repo is exclusively for language suggestions, and the suggestions are considered by the language designer(s), and then closed and accepted, or closed and rejected. Sometimes an issue is open to keep track of things, but don't you do that yourself without consulting higher authorities, is my advice. Often I see work issues are created when suggestion issues are closed.

I'm sure it's perfectly normal if we stretch it a little bit and discuss related matters, such as documentation, as we've done now. What we can't do is take ownership of an issue and track our own matters not related to the purpose of this repo, and especially not close issues, because then we disturb that very important process in the language team. We have sometimes been surprised by somebody closing or even deleting issues after we've had more or less fruitful discussions, which is unfortunate because opportunities and sometimes even information is lost.

It can take some time - weeks perhaps - before the powers in be will scan through new issues and actually decide on something, but sooner or later there'll be a response of some sort.

When an issue is closed, it's still possible to post if there's more to say, and it does happen that we need to round off with a comment or two.

I suggest you report documentation issues through the pages. Normally there's a GitHub issue created, but I'm not sure whether it's publicly available for posting.

pbachmann commented 1 year ago

@BentTranberg

You should not close issues yourself.

No probs. I just keep getting prompted with a button "Close with comment" so I thought it might be my responsibility. Happy for it to be someone else's.

You should not use issues in this repo to track your own work.

I've no idea what you mean by this.

I suggest you report documentation issues through the pages. Normally there's a GitHub issue created, but I'm not sure whether it's publicly available for posting.

I've made some effort to try to explain why I think language proposals, documentation, language goals, perspectives of new users etc etc. are all intimately related and should, where relevant, be discussed together. I accept people who have been in these forums a long time probably have a different view and are unlikely to change their practices.

I look forward to seeing how the "powers that be" engage with the contents of this thread.

Tarmil commented 1 year ago

@Tarmil

But labelling a value without using it is still somewhat dubious (why not ignore it instead?)

As I suggested earlier, I guess coders like to label values they haven't used yet because:

* Maybe they will use it, they haven't decided yet.

* They just want to look at it to see what it contains.

Well I guess I can specify: labeling a value in final code is somewhat dubious. In both these cases it's temporary during development and the warning isn't really a hindrance, in fact for the former it will remind you to use it. (And for the latter you can use the underscore prefix trick to silence it if you want)

pbachmann commented 1 year ago

@Tarmil,

Well, since the first variable is unused, this is already in the compiler as warning FS1182 (off by default): "The value 'myName' is unused".

Deepest apologies - I read your comment too quickly and misunderstood your point. It was only when I went back to some example code with the FS1182 warnings on for that project, that I noticed the warning covers shadowing as well - not just labels falling off the end of scope. So in effect the existing warning flag FS1182 covers the proposed warning raised by this issue. It does more, but with the underscore for labels you alluded to, there is hardly anything left to complain about.

In your honour, I will specify FS1182 in all current and future projects. :)

BentTranberg commented 1 year ago

You should not use issues in this repo to track your own work.

I've no idea what you mean by this.

I wrote that because it seems to me you encouraged further discussions here, about issues not relevant in this repo. I think it is better if you could join us in F# Slack or another suitable forum for broader discussions, for example about the issues you suggested.

pbachmann commented 1 year ago

@BentTranberg,

I wrote that because it seems to me you encouraged further discussions here, about issues not relevant in this repo. I think it is better if you could join us in F# Slack or another suitable forum for broader discussions, for example about the issues you suggested.

OK, I understand now. I have already given my explanation as to why I thought these issue were relevant and maybe we're not going to agree on that. I will of course abide by any direction from people in authority about what is relevant. If I may suggest, perhaps specifying such constraints in the Code of Conduct or ReadMe would avoid people wasting each others' time.

charlesroddie commented 1 year ago

I have been missing the fun.

I think an analyzer would be good here. I left a thumbs up not for the exact implementation described but for the motivation which would be better implemented as an analyzer.

It cannot be an warning or error for good reasons. In let x = expr1 in expr2 there can be no restrictions on the use of x in expr2: when expr2 is free to use any symbols as local variable names without knowledge of an outside context. That's important for local reasoning (and copy and pasting). There can't be a warning or error in sequential bindings too when they are formally equivalent to let ... in (which is the case most of the time, with bindings in modules being an exception where duplicates are currently already not allowed).

But if an analyzer is available I would turn it on. I.e. I would prefer to ban this in a codebase not as something formally invalid but as a matter of code style.

Reasons why I would discourage shadowing (even though it does have the uses described in posts above):

pbachmann commented 1 year ago

@Tarmil ,

Oops, I may have spoken too soon about FS1182 being a workable substitute. I put FS1182 into a couple of existing projects and got lots of warnings - mostly to do with parameters not being used. Now these parameters overwhelmingly belong to event handling type functions in which the "event handler" gets a number of parameters which may or may not be used. In two months time, a new requirement might come in for one of these handlers and some of the unused parameters may need to be used, which now have to be renamed from their underscored equivalent. Now I am hesitant to rename all those unused parameters.

I wonder whether not using a parameter is really akin to "throwing information away"? It feels different to me because the event handler is offered these pieces of information as a kind of courtesy, it is not obliged to take any notice of any of them. On the other hand, when you call a method to get information, or do some calculation to create information, you are kind of responsible for the result.

To make matters worse, it didn't catch any shadowed identifiers because I don't use any.

So I return to your comment...

But labelling a value without using it is still somewhat dubious (why not ignore it instead?)

... and ask myself: Is "somewhat dubious" such a bad thing to be?

pbachmann commented 1 year ago

@charlesroddie

Glad you see it as fun.

Now, to the extent I understand what you are saying, I agree with you.

Can you help me understand something of the other part of what you're saying?

eg. If you enable:

/warnon:1182

in a project, does some sample code in the form below suffer "undue restrictions on the use of x in expr2: when expr2 is free to use any symbols as local variable names without knowledge of an outside context":

let x = expr1 in expr2

?

Maybe if I have something to copy 'n paste into my editor I will understand.

Happypig375 commented 1 year ago

@pbachmann Maybe you should use the discard _ for all those unused parameters or prefix them with the underscore _ to indicate that they are unused.

pbachmann commented 1 year ago

@Happypig375

Well, as suggested in my comment to Tarmil, I started doing that but, given the enormity of the task, paused to consider whether I was actually making the code better. (Better meaning easier to read and maintain, and less prone to bugs etc.).

pbachmann commented 1 year ago

@Happypig375

Just to clarify a point you made a long time ago which I never got back to ...

You said:

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.

I (naturally) thought they should be considered the same - as you point out they are the same code written differently.

Perhaps rather than "same indent levels" I should have used the term "same scope". I was nervous to do so lest some readers interpreted "scope" to mean any code, including more deeply nested code, that might capture identifiers at the first level.

smoothdeveloper commented 1 year ago

this is great example where shadowing solves a problem:

image

(delta in the talk, it is fixed to use a wrapping type to further the safety, but with shadowing, you can prevent the original input to be used anywhere).

pbachmann commented 1 year ago

@smoothdeveloper

I agree that is a great example. I wonder if the proposed banning of shadow identifiers would encourage F# developers (both old and new) to write that code in the following way:

let read_user_input () = "sample user input"
let sanitize_input dummy = dummy
let log = printfn "%s=%s"
let query_user_data dummy = dummy
let process_result dummy = ()

let react_to_user_input () =
    read_user_input ()
    |> sanitize_input
    |> log "input user data"
    |> query_user_data
    |> process_result

react_to_user_input ()

The original input can't be used anywhere other than to feed into the sanitiser. Not only that, the code loudly proclaims that it doesn't even want to use it anywhere else, the user input being merely a step in a process chain.

bartelink commented 1 year ago

IMO overuse of pipelines like that just makes a mess; the example above makes me think of people using arrow libraries and point free programming, as deconstructed in https://eiriktsarpalis.wordpress.com/2017/04/02/programming-in-the-point-free-style/

Computation expressions and their affordances are a core part of the language to leverage to express things, as opposed to something to work around

Anonymous interstitial values can be the right tool for the job in some instances, but they're not a substitute for legitimate shadowing to hide a superseded value

Ultimately shoehorning things into pipelines like this only works up to a point; you end up reinventing the affordances of computation expressions, i.e. putting in an an Async.map in the above in order to thread values through in order to avoid using shadowing etc

I'm not sure whats a good talk to convey it, but I suspect the 'F# code I love' talk series might have touched on it a bit

The bottom line in this thread for me is that while there's a value to an open minded thought exercises, in reality F# (and ML etc) have shadowing; it's a natural part of the idiom, and imagining a 'better world' without it is pretty pointless

Which is a long winded way of saying 'this is a core thing that has long been decided' IMO! That's not to say that shadowing is not something that can't be confusing; I suspect there's definitely a way in which IDEs can help in highlighting when it's occurring Perhaps having a let new x = as a marker of intentional shadowing (new has other connotations so isn't quite right for F#, but I'm alluding to the secondary use of the new keyword in C# for something similar)

pbachmann commented 1 year ago

@bartelink

Sometimes I think you fellows are just pulling my leg. OK, I can use Google to see what you mean, but should I have to?

I'm not sure whats a good talk to convey it, but I suspect the 'F# code I love' talk series might have touched on it a bit

I should spend an hour watching a video you "suspect" that "might have" just "touched a bit" on a point you are trying to make? Perhaps you might notice that when I have linked to videos, the links consistently point to a particular part of the video and I include the start/end times in written form for further clarity?

bartelink commented 1 year ago

My apologies - this is not intended in the way you are taking it at all. I have watched many Don Syme videos. The peak of him doing that particular series of talks (and me watching them) was about 8 years ago. I am 95% sure he covers relevant territory in that video. But I don't want to mislead you into thinking that this is the one talk that explains it all, so I put a qualifier on it.

One key take away for me (and those talks are very prone to confirmation bias) was the fact that most things in the language are in there for a reason, and that you'd do well to know how to wield them.

But, a counter point he also makes is that there are bits that are prone to abuse.

And the job is to figure out a good path through that balances these subjective tradeoffs.

Let me be clear about one thing: I dont think that video is a waste of anyone's time. I highly recommend watching it. I'm not as high on confidence that it will assist me, or anyone, in conveying my point (shadowing can be a great tool; it can be abused, but I don't think it belongs on any "I can't believe that got into the language" list)

If there is a particular sentence in the above you want to unpack, please ping back; but trust me, nobody who watches this repo is out to get you. There is absolutely a risk of us being fishes or frogs that don't understand/question and/or are insufficiently aware of the temperature or nature of our water.

But, I'm serious about over-piping and arrows not being a good thing - forcing piping is absolutely not an improvement on the negatives of shadowing IMO.

pbachmann commented 1 year ago

I dont think that video is a waste of anyone's time.

I don't think so either - I have watched it and indeed referred to it earlier in this thread. I was objecting to having to watch it again hunting for something that might be relevant. (As an aside, on another occasion I pointed a well-known F# advocate to a part of this video which contradicted the advocate's own view on a particular issue. The advocate promptly sent a message to Don Syme demanding an explanation lol.)

There is absolutely a risk of us being fishes or frogs that don't understand/question and/or are insufficiently aware of the temperature or nature of our water.

I agree, and am working on something that will hopefully make it clearer to everyone how the temperature/nature of the water appears to frogs from a different pond. Whether I can get anyone to play along with this process is another matter.

But, I'm serious about over-piping and arrows not being a good thing - forcing piping is absolutely not an improvement on the negatives of shadowing IMO.

In some ways, we may be in complete agreement. In the essay that started this thread, the child tells Grandma that he misses his SaltySnow (the name of an intermediate step in the cooking process). He didn't want over-piping either. (BTW when I wrote that essay I'd forgotten that F# mostly enables what I called "at this point" labelling.)

Despite my concession, I think a clear, simple example of shadowing code that would be much better than the following and does not "just make a mess" would be welcome:

read_user_input ()
|> sanitize_input
|> log "input user data"
|> query_user_data
|> process_result
bartelink commented 1 year ago

For me, log is an imperative thing that I'd do on it's own rather than teeing it in a pipeline And the query is Async. If I do that with a pipe, I need an Async.map So the bottom line is that I don't think it improves on

let handle req = async {
    let req = sanitize req
    log req
    let! res = query_user_data req
    return render res }

I like the shadowing in the above If asked to do without it, I'd probably name req to rawInput so it would stick out. But, honestly, the exact thing I want to do having sanitised it is to:

In some cases, I'll even do three phases of that. In other words, there might be an input, a half_sanitised and a fully_sanitised.

Being able to use shadowing gives me options - I can call one raw and the next two cleansed, or I can refer to them as req and req' etc.

I agree that avoiding naming things can absolutely be a good thing. But that's got little to do with shadowing - if I'm naming things, that's intentional.

But that's my only real point - I'm not trying to come up with reasons to remove something I value.

Half of me wants to go mine my stuff on github.com/jet for examples of 'good shadowing' now, but I'm struggling with the overall terms of reference; I don't get:

vzarytovskii commented 1 year ago

It's a great discussion, lots of different viewpoints. It'll ultimately be up to @dsyme, I personally think it shouldn't be in compiler itself, but rater an analyzer.

We plan to work on analyzers sdk for F# 9.

smoothdeveloper commented 1 year ago

@pbachmann, your code snippets highlight you've groked the piping, and beside the "point" @bartelink brought, and the issue with the log function (which does return unit in the original code, and should still, to stay true), I think you've come to point of feeling the full nuance of why shadowing exist, how piping can be used to go towards "point free".

I feel piping is not making it as bad as when pointfree notation is used in the more esoteric ways, which is a bit frowned upon in commoners F# realms.

Both techniques are common place, and piping is generally great fit for data processing or similar use cases as fluent notation but with more composability; yet it tend to be over used when picking up the language, after the early humps.

Sometimes I think you fellows are just pulling my leg.

This is just the same some would feel, reading your points, but is transitory and illusory feeling, AFAIU. We are just trying to get minds to exchange information, with our limitation related to lacking full fledged telepathy.

The advocate promptly sent a message to Don Syme demanding an explanation lol.

I'm sure it was effective in giving a perspective to Don, but Don has nobody to send a mail to, to ask for explanation about F# idioms, he is our solution to the halting problem, and always will consider valid examples to adjust perspective on the design 🙂.

Whether I can get anyone to play along with this process is another matter.

If there is a bit of contention, I'd just observe and let people express their bias, while having own productivity party; the code you come up with can be consumed with generally minor/no efforts in remaining of dotnet codebase, and will prove itself.

As for handling biases / preferences, FUD, there is nothing else than patience, not being emotional, and your best intents, the whole of dotnet is not like something ideal while F# would be something "out of place", F# stands, and it does so much better than it did in the past in dotnet eco system, even if people emphasise/insist on dismissing the core advantages of the language itself, and finicking about things left and right.

If this goes bad, you just have to highlight that taking the same approach, you could also dismiss usage of "non F#" with equally "valid" biases, which you would not do 🙂.

Also, I'd like to share that F#, compared to other functional programming language of same family, can pretty much be seen as the VB of functional programming languages, for most of the code it is very accessible, even to non programmers (who have the chance of not being brainwashed with braces and semicolons).

So "anyone to play along" will not necessarily be a peaceable journey, but it is worth it, robust and succinct code, consumable in the whole of dotnet ecosystem (and more).

dsyme commented 1 year ago

It's a great discussion, lots of different viewpoints. It'll ultimately be up to @dsyme, I personally think it shouldn't be in compiler itself, but rater an analyzer.

From my point of view it's basically ok to have an opt-in warning for this kind of thing - I don't mind them being in the compiler, but equally if analyzers are in the pipe then yes, it will be fine as an analyzer.

p.s. I tried to understand all the different "Don Syme" references up above but couldn't quite grok them :) My understanding is that this thread is best interpreted as a request for an opt-in warning for duplicate rebinding of identifiers within a function/method.

pbachmann commented 1 year ago

@dsyme

I tried to understand all the different "Don Syme" references up above but couldn't quite grok them :)

The “Don Syme” references might usefully be summarised this way: They were an attempt by various contributors to try to gain further clarity as to our purpose here, beyond what can be discerned from reading the readme file. We all have heard you say that F# is for programming, but really feel we need to understand not "What is it for?" but “Who is it for?”

Without clear direction on this point, the default behaviour of any group is to naturally consider how any proposed change would affect them, probably to the exclusion of other key groups.

In addition to the “Don Syme” references, there was also a “Kathleen Dollard” reference in which I asked my co-contributors whether she was (like me) right to have a soft spot for early developers using F#. I can’t find any response to my question and am consequently no wiser as to “who F# is for”. Any clarification on how your views relate to Ms Dollard’s on this point would be welcome.

My understanding is that this thread is best interpreted as a request for an opt-in warning for duplicate rebinding of identifiers within a function/method.

I don’t know that I would call it a “request”. A request implies that I did it for my benefit and was in a position to assess the consequences of the proposal. I rather think I made a suggestion that I thought might benefit the whole F# ecosystem. Until I started discussing the issue here, I had no idea that so many people were using shadowing (as I pointed out during the discussion, it is not used in any of the example code in Microsoft documentation other than as an illustration that it can be done. My previous visits to Fsharp.Core or FS compiler source code did not alert me to any use of shadowed identifiers).

I am not sure of the utility of an opt-in approach. Who would choose to opt-in and why?

As to the “duplicate rebinding of identifiers within a function/method”, the suggestion was:

let fn item =
    match item with
         | item  -> “hooray: ” + item

… would be OK even though they are in the same function because | item -> is in a different scope to the item in “let fn item =”. I hope “scope” is the right term here.

Again, all I was trying to do was help make F# better so am entirely open to alternatives that may be better. When @kerams said, “shadowing values of the same type can be dangerous”, he might have been right to imply that perhaps it makes sense to ban the duplication in the entire method.

From my point of view it's basically ok to have an opt-in warning for this kind of thing - I don't mind them being in the compiler, but equally if analyzers are in the pipe then yes, it will be fine as an analyzer.

I previously asked for some clarity of how an analyser and compiler would work together to present users with a consistent message. I did not learn much from the responses so I guess I have to respond here with “no comment”.

PatrickMcDonald commented 1 year ago

My previous visits to Fsharp.Core or FS compiler source code did not alert me to any use of shadowed identifiers

An example of where it is used in FSharp.Core would be https://github.com/dotnet/fsharp/blob/b0a13ad0d5b3ef7d37869377bb88f37e508d0c11/src/FSharp.Core/prim-types.fs#L817

pbachmann commented 1 year ago

@PatrickMcDonald,

I checked the file you referred to and provide my evaluation below:

FSharp.Core/prim-types.fs

Label reuse to support compiler hack code

Here there appears to be some sort of hack which enables F# code to directly emit IL (# "IL syntax" #):

Line 684 :

    let reprAttr = get reprAttrs 0
    let reprAttr = (# "unbox.any !0" type (CompilationRepresentationAttribute) reprAttr : CompilationRepresentationAttribute #)

Maybe it can be re-written as follows, maybe it can't:

    let reprAttr = get reprAttrs 0
    |> (# "unbox.any !0" type (CompilationRepresentationAttribute) reprAttr : CompilationRepresentationAttribute #)

In any case, since it is specialised compiler code, I guess we don't have to think about it. At the very least, the code can be rewritten as "let reprAttrIL = (#..."

Label reuse to validate parameters

Line 781 Line 817 Line 859

Of the 7000+ lines of code in the file, these three lines and #5922 below, are the only relevant pieces of code I can find. The three are all a variation of the following:

let inline GetArray4DSub (src: 'T[,,,]) src1 src2 src3 src4 len1 len2 len3 len4 =
    let len1 = (if len1 < 0 then 0 else len1)
    let len2 = (if len2 < 0 then 0 else len2)
    let len3 = (if len3 < 0 then 0 else len3)
    let len4 = (if len4 < 0 then 0 else len4)
    let dst = Array4DZeroCreate len1 len2 len3 len4
    for i = 0 to len1 - 1 do
        for j = 0 to len2 - 1 do
            for k = 0 to len3 - 1 do
              for m = 0 to len4 - 1 do
                  SetArray4D (* use values *)
    dst

Basically, this is an example of "transforming function arguments on function entry" (ie. validating parameters) suggested by @realparadyne.

There is no question that in some ways this makes sense. The GetArray4DSub function has a whopping 9 parameters. Mapping each of the four len parameters to four new validated versions ends up with 13 identifiers in a small function. Enough for @vzarytovskii to justifiably complain about "polluting the scope with bindings that are essentially discarded."

Here I am inclined to wonder why the lengths need so many labels. If we were to put them a single value, what type would that value be?

The type would need to specify that:

  1. There are exactly four values.
  2. The values are all of the same type.
  3. They should be accessible independently or as a group.
  4. The containing set and the individual values should be immutable.

The 'List' type seems to fit best, though because it fails on the first point, we get a warning when we try to use it:

let inline GetArray4DSub (src: 'T[,,,]) src1 src2 src3 src4 (lengths:int list) =
    let [len1;len2;len3;len4] = // Warning that there might be more or less than four lengths
        lengths
        |> List.map(fun x -> if x < 0 then 0 else x)
    let dst = Array4DZeroCreate len1 len2 len3 len4
    for i = 0 to len1 - 1 do
        for j = 0 to len2 - 1 do
            for k = 0 to len3 - 1 do
              for m = 0 to len4 - 1 do
                  SetArray4D (* use values *)
    dst

Another problem is the caller is not told how many lengths he has to supply.

It seems to me that there is no reason not to create a variant of the list (and other collection types I suppose) that explicitly nominate how many entries they contain. There would be no need for new IL generation that I can see, checks would just be imposed on the compiler and user:

let inline GetArray4DSub (src: 'T[,,,]) src1 src2 src3 src4 (lengths:int list(4)) =
    let [len1;len2;len3;len4] = // No compiler warning!
        lengths
        |> List.map( fun x -> if x < 0 then 0 else x)

Line 5922

Here as far as I can tell (still being aghast that there is a second, "verbose" syntax to the language), that the label 'v' is being re-used:

let v = loop (n/2) in 
let v = mul v v in
if n%2 = 0 then v else mul v x in

I suppose this is the "light" syntactical equivalent:

let v = loop (n/2)
let v = mul v v
if n % 2 then
    v
else
    mul v x

Which could avoid the duplicate label with something like:

let v =
    n / 2
    |> loop
    |> fun v -> mul v v
if n % 2 = 0 then
    v
else
    mul v x

... or the second v replaced by some name that means more than 'v'.

Code that looks like it reuses labels, but doesn't

I was tricked in some cases to imagining I had found some code that re-uses labels but doesn't. In a sense they are not relevant to the discussion, but I thought I should look at why I got confused and if there is anything that can be done about it.

Line 1050

Below is what might be described as "quick check" code, ie. auditing code that is hunting for a result, uncovering interesting data that might be used to further the search as it goes along, but ultimately trying to return as quickly as possible. I thought it might be re-using the label 'c', but I was merely fooled by the indenting:

    let c = intOrder x.Rank y.Rank
    if c <> 0 then c else
    let ndims = x.Rank
    // check lengths
    let rec precheck k =
        if k >= ndims then 0 else
        let c = int64Order (x.GetLongLength(k)) (y.GetLongLength(k))
        if c <> 0 then c else
        let c = intOrder (x.GetLowerBound(k)) (y.GetLowerBound(k))
        if c <> 0 then c else
        precheck (k+1)
    let c = precheck 0

Had it been written using standard formatting, I would not have been fooled. Admittedly, there would have been a considerable use of horizontal space:

    let c = intOrder x.Rank y.Rank
    if c <> 0 then
        c
    else
        let ndims = x.Rank

        // check lengths
        let rec precheck k =
            if k >= ndims then
                0
            else
                let c = int64Order (x.GetLongLength(k)) (y.GetLongLength(k))
                if c <> 0 then
                    c
                else
                    let c = intOrder (x.GetLowerBound(k)) (y.GetLowerBound(k))
                if c <> 0 then
                    c
                else
                    precheck (k+1)
        let c = precheck 0

Perhaps there is a need for an editor shortcut that says, "reduce indent for the selected lines only":

    let c = intOrder x.Rank y.Rank
    if c <> 0 then
      c
    else
      let ndims = x.Rank

      // check lengths
      let rec precheck k =
      if k >= ndims then
        0
      else
        let c = int64Order (x.GetLongLength(k)) (y.GetLongLength(k))
        if c <> 0 then
          c
        else
          let c = intOrder (x.GetLowerBound(k)) (y.GetLowerBound(k))
          if c <> 0 then
            c
          else
            precheck (k+1)

        let c = precheck 0

Line 2031

Again here is code that looks like it might be reusing labels, but isn't, and again the confusion arises because of the curious formatting, presumably done to avoid overuse of horizontal space: 

    let inline FastCompareTuple3 (comparer:IComparer) (x1,x2,x3) (y1,y2,y3) =
        let  n = GenericComparisonWithComparerFast comparer x1 y1
        if n <> 0 then n else
        let  n = GenericComparisonWithComparerFast comparer x2 y2
        if n <> 0 then n else
        GenericComparisonWithComparerFast comparer x3 y3

I have already suggested a remedy for this.

Happypig375 commented 1 year ago

@pbachmann kudos for your great patience and for persevering with this idea even though so many have been against it, you deserve a 👍 unfortunately, this is a cultural change that takes much more than just a feature. It's just so easy to use this "hack" even though better alternatives may be available. If a codefix can be used to fix all uses of shadowing then by all means go for it. It would be the best way to get rid of shadowing.

pbachmann commented 1 year ago

@Happypig375

I don't understand what you mean by "codefix" and consequently what I should "go for".

Happypig375 commented 1 year ago

@pbachmann https://github.com/dotnet/fsharp/issues/15408

Code fixes are code actions that react on diagnostics and can fix them. In VS, they are represented with a lightbulb and a small cross on it.

If we can refactor code that uses shadowing easily by applying a code fix, then more people can avoid the use of shadowing.

pbachmann commented 1 year ago

@Happypig375

Sorry I answered without noticing the link in your response and have now deleted the response to give me time to read the link :)

bartelink commented 1 year ago

Half of me wants to go mine my stuff on github.com/jet for examples of 'good shadowing' now,

That half won; Intentional shadowing I don't know how to write better:

Not a total maniac; I use separate names too:

pbachmann commented 1 year ago

@bartelink

If I may, I will use your excellent analogy that F# is a pond full of fishes and frogs. I suppose the fishes inhabit the pond because they have no thought or means of escape, and the frogs are here because they like this pond and see no reason to hop elsewhere.

Occasionally, another type of animal may arrive at the side of the pond, let's say a duck. The duck may be visiting from another pond, such as the much larger C# pond, or it may be seeing a pond for the first time.

The occupants of the F# pond seem friendly and will happily answer any of the duck's questions; they have no problem at all describing in detail the various nooks and crannies that adorn the F# pond. What does not come natural to them, however, is to explain what the pond looks like from outside the pond. That's a shame, because "outside the pond" is where the duck is.

Now three key questions arise naturally from this:

  1. Should the fishes and frogs want strange animals to come into their pond?
  2. If they do, how will they make the pond seem appealing to these animals, given the communication difficulties?
  3. And if new animals are welcome, how will the process of integration be managed so that all the animals, old and new, regardless of their temperament and priorities, get what they want from the pond without messing it up for other inhabitants?

With regard to the first question, I have already offered the view that the F# pond is a very good pond, probably the best there is, and should host a greater variety of animals. I have already asked for others to offer their views on this and am patiently await a reply.

With regard to the other two questions, I have previously suggested that:

Language proposals, documentation, language goals, perspectives of new users etc etc. are all intimately related and should, where relevant, be discussed together.

ie. The fishes and frogs should, in deliberating over what they want their pond to look like, be thinking outside of themselves and be including the values and perspectives of ducks and other animals in these deliberations.

With that in mind, I am would like to pretend to be writing documentation explaining to F# newbies why my someone might want to write code in the way that you, @bartelink, have done in the first two examples you provided.

Proposed documentation explaining the benefits of shadowing

Unlike many other languages, F# allows you to give different meanings to names within the same function. Three ways this can be helpful are described below:

Reshape landscape

As the use of 'log' from this example shows, F# enables you to reshape the landscape within a function. How does this work?

Imagine you have a function that does some logging as it goes about its work. It so happens that you want to restrict the logger to logging only things appropriate to certain parts of the function. At the start of the function, you might have a section that logs anything. Then you have a section that only logs ints. Using a conventional programming language, you might do this as follows:

let DoThings (logger:Logger) =
    // The bit where we log anything
    logger.LogInt 1
    logger.LogInt 2
    logger.LogFloat 1.1
    logger.LogString "hello"

    // The bit where we only log ints
    logger.LogInt 3
    logger.LogInt 4

This code may be considered unsafe, however, because there is nothing to stop someone from logging a string in the section clearly allocated for logging only ints:

    // The bit where we only log ints
    logger.LogInt 3
    logger.LogString "sneaky"
    logger.LogInt 4

To avoid this difficulty, you can use a technique called reshaping the landscape. The code now looks like this:

let DoThings (logger:Logger) =
    // The bit where we log anything
    logger.LogInt 1
    logger.LogInt 2
    logger.LogFloat 1.1
    logger.LogString "hello"

    // The bit where we only log ints
    let logger = logger.LogInt
    logger 3
    logger 4

After the statement, "let logger = logger.LogInt", we have reshaped the landscape so that the orginal logger will be no longer accessible and consequently, only ints may be logged.

Limited use functions

As the use of "primary" and "fallback" from this example shows, sometimes you know you want to use a function only a certain number of times and then seal it off to ensure is not used again. F# allows this as the following example illustrates:

let fn () =
    let add x y = x + y
    let add = add 1 2
    let addmore = add 3 4 // add is no longer a function and generates a compile error!

If you hold your mouse over the second add, you can see another benefit. The add is now an integer, ready for use as a return value or as an input to further calculations.

Pseudo mutable variables

As the use of "events" from this example shows, sometimes you yearn to use a mutable variable, though your conscience tells you not to.

One situation where this might be the case is if you want to track something that isn't really part of the main flow of what you're trying to do. Consider:

let result inputs =
    /* some extremely detailed calculations based on the inputs resulting in 42 */
    42

Obviously, the details of these "extremely detailed calculations" have not been shown, but we can imagine that we're not going to want to be distracted when we're doing them. We don't, for example, want to be fussing with data that isn't really part of the calculation. eg. recording occasional notes.

One approach is to create a mutable list of notes and progressively add to them:

let result inputs =
    let mutable notes = []
    /* calculations */
    notes <- "Found something weird" :: notes
    /* more calculations */
    notes <- "Found something else" :: notes
    42

To avoid using a mutable variable, you can simply rebind the immutable notes to new notes:

let result inputs =
    let notes = []
    // calculations 
    let notes = "Found something weird" :: notes
    // more calculations 
    let notes = "Found something else" :: notes
    42

Admittedly, it could be argued that you've introduced mutation by stealth and made the code less clear. In the first example, you can hover your mouse over any reference to notes and see that it is a mutable string list, from which you can surmise that it is variable that has deliberately been created with a view to progressively taking notes during the course of the function. Furthermore, the < - operator implies the same thing, so hovering is not really required.

Alternatively, hovering your mouse over any reference to notes in the second example leads you to a daisy chain of notes references that might have you guessing as to their purpose.

pbachmann commented 1 year ago

@Happypig375

OK, I went off to understand codefixes.

Three examples of what I saw:

1. Unused function parameters

let fn parm1 = 15

There is a light-bulb that appears when I hover over parm1. This, apparently, is a "code fix". However, as explained previously, I am very happy with the greying out of unused parameters and see it as a perfect response from the editor. In the rare case I have not used parameters, it's because they are part of a event-handler type code and are waiting to be used at some time in the future. The "remedies" suggested by the code fix do not seem relevant.

If you think about it, there are three main possibilities why a parameter might not be used:

2. Implement interface

type MyLog() =
    interface ILog with

There was a light-bulb offering to help me implement the interface in two different ways. Brilliant, I reckon - just what the doctor ordered.

3. Missing code fix for incomplete pattern handler

let fn (x:int) = 
    match x with
    | 15 -> "hoy"

There is no code fix for it! Should this warning be removed from the compiler while we wait for a code fix to be written? This warning seems to have more in common with the shadowing proposal than Example 1 does. Any attempt at creating automated refactoring code here would likely generate useless code.

Summary

From these examples, it seems that code fixes can be very useful in some situations and hopeless in others.

The situation where it was good, "Implement interface", was one in which the appropriate "fixes" are clear and unambiguous but complex enough for the developer not to want to write them manually. It seems to me that code fixes should be created only in circumstances where they are unequivocally useful. Otherwise, even useful code fixes might be ignored by developers having had bad experiences with the other ones.

Given the examples from FSharp.Core/prim-types.fs and @bartelink's code (see separate comment), "code-fixes" for shadowing does not feel right to me. There are a endless number of fixes. Take even the simplest example:

let add x y = 1
let add = add 1 2

How is the analyser supposed to think of a better name for the second add? What if the code were written by an non-English speaker?

Review of discussion so far

It seems to me that the use shadowing in FSharp.Core/prim-types.fs is trivial (and of the four places it is used, three are essentially the same code). Yes, I speculated how F# code be even more safe and expressive by adding a list(count) type but that's really a side issue.

It seems to me that @bartelink's examples, when explained simply, are not persuasive. Feel free to explain them some other way. It may be that there is a way to explain the benefits of shadowing to new users so they thrilled with it.

To me, I just don't see s the big deal in all of this.

Assuming the proposed feature is enabled as a warning by default, the worst that can happen is that the FSCore/compiler writers and @bartelink need to change their projects to turn off the new warning. Or, alternatively, #nowarn directives are added to the top of files. There's already 9 warnings disabled in prim-types.fs, so what's one more? Maybe people would get around to refactoring the code, maybe they wouldn't. What difference does it make?

I agree with your previous comment that there are cultural issues involved, but people who are new to F# or who have never heard of F# also have cultures. Several times now I have inquired, in one way or another, the extent to which the perspectives of these outsiders are factored into decisions on the future of F#. I kind of think that a clear answer to this question is due soon.

bartelink commented 1 year ago

Thanks for taking the time to reply in detail. Bring on the ducks ;)

However, I don't agree with the characterization that the events in my examples is 'yearning for mutation' (or the general concept of 'pseudo mutable variables'); I'd argue that I'm progressively building something.

The point is that in each phase of the processing, there are 'events' - the types may differ, but the role it plays is stable

Also, in some cases, the value may be derived partially from the preceding value

In all cases, the previous variants are irrelevant and superseded.

When expressing intent like this in e.g. C#, I've often used braces to group blocks of logic so that I get to re-use a given name (like events). Yes, the types where the same, I could declare one at the top, but then each phase of processing would involve me mutating the variable. Value bindings (and being able to shadow ones that are being superseded) in F# is about names in a scope, not about mutation.

The best writeup I'm aware of regarding this role based naming is https://blog.ploeh.dk/2020/11/30/name-by-role/

The other thing here is that each of the phases of the processing is indented to the same level, but really we have a big nested if/else chain with early exits. I was trying to hint that any definition of 'the same level' needs to cover that nuance

Similarly, with the log example, I'm not really doing (or seeking to do) mutation (in fact I believe the Serilog API internally is implemented via structural sharing)

In this instance the type of log remains stable. Again though, I'm not intending to mutate anything. I just have a newer thing playing a given role in my context.

I dont have a decent example to hand but another use of shadowing is to progressively partially apply arguments to a curried function. Something like

let execute usercontext req = ....
let run user thingb thingc =
    let execute = execute (createUserContext user)
    execute (combine thingb thingc)
    execute req

Think of this in the context of a type that has a primary ctor arg that requires lots of context, and a member that uses 3/4 helpers to map 4 arguments to things that you partially apply to the 'execute` thing

The bottom line for me is that the fact that shadowing can look like mutation and/or be substituted with it is invalid, and misrepresents the point of shadowing.

Apologies for not explaining myself very well - documentation is hard.

I can't personally recall where I read about shadowing first - chances are it was fsharpforfunandprofit.com and/or Expert F#

But I do suspect there is some prior art that might be useful in conveying the various ways in which shadowing can be a valid tools.

Naturally, the degree to which the shadowing idiom is valid, clear and/or better replaced with something else in a given context is a highly subjective thing - lots of factors come into play:

I'm not sure the compiler team is likely to turn on warnings by default for a new warning, especially one that's only flagging usage of an established feature. Yes, even if it was a level 5 warning (though it does seem to me that having a warning level '6' that ispurely about emitting diagnostic information might be useful)

Also, to reiterate to editor visualisation people: I'd love to see some subtle visual indicator where a value shadows something (though I'm thinking that this could get noisy and painful - I use log as a binding name very frequently, despite knowing what it already means!)

Happypig375 commented 1 year ago

@pbachmann The code fix should just convert the use of shadowing to e.g. piping, instead of trying to suggest names if possible. @bartelink That example piece of code shadows in a different indentation. It is not a problem as they are not of the same indentation.

pbachmann commented 1 year ago

I read your points, I won't respond in detail. We have both expressed our views quite fully, I think.

The bottom line for me is that the fact that shadowing can look like mutation and/or be substituted with it is invalid, and misrepresents the point of shadowing.

The terms that stick out for me are "bottom line" and "invalid". I remember @Happypig375, right at the start of this thread, referred to a point I was making as "invalid". You've used the same term in making your summary assessment of my views. By contrast, I am inclined to assume that when people see things differently it is because they are looking at it from different angles.

Bring on the ducks ;)

Will they come?

Apologies for not explaining myself very well - documentation is hard.

Everything (eg. ice skating and tennis) is hard until you practice. Why not practice explaining things simply? The world is crying out for F# to be explained simply.

You could, for example, look at other how members of the F# community tried to explain things simply, and try to do even better. In a recent video, @vzarytovskii said in response to a question about "What he loves about F#":

I actually wanted to add something about piping, it's very a interesting point. So the whole idea of piping in F#, actually any ML language, is built on top of currying and partial application. Using piping I was able to explain to people who are new to F#, and functional programming in general, [...] why piping works and how partial application works. It was a very surprising moment for them, it was a "Wow!" moment and like "I can do so much more!" And I guess this is also why in some other languages it's harder to make it work properly or to make it look good.

At this point we can see Kathleen Dollard raising her hand and asking a question to direct the conversation towards "simplification". Her active engagement in the conversation, not just passively sitting there waiting for her chance to talk, is I think something we could all learn from:

I want us to answer a question here. Either one you can pick this up, because I can't do this right now off the top my head well. Who can give us a simplistic, pretty common denominator explanation what currying and partial applications are because we just used those words and they're very functional words but in the end they're probably actually pretty simple concepts. They just often get explained it very confusing ways, so I don't want to do that. No confusing, no Lambda calculus theory here.

Chet Husk responds:

Let me try and take a stab at it. Currying is the process of converting a function that takes multiple parameters into a series of functions that each take a single parameter. If you think about an add function, there are typically two parameters, left side right side. I'm sure there's a mathematical term for that, but in F#, if you define an add function that takes two parameters, what you get is actually two or three different functions under the covers and you will end up with an add function that takes a single parameter, which returns a function that takes another parameter which then gives you your value. This process of automatically generating the sequence of functions is currying, and partial application is when you as a user only supply a subset of the parameters to a function. These things work together fairly nicely because function syntax in F-Sharp is very uniform. It's all just whitespace, you're applying some parameters or you're applying all parameters and it all looks the same.

Ms Dollard finishes with:

So hopefully that makes sense to people.

@bartelink:

pbachmann commented 1 year ago

@Happypig375

The code fix should just convert the use of shadowing to e.g. piping, instead of trying to suggest names if possible.

I have already said why I think code fixes are counter-productive in all but limited cases. You are welcome to re-read what I wrote.

bartelink commented 1 year ago

Apologies; I really didn't have time to write a shorter letter; but it's not in my nature to sit on things (and, anyone that's suffered my writing knows that succinct direct prose aint my forte). I definitely make myself write docs and am not the type to pull up the ladder after I've climbed. I appreciate you putting in the legwork here at a time when you are close enough to the learning experience when you have a good chance of casting yourself back to early learning phases. I've watched that video and also appreciate the verbal skill, empathy and deep experience on show. I can only do my best but please rest assured that I'm not here solely to fix people being wrong on the internet at all costs.

I had not followed the thread sufficiently closely to know whether my example of reusing the same names within conditionals was 'at the same level' or not and was assuming it might be. Happypig clarified that, so that's half my examples gone, which makes things easier to explain/reason about.

I was/am attempting to help by surfacing what I believe to be useful examples of shadowing:-

I fully agree there should be better docs about this. I am open to there being better ways of writing things and am not assuming there isn't a better world out there were shadowing is not part of the answer. I deliberately brought long ugly pieces of code (that long function can definitely be expressed better but I've jet to figure out how). If the proposed warning existed (esp if it was on by default at level 5!) I might have been forced to write it more clearly.

But, instead of pointing out caverns in my ability to express myself in prose (or code), I'd like to focus on more deeply figuring out whether the log example is 'valid'. I really don't mean valid/invalid as an aggressive pejorative. The TL;DR of my message was "if you thing mutation begins to solve my need, then I'm pretty sure you've misunderstood; let me expand". My interest is in knowing if there is a better way to do that code. The reason to ask that is not some wacky putting it up to you motive. It's because I'd love to have a warning to nudge me out of a technique, if there is a viable alternative. Until there is one, the notion of having such a warning on by default is 'invalid' in my mind.

Hopefully one of us will find the time to find writeups on the concept of shadowing with examples soon. Multiple lang communities have it, so I doubt we really need to come up with all the examples or write the prose from scratch based on random people in this thread bringing examples here and there.

When we reach that point, I'd hope we'd be able to sketch out a notional table that summarises ways in which shadowing arises, along with potential ways of refactoring that to avoid it (e.g. piping so interstitial values don't need names, mutation if that more honestly conveys what's going on etc). After that's arrived at, a clear formulation for the conditions under which a warning would fire can be more explicitly defined, and the exact scope of the documentation that we hope a person facing the warning would want can be arrived at.

pbachmann commented 1 year ago

@bartelink

Thanks for your response, I feel you are genuinely trying to work in a conciliatory way.

Perhaps its important to restate my key points: I studied your code with a view to explaining what the value shadowing had for you and how that value could be explained to people new to F#. If these benefits could be made clear, there would be no need to ban shadowing at all, just update the documentation and everyone is happy.

I decided there were three distinct ways in which you used shadowing to get benefits:

  1. Reshape landscape
  2. Limited use functions
  3. Pseudo mutable variables

With regard to your logging code, I thought it was an example of "Reshape landscape" rather than "Pseudo mutable variables". ie. Your logging function changed during the course of your code so that it had to be used differently in some places than others. Your "progressively building event list" code, by contrast, seemed to me an example of "Pseudo mutable variables"

So, if I may focus on what you say you want to focus on:

I'd like to focus on more deeply figuring out whether the log example is 'valid'.

... and ask:

bartelink commented 1 year ago

TL;DR I don't know but let me try to answer anyway on a best effort basis....

I feel a 'phases of processing' characterization is missing.

The common bit being that within any given phase you have similar names playing similar roles. Sometimes the types change a little. Sometimes the values are derived fromthe thing they will then shadow. Often that can be refactored to a set of individual functions chained together with piping. However, e.g. in an async expression or similar, you can't 'just change it to piping' without going down the road of having Async.map and other such combinators that many people for better or worse try to avoid (I do). As mentioned, in C# with no shadowing, sometimes I'll put a block inside braces to avoid having to mutate something that I'm really just trying to shadow.

The logInt vs logFloat example doesn't convey that really there are multiple phases of decorating the logger going on.

re The limited use functions: I'd try to broaden the definition to cover my botched attempt of doing a series of partial applications to the same function based on some inputs. (Though I suspect that the key thing there is that you want to convey that often it can be refactored from a long function to a chain of functions that return another function. NOTE1)

I feel your groupings/characterizations are heading in a decent direction, but they don't speak to me yet. I've never tried to explain or justify the uses before so my next step would not be to try to work from your three groupings, but instead to go off and see if I could find articles that give better examples.

If you forced me to write something right now that I'd like to have a beginner exposed to, I think I'd seek to convey that:

NOTE1 Having said that, as a beginner, I recall functions that have a body returning a fun nextArg -> with a block being very hard to traverse. e.g. https://github.com/jet/FsCodec/blob/master/src/FsCodec/Union.fs#L30 is concurrently a) terse and efficient b) incredibly cryptic and not something I'd subject someone new to (and the let tmp = ... in fun x -> <something using tmp> piece was something I hated even more)

NOTE2 not sure I have a good example where mutation is better than constantly shadowing other than perhaps where there are conditional bits or nested functions. In general I try to map such cases to simpler list/array/seq comprehensions (potentially with use of yield! and/or recursion etc), which removes the need for a temporary, piping or mutation and instead makes for an easy to scan way of building something (including with conditionals)

pbachmann commented 1 year ago

The logInt vs logFloat example doesn't convey that really there are multiple phases of decorating the logger going on.

Just quickly, to show my sincerity, I want to say that I did have another example which had more phases (if I understand what you mean by this), but that example didn't stop people working around it. It was something like:

let fn (logger:ILogger) =

    // Log ints
    let log value = logger.LogInformation (sprintf "%d" value)
    log 15
    log 3

    // Log strings
    let log value = logger.LogInformation (sprintf "%s" value)
    log "one"
    log "two"

    // Log floats
    let log value = logger.LogInformation (sprintf "%f" value)
    log 15.3
    log 3.3

.. but people could work around it by calling logger directly, which I was trying to hide.

Now, as usual you you've made many points in your reply and I think it would be unproductive for me to pursue all of them, so I'll just focus on a single easy-to-understand point you made and see whether we develop some ideas around that. You said:

If you forced me to write something right now that I'd like to have a beginner exposed to, I think I'd seek to convey that:

  1. ....
  2. it can be used for good (putting bad/superseded/uncleansed things beyond use)
  3. ...

I think someone else made this point previously in the thread (maybe it was you), but in any case it is something that many languages cannot do. The obvious example, again made previously (by realparadyne I think), is that you can validate input parameters and then ensure that the unvalidated parameter is not accidentally used.

let ProcessData input =
    let input = cleanse input

But, even though I really, truly want to see the benefits of this, I can't stop nagging myself with questions. eg. If this input is potentially dangerous, shouldn't you describe it as such?

let ProcessData untestedInput =
    let untestedInput = cleanse untestedInput

Now the second line doesn't make sense, so fixing that:

let ProcessData untestedInput =
    let input = cleanse untestedInput

.. and you're back to no shadowing.

Anyway, @bartelink, I think it's up to you, now that you have identified "putting bad/superseded/uncleansed things beyond use" as a key benefit of shadowing, to provide some easy to understand examples to highlight this. Examples that, as @vzarytovskii described, make the ducks sit up and say, "Wow! I can do so much more!"

bartelink commented 1 year ago

Re the logInts vs logFloats and/or the 'reshaping the landscape' characterization in general is not speaking to me, but I'm not going to call it pointless either; ultimately conveying the value is a subtle thing, and it's all about the target audience (which likely aint this bullfrog)

I don't have a decent 'put dangerous stuff beyond use' example and I agree that in general the 'well then give it a pejorative name that will guard against misuse' solution is an excellent default answer until someone finds a better example that isnt solved by that.

I hereby relinquish any claim you're attributing to me wrt this being a 'key benefit of shadowing' ;)

Which leaves:

The concrete example of the log layering I gave:

But I think it should remain on the table as:

TODO: search outside my code and this thread for examples of shadowing to work into the "raison d'etre list". (For me, having an agreed set of valid examples that more than 2 people like is a prerequisite for sketching docs and/or specifying a warning policy)

smoothdeveloper commented 1 year ago 
use reader = new StringReader(data)
use reader = new CsvReader(reader, ...)

same with writer using StreamWriter and CsvWriter

TheJayMann commented 1 year ago

One way in which I tend to use shadowing, outside of what has been mentioned previously such as to provide a default value for an option argument, is to split the core logic from the decorations, such as logging and validations, in a function. Typically, in C#, where shadowing is much more limited, this is done by creating a method for the core logic which has a suffix of -Core, and access to this method is made as limited as possible. In recent C#, making use of local functions tends to help best limit access. In F#, for class functions and local functions (as module functions are not allowed to have duplicate names), I can use the same name for these functions, and shadowing prevents code from accessing the function with the core logic directly.

let log = createLogger()

let processData data =
  // complex data processing logic

let processData data =
  log.Info "Begin processing data"
  let result = processData data
  log.Info "Data processing complete"
  result

let processData data =
  match data with
  | [||] -> None
  | _ -> Some (processData data)
pbachmann commented 1 year ago

@smoothdeveloper

Thanks for pointing me to "use", though it turns out to be more complicated than I realised.

At first I thought your example was not an example of re-binding because use would create a new indentation (scope) for each use (as in C#). eg the following C# code compiles just fine, there is no clashing of names:

using (var reader = new StreamReader(new MemoryStream()))
{
}

using (var reader = new StreamReader(new MemoryStream()))
{
}

It turns out after examining this issue that F# is a different kettle of fish. Firstly, use doesn't work like the C# equivalent, instead disposing the IDisposable value at the end of its scope. So, in your example, both readers gets closed at the end of the enclosing function, though which is closed first is undefined:

use reader = new StringReader(data)
use reader = new CsvReader(reader, ...)

Another hilarious consequence of the way F# has implemented use is that because it uses shadowing, your example can't work at the module level. A hapless newcomer will paste your example code into an F# console app and be told that the use bindings will be converted to let bindings! He will get a light bulb but the code fixes offered do not fix anything.

Nevertheless, it must be said that the F# version, where it does work, does allow labels assigned using one reader to be used as imputs in another.

use reader = new StringReader(data)
let a = reader.Read()

use reader = new CsvReader(reader, ...)
let b = a

Apparently in recognition of the fact that not everyone will love use keywords that do not precisely control the extent of the use, another keyword has been introduced which is more similar to the C# equivalent, but looks different:

let result1 =
    using (new StringReader(data))
        (
            fun reader -> reader.ReadLine()
        )

let result2 = 
    using (new CsvReader(reader, ...) )
        (
            fun reader -> reader.ReadLine()
        )

Now, as you can see, the readers are in different scopes and do not shadow one another.

After reviewing this, I suspect that in explaining the benefits of shadowing, we should steer clear of use and using because of these complexities.

pbachmann commented 1 year ago

@TheJayMann

Thanks for your explanation and example, but you're making a lot of points, not all of which appear to be represented in the example code.

In my comment explaining three concepts (Reshape landscape etc.) I tried, for each concept or idea I was presenting, to focus on it entirely with explanations and examples until I was done with that concept. I thought it would be burdensome to readers to have several new ideas thrown at them at the same time.

Similarly, I wonder if you could identify a single key idea you want to get across and explain that simply? If you have the energy to explain a second concept, you could do that later.