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.87k stars 780 forks source link

Improve error reporting: Accidental use of , instead of ; as collection separator #1120

Open isaacabraham opened 8 years ago

isaacabraham commented 8 years ago

What

The following error message is generated from this code sample: -

let x = [ 1, 2, 3, 4, 5 ]
x |> List.map(fun x -> x + 5)

error FS0001: The type 'int' does not match the type 'int * int * int * int * int'

Why

Users new to F# often accidentally use , instead of ; for item separators as this is what they are used to. This is valid F# as you simply get a list with a single tupled item. You will probably get an compiler error (or an exception if you have simply indexed into the list) further down the line as you try to get at the "elements" of it. The user then has to work backwards to realise that they should have used ; instead of ,.

How

This is open to debate as to how this can consistently (and easily) be warned against. It's probably too late to do this where the real error is occurring, because (I would imagine) that by then all you have is a tuple with no context as to where it came from.

Perhaps an alternative approach would be to create a new warning which would be fired if the compiler identifies list with a single Tuple - happy to get ideas on this.

smoothdeveloper commented 8 years ago

A warning asking to put a single literal tuple in a list between parens could solve the problem, but I'll probably find it annoying at times (despite it makes code clearer whenever it used).

let a = [1,2,3,4]

warning: If you intend to use a tuple, please make it explicit by wrapping with parens: let a = [(1,2,3,4)], if you intend this to be several elements, use ';' instead of ','.

isaacabraham commented 8 years ago

That could work. I'm almost thinking that there could (should?) be a "newb mode" compiler flag which would activate warnings like this which are extremely useful for the beginning but could be turned off for the more experienced dev.

forki commented 8 years ago

Please please please no flag. We need to make the error messages good for everyone.

isaacabraham commented 8 years ago

:-) But this is the sort of warning I can see not appealing to everyone. There may be valid use cases for a list with a single item in it which happens to be a Tuple. I'm struggling to think of one - you could (should?) probably use an Option instead.

forki commented 8 years ago

yes that's why we should not do that here ;-)

singleton tuples in lists are perfectly valid use case.

forki commented 8 years ago

But maybe we can actually do something on the side of the error. I'm not completely sure what we actually have in context at this point, but maybe we find something interesting when we look deeper.

isaacabraham commented 8 years ago

But you agree that there is a valid issue in terms of people using , instead of ;?

forki commented 8 years ago

yes. It even happens to people that do F# for very long time ;-)

isaacabraham commented 8 years ago

Not just me then

dsyme commented 8 years ago

Yes, this is a valid usability concern, the "Why" explanation above is very clear.

smoothdeveloper commented 8 years ago

I think single item list with tuples benefits the warning asking to enclose those with (), really makes intent clearer IMHO.

forki commented 8 years ago

ok. I look at this. I think there is no way to solve this at error reporting site.

@dsyme what do you think about issuing a warning at collection constructor side?! (not checked if that's possible to do)

dsyme commented 8 years ago

@forki Can you pass down a context saying "we're checking an item in a list"?

forki commented 8 years ago

image

I looked at the call stack of the type checker for this (like I did in the other cases), but here we only see stuff that's inside the lambda or inside op_PipeRight. So there is no method on the stack that seems to contain any information about the list type.

But there is information about the "List.map" function.

image

So in theory we could pass context that we are using the List module. That is of course a smaller context, but it might actually help for most cases that beginners encounter.

What do you think?

isaacabraham commented 8 years ago

Can we do it for Array and Seq as well? That at least would cover the three most common collection types.

forki commented 8 years ago

I assume we would see array.map in same place. But if someone implements her own higher order function in new module it would not work On May 19, 2016 13:19, "Isaac Abraham" notifications@github.com wrote:

Can we do it for Array and Seq as well? That at least would cover the three most common collection types.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/Microsoft/visualfsharp/issues/1120#issuecomment-220296197

dsyme commented 8 years ago

@forki Yes, you're right - this is classic action-at-a-distance in type inference.

One interesting and general way to approach this is to use special annotations on solved type variables to propagate extra information through type inference. For example, the [ ... ] construct gives rise to a list<?1> type where ?1 is a type variable, later solved to be ?1 ---> (int * int * int * int * int) (using ---> to represent solved type variables). Later this type is unified with an unknown type variable "?2" coming from the signature of List.map, so you get the solution ?2 ---> ?1 ---> (int * int * int * int * int). Later, when processing x + 5 this type is then unified with int.

Now, if ?1 where magically annotated with some kind of flag or special annotation saying "I come from the element type of a syntactic list literal!", then you can imagine this context being integrated in that later unification of int and ?2 ---> ?1 ---> (int * int * int * int * int).

We actually use a whole bunch of flag-like annotations on type inference variables already - for example to record that a type arises from an error. We try to be careful not to unify this information away when solving ?1 = ?2 (however there may be cases where some annotation information is lost, particularly when we have chains of equations ?1 ---> ?2 ----> ?3 ----> ?4 ----> ...)

This is one technique to propagate information at a distance. Another would be to keep another lookaside table in the constraint solver state. For example, you could keep a lookaside table to record that ?1 came from a list literal, then check that later.

forki commented 8 years ago

Crazy. I actually understand that text

crazy

KathleenDollard commented 2 years ago

I think this is an easy problem to encounter. Overall, F# is opinionated, and if we assume we'll find a way to manage the balance between evolving opinions and back compat (MSBuild/C# do this with warning waves, as an example), then I'd like folks current thoughts on whether, in the (relatively rare?) situations where you want a single item tuple the following is reasonable to encourage?

let x = [(1, 2, 3)]
smoothdeveloper commented 2 years ago

@KathleenDollard, yes, people trip on this if they switch frequently between SQL or C# to F#, even when they know F# well, and it trips beginner, boggling them for a bit, because of seeing the type subsumption error message it falls to.

Overall, F# is opinionated,

I'd say, similarly as many other languages 🙂

then I'd like folks current thoughts on whether, in the (relatively rare?) situations where you want a single item tuple the following is reasonable to encourage?

I don't know how hard the type checking stuff is to add the warning (please see my proposal https://github.com/dotnet/fsharp/issues/1120#issuecomment-214747599), but it seems "doable to me" if time is put on it by a contributor.

What I understand from glancing at this old issue is that for single item arrays or list literals, we would want a warning no matter if there is a type subsumption error.

smoothdeveloper commented 2 years ago

I'll add, about @isaacabraham "newb mode":

This sound like the "newb mode" we want baked in (and which is baking) F# tooling; in that respect, in agnostic context, I think F# is already friendlier than average compiler.