dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.85k stars 776 forks source link

Improve error message for: "The type 'int' does not support the operator 'DivideByInt'" #83

Closed KevinRansom closed 5 months ago

KevinRansom commented 9 years ago

This was originally opened on CodePlex by latkin

Today this happened to me:

Microsoft (R) F# Interactive version 12.0.20809.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> [|2;4;6;8|] |> Seq.average;; 

> [|2;4;6;8|] |> Seq.average;;
  ---------------^^^^^^^^^^^

stdin(1,16): error FS0001: The type 'int' does not support the operator 'DivideByInt'

I was surprised to discover that int does not support DivideByInt. Since many times I have successfully divided ints by other ints.

I think we should either:

comments dsyme wrote Jun 3, 2014 at 6:02 AM +1 for a better error message

forki commented 9 years ago

So Seq.average is only expected to work with floats?

vladima commented 9 years ago

Nope, it works on any type that:

type Number(v: float) =
    member val Value = v
    static member DivideByInt(x: Number, n: int) = Number(x.Value / float n)
    static member (+) (a: Number, b: Number) = Number(a.Value + b.Value)
    static member Zero = Number 0.0

let s: seq<_> = upcast [Number 10.0]

let r = Seq.average s
forki commented 9 years ago

Ah Ok and we can't implement DivideByInt for ints because it would have to return float to make it useful, right?

dsyme commented 9 years ago

That's correct

zpbappi commented 8 years ago

@dsyme you've made it up for grab. I am not quite clear about the expected behavior. As far as I can see, DivideByInt for integers have to return float to be accurate. Please confirm if that is the expected behavior. I would like to grab this one.

KevinRansom commented 8 years ago

@zpbappi , I'm going to close this the current error message and behaviour is at least explainable. Even if averaging a collection of ints seems like something the average API should actually be able to accomplish.

Kevin

rlundy commented 3 years ago

This caught me unaware today, and I had to go hunting for an explanation for this error message. How can DivideByInt not be valid for an int? If I can't average ints with List.average, at least the error message ought to explain why. This error message doesn't.

cartermp commented 3 years ago

You can't because 3/2 is not an int, but the current signature requires the types to be the same:

https://github.com/dotnet/fsharp/blob/cee7bb4f273b135887e74fc2d222ed7c3b7ab3e3/src/fsharp/FSharp.Core/seq.fs#L1185

I'll re-open this old issue to track improving the error message. It's not clear.

pbachmann commented 3 years ago

Thankfully, .NET has been integrated nicely and can substitute here ...

let average = [2;4;6;8] |> System.Linq.Enumerable.Average

Martin521 commented 9 months ago

So, I have set myself to close some of these very old issues on error messaging.

For this one here, let me first list some thoughts.

First, in hindsight, I think the average functions of List, Array, Seq and QueryBuilder should have had result type float. I am not sure if everybody agrees, but anyway, it is too late to change it.

Second, it is not really clear what it is about the current error message that people struggle with.

  1. Do they wonder why int should support the operator?
  2. Are they stuck wondering why an operator called DivideByInt does not work on int? (Before even getting to question 1)
  3. Do they assume average should have return type float?
  4. Do they assume average should return the closest int for int collections?

Third, I doubt that DivideByInt has ever been used in places other than average functions. At least in the whole of fsprojects this is not the case.

So, what does all of this mean for this issue?

If we can assume that the inference of the operator comes from some average function, we can answer all of the above four questions (specifically for the DivideByInt operator and int) by an error message such as

"The type int has been used in the context of a generic function such as average that requires the generic operator DivideByInt: 'T -> int -> 'T, which in the case of 'T = int does not have obvious semantics and has therefore not been implemented for int".

This error message would not be too difficult to implement, but

It still may be completely sufficient for closing this issue.

If, however, this issue was about question 1, i.e. about getting a clue what type inference did here, then we would need an error message like

"The type int does not support the operator DivideByInt as required by Seq.average"

It would indicate the source of the constraint (for any constraint type and constraint source). This is much harder to implement, but I got a PoC for it already (after finding out a lot about type checking, metadata, tcimports etc). And it could be useful in other situations also.

So I see three ways to handle this issue.

  1. Close it as is, because the error message is good enough (but this has been tried before).
  2. Implement the specific error message for DivideByInt<int>.
  3. Implement the "constraint source hint".

Before starting a PR, I'd like to hear advice on which way to go.

rlundy commented 9 months ago

Second, it is not really clear what it is about the current error message that people struggle with.

  1. Do they wonder why int should support the operator?
  2. Are they stuck wondering why an operator called DivideByInt does not work on int? (Before even getting to question 1)
  3. Do they assume average should have return type float?
  4. Do they assume average should return the closest int for int collections?

Speaking for myself, it's the second point: It doesn't make sense why an int can't be divided by an int. To be sure, an int can't necessarily be divided evenly by an int. But we've all done integer division before (5/2 = 2), yet for some reason, according to the error message, normal integer division is broken in this specific case.

We run a mathematical risk if we blithely average 2, 4, 6, and 8 and truncate to int. But we also run a risk if we divide 5 by 2. So why does the average give us an error, and not the division? The inconsistency creates confusion.

Look at the docs for DivideByInt:

If a type supports DivideByInt, the type supports exact division (floating-point division), rather than integer division, which rounds down to the nearest integer result.

So if a type supports DivideByInt, then it doesn't support integer division!? 😵

Something like DivideToFloat would be a more logical name for this function.

In lieu of that renaming, a better error message would be very helpful.

Granted, after people run into this error, they probably search and find out the reason and then don't have to wonder anymore. By the same token, someone who's lived in a small town for twenty years doesn't need street signs to find his way around. But that's not whom the street signs are for.

Martin521 commented 9 months ago

@rlundy thank you for your comment. I do understand the frustration with the current situation, but I don't think we can change the existing names or signatures and introduce breaking changes.

Also, I don't think that DivideToFloat would be a better name for the given operator, because its signature is 'T -> int -> 'T, so the return type can, for example, be decimal. The root issue is indeed the return type 'T of both the average functions and DivideByInt. It should have been float. But that's to late to change.

That's why I decided to work on the error message. Question is which one.

pbachmann commented 9 months ago

@Martin521

How about just adding a few words to the end?

error FS0001: The type 'int' does not support the operator 'DivideByInt'. F# is strict, try another type.

pbachmann commented 9 months ago

It would be better if the new bit is separated from the rest with a colon:

error FS0001: The type 'int' does not support the operator 'DivideByInt': F# is strict, try another type.

smoothdeveloper commented 9 months ago

@Martin521:

@rlundy thank you for your comment. I do understand the frustration with the current situation, but I don't think we can change the existing names or signatures and introduce breaking changes.

I think changing the signature of average function to not resort on DivideByInt wouldn't necessarily be a breaking change (code which compiles/run today would still do), but changing DivideByInt constraint would be, because it is a public member of FSharp.Core.

Implement the "constraint source hint".

Do you mean, a way for the compiler to print the definition of the constraint? a bit the same the compiler prints the list of overload when you fail to pick one:

TaskFactory().StartNew "123"

No overloads match for method 'StartNew'.

Known type of argument: string

Available overloads:  - TaskFactory.StartNew(action: System.Action) : Task // Argument 'action' doesn't match  - TaskFactory.StartNew<'TResult>(function : System.Func<'TResult>) : Task<'TResult> // Argument 'function' doesn't match

If so, could you show a full example of what it would render for DivideByInt<int>?

Martin521 commented 9 months ago

Yes. The most simple version would be an extension of the current error message, like I proposed above. For the example code in the original post of this issue that would be

"The type int does not support the operator DivideByInt as required by Seq.average"

We could make it more wordy if that was considered helpful.

smoothdeveloper commented 9 months ago

@Martin521 I think it should provide the detail of the constraints that don't match, it would help all messages around failed constraints rather than this specific case.

I know it was major driver for C++ concepts: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2429r0.pdf https://en.cppreference.com/w/cpp/language/constraints

We could take same idea as overload error message, and list each constraint that fail on separate lines.

If the state of resolving constraint in the language allows to resolve with type extension, we could also guide user to define the type extensions that fits the missed constraints, which would make the code compile.

To me it is not more wordy, but more informative 🙂.

Martin521 commented 9 months ago

Yes, the fix can be made for all constraints, not just operator member constraints, if that is what you mean. And the details are already present in the message, or am I missing something?

I am not sure, though, if guiding users to define type extensions makes sense here. The constraints are typically there for a reason. This is different from overloads, where defining a new one does not change the semantics.

smoothdeveloper commented 9 months ago

if guiding users to define type extensions makes sense here

but does it make sense to special case for int, and have extraneous logic in the compiler, just for Seq/List/Array.average?

For this, it would be better if there was a mechanism for library authors, to customize constraint failure message, based on type predicate for example, the message would come as prelude before listing the failed constraints.

For suggesting to define the constraint extensions, I think it guides the user towards fuller understanding also, even for this message.

Martin521 commented 9 months ago

but does it make sense to special case for int, and have extraneous logic in the compiler, just for Seq/List/Array.average?

Not sure what you mean here. There is no special case for int, neither in the compiler, nor in the planned fix, nor in the core library.

smoothdeveloper commented 9 months ago

What is this planned fix?

The type int does not support the operator DivideByInt as required by Seq.average

I'm for adding the expression in this message, but I may be missing something, as I don't think it helps the user a lot more to understand.

Maybe:

Seq.average requires (source: seq<'a>) where a :> DivideByInt, but int doesn't support constraint(s) defined by DivideByInt.

This would improve current situation for all constraint mismatch, while remaining probably simple enough.

In another pass, we would list the failed constraint and suggest type extension.

Martin521 commented 9 months ago

What is this planned fix?

I shouldn't have said planned fix but proposed fix. That's option 3 in my lengthy post above, which would provide the constraint source at the point where the error is created.

So we could do (picking up on your proposal):

This expression has type int, which does not match the constraints of 'T that it was inferred to have from Seq.average: seq<'T> -> 'T (requires static member ( + ) and static member DivideByInt and static member Zero )

(Note that this uses the current compiler's formatting for the constraints. Possibly changing it to the when syntax would be a separate issue.)

smoothdeveloper commented 9 months ago

@Martin521, this would be good, but I feel then, the effort to actually filter out the constraints that are matched, and list them on separate lines (like overload resolution) would really really help for usage of SRTP in general sense, and remove clutter from the constraint definition, especially in more complex cases.

For clarity, I would also mention the argument name (if it exists).

Martin521 commented 9 months ago

Makes sense. Thanks.

So we have 4 options now.

If anybody has opinions on which way to go, please speak up. Else I will publish my PoC as a draft PR so people get an idea of what the implementation of the third/fourth option would mean.

smoothdeveloper commented 9 months ago

I vote for:

'int' does not match the constraints of 'T that it was inferred to have from 'Seq.average'.

'Seq.average' has signature 'source: seq<'T> -> 'T '.

'int' doesn't meet constraint(s):

  • static member DivideByInt : 'T -> int -> 'T
  • ... // assuming it lacks several for other cases, otherwise put on the same line as "'type' doesn't meet constraint: static member DivideByInt: 'T -> int -> 'T"

and if it is too painful to filter the unmet constraints by member, it would list all, but with detailed signature still.

pbachmann commented 9 months ago

I think my suggestion of

The type 'int' does not support the operator 'DivideByInt': F# is strict, try another type.

should be added to the four options and I would vote for that one. Of course, I would be happy to alter it based on constructive feedback.

Perhaps @Martin521 did not include my suggestion because it appeared so off-beat that it did not deserve consideration or comment. So maybe I should explain my reasoning.

I agree with the opening comments made by @Martin521:

First, in hindsight, I think the average functions of List, Array, Seq and QueryBuilder should have had result type float. I am not sure if everybody agrees, but anyway, it is too late to change it.

I also agree that Martin was right to focus on "What it is about the current error message that people struggle with." However, the four suggested answers do not, in my mind, answer that very important question.

I suggest a way to best answer that question is by putting oneself in the position of a newcomer to F#. A newcomer might go to F# interactive and type:

[1..3]

and be delighted to see...

val it: int list = [1; 2; 3]

Then they try:

[1..3] |> List.sum

and are further delighted to see

val it: int = 6

Then they try

[1..3] |> List.average

and get

stdin(7,11): error FS0001: The type 'int' does not support the operator 'DivideByInt'

I don't think they're thinking, "Why doesn't DivideByInt not work on int?" or "What should the return type be?" etc. They are thinking things like:

It is this scenario that my suggestion seeks to address. Let's break it down:

The type 'int' does not support the operator 'DivideByInt': F# is strict, try another type.

The first bit just keeps the error as it is. I did this so as not to offend the experts who do understand Statically Resolved Type Parameters and who might benefit from this information.

However, as @rlundy sagely suggests:

... someone who's lived in a small town for twenty years doesn't need street signs to find his way around. [...] that's not whom the street signs are for.

So what to do about the fellow new to town, swearing his head off? I thought that such a person really needs a gentle tap on the shoulder, and a pointer in the right direction. Specifically, we should help him:

  1. Understand, in general terms, what has caused the problem.
  2. Tell him what to do next.

So with regard to (1), I say "F# is strict." This would hopefully translate, in the mind of the newcomer, to something like:

"F# is a stricter language than you might be used to. There are good reasons for that that we won't bore you with now, but hopefully you can just accept it and be guided by it. As you continue to use F#, watch out for examples of F#'s strictness and see how they actually help you to reduce bugs etc."

With regard to (2), I say, "Try another type."

Here I was constrained by needing to say something useful even though the error would appear in many places where I wouldn't know the specific details. "Try another type" seemed the best catch-all, and hopefully prompts the newcomer to try something like:

[1.0 .. 3.0 ] |> List.average

Hooray, right? He is on his way, and can save learning SRTPs for some other day.

smoothdeveloper commented 9 months ago

Perhaps @Martin521 did not include my suggestion because it appeared so off-beat that it did not deserve consideration or comment.

I don't like to assume about others. For me, your suggestion is a special case on int only for this function, it is not a generic solution for all "constraint not met" situations.

I also opened the discussion about providing library authors with a way to provide such customized message, for specific type.

Quoting myself here:

For this, it would be better if there was a mechanism for library authors, to customize constraint failure message, based on type predicate for example, the message would come as prelude before listing the failed constraints.

Martin here:

I think the average functions of List, Array, Seq and QueryBuilder should have had result type float.

But this is a breaking change, and for decimal, it should give decimal, for float32, a float32, etc. This won't change, and it won't help to change.

pbachmann here:

They are thinking things like:

They are very emotional then, or maybe you are making assumption about users, but they are not all like this :)

The suggestion I make is:

pbachmann commented 9 months ago

@smoothdeveloper,

For me, your suggestion is a special case on int only for this function, it is not a generic solution for all "constraint not met" situations.

To be clear, I only meant 'int' and 'DivideByInt' as examples. The current substitute mechanism would obviously be retained:

The type '{type in question}' does not support the operator '{operator}': F# is strict; try another type.

I also opened the discussion about providing library authors with a way to provide such customized message, for specific type.

I guess that has possibilities - but you seem to be opening up a whole new area where the compiler spits out error messages written by people who write ordinary F# constructs like SRTP. It's a big deviation from how things have been traditionally done and would need to be justified with big benefits. Do you have a suggestion for what the "customized message" for the [1..2]> List.average situation would look like?

They are very emotional then, or maybe you are making assumption about users, but they are not all like this :)

Emotional is part of being human. Maybe a greater willingness for designers to become aware and deal with the typical human responses to complexity might increase the number of people using technology. Apparently the number of F# users is currently one percent of C# users.

The suggestion I make is....

I note your suggestion, and read similar previously. My response in summary is that I think "great details" are not necessary for experts as they have enough information, and "great details" would only confuse new users who, as I described, are likely to be already overburdened with what, as far as they are concerned, are red herrings.

Since you have dismissed my insights as mere speculation and seem convinced of your own facts, it probably doesn't make sense for us to keep talking. Our positions are reasonably clear. Let's hear what @Martin521 and others think.

Martin521 commented 9 months ago

First, sorry @pbachmann that I did not react to your contribution, I forgot about it during the other discussion. I am not so sure if the exact wording of your error message will be effective for most users, but you certainly have opened another category of solutions to this issue, so I am adding it.

So we have 5 options now.

To add also my personal opinion, I believe that the compiler error messages should be concise unformatted one-liners. Additional information, explanation, hints can be put onto the respective compiler-message page that your IDE points you to (here is an example). But I am open to whatever the outcome of this discussion will be, I am mostly interested in implementing a solution and closing this issue.

smoothdeveloper commented 9 months ago

To be clear, I only meant 'int' and 'DivideByInt' as examples. The current substitute mechanism would obviously be retained:

thanks, I really didn't understand it like this. I still find the tone, and no extra information not helping user make progress, but it is just my opinion, not going to tip the balance any more than another opinion.

It's a big deviation from how things have been traditionally done and would need to be justified with big benefits.

If the message you suggest is generic (not for Seq.average for int), I agree that what I'm devising is a deviation from simplest "let's adjust error message", but:

Maybe a greater willingness for designers to become aware and deal with the typical human responses to complexity might increase the number of people using technology.

Are you implying that F# is worse than average, and that many other languages are good compared to F# in this regard?

Please consider showing examples so we can take something actionable to improve on.

There are few languages that are good and showing the example we follow, but as far as I experienced, the main languages (C++, java, C# to pick a few, and despite there are several impls for C++) don't offer better error messages or diagnostic than F#, you may not fully appreciate this.

Apparently the number of F# users is currently one percent of C# users.

I don't see the relation you are trying to establish, from my understanding, C# users don't spend time commenting a lot about bad error messages towards better suggestions, nor they contribute to improve them as much, comparatively to what F# community has been doing for years.

I helped onboard people to contribute in doing those changes, and contributed several also, so I think your comments don't apply as much as you think.

It doesn't mean we've reached perfection, but messages are generally reviewed, and improved, constantly.

This is my invitation to you to achieve the same also, if you feel the desire 🙂.

I think "great details" are not necessary for experts as they have enough information

They are always useful, there is not enough information because the tooling around constraints in tooltips, etc. is not as good as other areas. For example, the overload resolution error, in F#, is IMO, much better than what C# offers, and helpful to experts like novices. I'm biased as this is an important one I changed.

Of course, I'm suggesting similar approach for the constraints not met, as I think it is exactly the same problem space.

Since you have dismissed my insights as mere speculation

Until there is a PR or @dsyme comes up with a message he wants, I think we are all speculating, please don't take it personally :)

I don't intend to dismiss anyone, I tried to provide guidance (from the trenches of having improved and discussed similar improvements in many occasions) and @Martin521 has done a great job to not dismiss anything.

To clarify:

pbachmann commented 9 months ago

I believe that the compiler error messages should be concise unformatted one-liners.

I agree.

I am not so sure if the exact wording of your error message will be effective for most users,

I have already provided an explanation of what I hope the thought process of users reading that message would be, but perhaps I might suggest another question to focus our minds to guide us to a final decision.

Q: Leaving aside what we want them to hear, what do we want users to do when first confronted with our message?

The most compelling answer for me would be for them to try to change the type of the list elements. eg by:

Once they have had success in seeing it work with floats or decimals etc., they will feel good about themselves and eager to keep going. They might try it with strings, and see that it doesn't work - all the better for learning the purpose of the constraint.

I'm happy, of course, to hear others answers on what we want the user to do.

Now, should we accept that changing the type is what we want the users to do, the question then becomes, "What message is most likely to get them to do that?"

I agree @Martin521 that we should be working to close this issue promptly, it is almost 10 years old. I suggest that makes us duty-bound to focus sharply on:

smoothdeveloper commented 9 months ago

To add also my personal opinion, I believe that the compiler error messages should be concise unformatted one-liners.

Many languages have taken different direction, and are lauded for how precise and helpful the messages are, Rust is one.

The issue with one-liners and relying on tooling: disparate and when the tooling is not up to the task, requires more work to fix

There can be option to shorten to one line, as longer messages always will have the introductory line.

I think the overload resolution error is great example of how helpful it is, in surveying the whole space without having to click on small tooltips, and little arrows in them, that may lose the track if you move the mouse, it is all displayed in the error message, it works in all editors, even though the tooling in the editor may not provide the best support for it.

It is ok to have diverging opinions, no one is going to die for F# evolving one direction or another :)

One picture > thousands words:

https://github.com/dotnet/fsharp/pull/6596#issuecomment-488733133

pbachmann commented 9 months ago

@smoothdeveloper

Thanks for the reply but I feel our conversation has meandered off into the marshes and to reply in detail would simply bog this thread down further.

My purpose here is to help identify a message that we agree is much better than the current one, or alternatively make a recommendation that the status quo is as good as it reasonably gets. Either way, I agree with what I understand @Martin521 to be saying: that we should work quickly and effectively so the issue can be closed soon.

pbachmann commented 9 months ago

@smoothdeveloper nb. While I agree with Martin about one-liners being better, I accept your illustrations that multi-line messages under a single heading can be useful.

rlundy commented 9 months ago

Honestly, I'm still up in the air about whether the problem is the unclear error message or the unreasonable constraint. If the change has to be with the error message, then I think it might help to say something about how DivideByInt is usually implemented on types that divide evenly.

But as the original opener of the issue said:

I was surprised to discover that int does not support DivideByInt. Since many times I have successfully divided ints by other ints.

This seems like a case of F# being too careful for its own good. Wouldn't it be trivial to implement DivideByInt for ints and have it do integer division? It would be inaccurate for some sums, sure, but that's just the nature of integer division. Chances are decent that if I'm averaging a set of ints, I know they'll come out evenly. If not, testing will show me what's happening. If F# is concerned about the inaccuracy, then it shouldn't allow integer division either. This is an inconsistency in the language.

smoothdeveloper commented 9 months ago

@rlundy

or the unreasonable constraint.

I don't think it is unreasonable, and the name is also correct, "divide a number by int".

The reason it is not unreasonable, is the stance on numerical coercion safety of F#, and not incurring accidental integer division, thing which happens in majority of languages, and can have unexpected rounding consequences in more complex code. C++, or C# have those issues. F# is designed to make those coercion possible only when they can't have such consequences.

The signature is 'number -> int -> 'number, and averaged int can't be an int, despite the underlying implementation is division, and integer division are a thing.

The suggestion to turn the output into float is also not satisfying, it would be more correct to Seq.averageBy float (which seems better than System.Linq.Enumerable.Average for the explicitness, and ability to return decimal or other types that suppor the constraint.

Wouldn't it be trivial to implement DivideByInt for ints and have it do integer division? It would be inaccurate for some sums

Then it wouldn't be an average operation anymore :) I think I'm getting the feeling of being a mathematician when I say this, I should be careful.

If one wants integer division, one writes an integer division, if one wants rounding of an average of integers, first convert to a type that supports that mathematical operation, then do the rounding.

Yes, F# is a strict in this way and others, and it is considered good, especially in space where it is used for heavy number crunching, with complex formula, this safety compounds to great effect, compared to C++ or C#, which would need to resort on analyzers, lints, or advanced constructs, etc. just to get there.

pbachmann commented 9 months ago

@rlundy

If F# is concerned about the inaccuracy, then it shouldn't allow integer division either. This is an inconsistency in the language.

I see your point that it seems odd that F# should allow 5 / 2 = 2, yet insist List.average [1;4] be incalculable.

@smoothdeveloper's counter-argument seems to be that average has some kind of "rounding" characteristic that distinguishes itself from raw integer division. While I admire his remarks about F#'s strictness, eg. "this safety compounds to great effect", I don't know whether he invalidates what you are saying.

I wonder if it might be best to take a practical approach and ask: If F#'s int/int64 (int16?) suddenly supported DivideByInt, what would be the practical consequences for developers?

Martin521 commented 9 months ago

Ok, after diving deeper into this, I see that the field is larger than I thought. To share that broader view (which certainly is still incomplete), I have prepared some more examples. Further below you can find for each example

the (erroneous) code
=> the current error message
==> a proposed new succinct error message with additional information, meant to clarify the error for non-beginners
===> an explanation of the error

The explanation could go to a separate web page (similar to this one, but parameterized), or be requested and shown by the IDE, or (as @smoothdeveloper would prefer) just go into the error message.

So, here are the examples.

let a = List.average [2; 3]
// => The type 'int' does not support the operator 'DivideByInt'
// ==> The type 'int' does not support the operator 'DivideByInt' as required by 'List.average'
// ===> 'List.average' is defined as a generic function.
//      The returned average has the type that the list elements have.
//      This is powerful in that it can now work for different types such as 'float' and 'decimal',
//      but also limited, as it can work only for element types that can express their own average.
//      The latter is not the case for type 'int', because the average of 2 and 3 is 2.5, which is not 'int'.
//      Technically, 'List.average' has signature 
//        'list:'T list -> 'T
//             when 'T : (static member (+) : 'T * 'T -> 'T)
//             and  'T : (static member DivideByInt : 'T * int -> 'T)
//             and  'T : (static member Zero : 'T)
//      So, internally, 'List.average' uses the above members of the element type 'T when computing the average.
//      Types such as 'float' and 'decimal' have all of the above members, including 'DivideByInt', while 'int' does not.

let b = List.sum ["hello"; "world"]
// => The type 'string' does not support the operator 'get_Zero'
// ==> The type 'string' does not support the operator 'get_Zero' as required by 'List.sum'
// ===> 'List.sum' is defined as a generic function that can sum up elements of types that allow it.
//      Technically, 'List.sum' has signature
//        'list:'T list -> 'T
//             when 'T : (static member (+) : 'T * 'T -> 'T)
//             and  'T : (static member Zero : 'T)
//      So, internally, 'List.sum' uses these members of the element type 'T when computing the sum.
//      'string' has a static member (+), but not Zero.
//      For concatenating strings, consider using 'String.concat'.

let c = -1u
// => The type 'uint32' does not support the operator '~-'
// ==> The type 'uint32' does not support the unary operator '-'

let d = 1 + 1.
// => The type 'float' does not match the type 'int'
// ==> The type 'float' does not match the type 'int' as required by operator '+'
// ===> The generic operator '+' requires one of the operand types to have a static member (+).
//      Type inference found such a member in the type 'int' of the first operand.
//      'int' has
//        static member (+): int * int -> int
//      Therefore the second operand must also be 'int'.

let now = System.DateTime.Now
let e = now + now
// => Type constraint mismatch. The type 'System.DateTime' is not compatible with type 'System.TimeSpan'
// ==> Type constraint mismatch. The type 'System.DateTime' is not compatible with type 'System.TimeSpan' as required by statix member '(+)' of 'System.DateTime'
// ===> The generic operator '+' requires one of the operand types to have a static member (+).
//      Type inference found such a member in the type 'System.DateTime' of the first operand, namely
//        static member (+) : DateTime * TimeSpan -> DateTime
//      Therefore the second operand must be 'System.TimeSpan'.

let f = "" <<< 2
// => The type 'string' does not support the operator '<<<'

I have not proposed new error messages or explanations for all cases. For example, case f looks perfectly ok to me. Also note that not all of the messages might be easily implementable, so some simplification may be necessary. But we can postpone such iterations and first look for the ideal error message.

I am now looking for

before going back to implementation.

psfinaki commented 9 months ago

Thanks for the extra examples. Yeah it's a bit of a bummer, IMO the message in the case d is fine whereas the one in the case b would be quite obscure for newcomers.

One thing that comes to my mind is to actually reduce the amount of detail. As in, in all those cases, the message would say "The type 'X' does not support the required operation". This is not perfect but arguably better - there is less confusing information and the developer will be then probably thinking "hmm so the operation is List.sum and the type is string... ah right, sum is probably for numbers then, so there should be some concat function for strings". Probably. Maybe they will be just equally confused :)

Alternatively we could have some branching for the message text logic depending on the operation but that sounds brittle, complicated and probably not worthy.

vzarytovskii commented 9 months ago

One thing that comes to my mind is to actually reduce the amount of detail. As in, in all those cases, the message would say "The type 'X' does not support the required operation".

I don't think we should, it will be very confusing and counter-productive for people who know what they're doing and will likely want to see that. We shouldn't think of users as if they're stupid, and oversimplify things in sake of them being simple.

I, again, think, it should be two messages. One short one and one which explains things in details.

vzarytovskii commented 9 months ago

But again, I can't stress it enough, we should not make it more obscure by simplifying it. I.e. we shouldn't be downgrading it to just "Type 'X' does not support this operation".

pbachmann commented 9 months ago

@vzarytovskii ,

I don't know whether I should bother commenting further as it seems like a done deal. The code involved in the Pull Request is clearly aimed at providing extra information rather than reducing it. It looks like the train has left the station.

I want to say in passing however, that I find @psfinaki 's comments very insightful and useful. I interpret his lack of stridency as a strength, not weakness. I also like how he focused his attention on the person reading the messages: eg. "What will the developer do if presented with such info? Probably this. Maybe that." psfinaki sets a good example, if you ask me.

As an aside, and in the spirit of "exploration is better than argument", I have compiled a table of the various suggestions made in this thread (even outlandish/improbable ones like, "make average return a float"), along with their likely consequences. I won't post the table here unless requested, but I can say that it helped me see things more clearly and change my mind about a few things. eg. I now think that even if it were possible to change it, it makes sense to me that the average of type X should also be of type X.

Martin521 commented 9 months ago

Thank you @pbachmann for staying involved. But, yes, I agree that the discussion is converging towards adding information. I will soon make a proposal for more definite next steps.

psfinaki commented 9 months ago

@pbachmann thanks for you words :)

Just to reiterate, my suggestion was in no way a hill I want to die on, just a fresh perspective from someone who has just looked at the issue. My point is that this error can happen to newcomers and hence the error text should be clear to newcomers. As long as we make a step in that direction, I am happy.

pbachmann commented 9 months ago

@psfinaki,

If you don't want to die on the hill, I suppose you have to get off the hill. The artillery that has been directed at the hill with the sign on it saying, "Newcomers are important F# users whose points of view need to be considered", has been unrelenting. @vzarytovskii has made it abundantly clear what he thinks of your "fresh perspective".

Even though you would be happy for a single step to be taken, I worry that this will not be achieved and that this analogy itself will be seen as inflammatory, ie. a small step towards newcomers' needs being seen as a step away from addressing regular F# users' needs.

It seems to me that in order to make everyone happy, a new methodology needs to be adopted: one that sets out to make everyone happy.

vzarytovskii commented 9 months ago

@psfinaki,

If you don't want to die on the hill, I suppose you have to get off the hill. The artillery that has been directed at the hill with the sign on it saying, "Newcomers are important F# users whose points of view need to be considered", has been unrelenting. @vzarytovskii has made it abundantly clear what he thinks of your "fresh perspective".

All I've been trying to say is that we shouldn't try to simplify messages at the cost of experienced users being robbed of useful information.

I don't think folks who work with language a lot will appreciate that instead of telling them what exactly they need to do, we will just tell them "Hey, this doesn't work, try something else".

In other words, I think we should always prefer diagnostics which have actionable information ("constraint X is missing on type Y") as opposed to just having message about what's wrong ("Type Y doesn't work with List.average, please try another one").

pbachmann commented 9 months ago

@vzarytovskii, As far as I can tell, I understood and represented your point of view accurately. That is, you don't want "experienced users to be robbed of useful information" just because the excess information confuses newbies. You are understandably concerned about the prospect of experienced users complaining about "FSharp is strict. Try another type" kind of messages.

What I liked about @psfinaki's comments was that, while not disrespecting the point of view of the experienced users, he seemed to have a genuine interest in why newbies were confused by the more detailed messages.

As I suggested in the final line of my last comment: Isn't it a matter of satisfying both newbies and experienced users simultaneously? I am genuinely trying to help here - feeling that empathetic changes to error messages might have a big impact on how new users perceive the language, and ultimately how many adopt it.

In other words, I think we should always prefer diagnostics which have actionable information ("constraint X is missing on type Y") as opposed to just having message about what's wrong ("Type Y doesn't work with List.average, please try another one").

Above is another example of where I feel I have represented you accurately and where I humbly suggest you're jumping the gun: I suggest an examination of newbie confusion should come first. Only after that has been settled will we have a solid evidence base for saying, "We should always prefer this over that... etc."

vzarytovskii commented 9 months ago

As I suggested in the final line of my last comment: Isn't it a matter of satisfying both newbies and experienced users simultaneously?

That's pretty much what I was suggesting both here and in the linked issue. Instead of just simplifying diagnostics, we can do both. Provide simple message as well as details which can be helpful to more experienced users.

Above is another example of where I feel I have represented you accurately and where I humbly suggest you're jumping the gun: I suggest an examination of newbie confusion should come first. Only after that has been settled will we have a solid evidence base for saying, "We should always prefer this over that... etc."

I don't see it that way honestly, I usually try to speak from experience as well as feedback I see from existing and new users (in Slack, Discord, SO, Telegram, Twitter, etc).

That said, I think we could learn a lot from Elm and Rust diagnostics, where they provide good information for both categories of users.

pbachmann commented 9 months ago

I don't see it that way honestly, I usually try to speak from experience as well as feedback I see from existing and new users (in Slack, Discord, SO, Telegram, Twitter, etc).

That conjures up a picture of a design methodology dominated by intellectual titans, each with their vast repositories of experience and information, offering competing, well-thought-out proposals.

You're quite right to suggest we do not see it the same way. I have previously, and continue to maintain, that simple focus areas be set up that anyone can contribute to. All that would be required from participants is to be focused.

For example, I suggested previously that two questions were important:

A focus area would be set up and contributions made in parallel. I guess we are limited a bit because, being from around the world, not everyone is available at the same time. Nevertheless, I'm sure allowances could be made.

A focus area could be set up along the following lines:

"What might we want new users to do when they see the error?

Then participants contribute answers like:

From there there might be further focus areas like:

The process continues until there is a well-rounded and broadly supported decision on what the error message should look like.

smoothdeveloper commented 9 months ago

ChatGPT is a great learning tool:

image