Crystal: Better error message

[mirek]

This kind of compiler errors are hard to parse by humans:

type must be (Array(Array(JSON::Type) | Array({Float64, Float64}) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | PG::PGPath | String | Time | {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}} | {Float64, Float64, Float64} | Nil) | Array(JSON::Type) | Array({Float64, Float64}) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | PG::PGPath | String | Time | {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}} | {Float64, Float64, Float64} | Nil), not Slice(UInt8)

It would be nice to have some kind of diff, maybe +/- prefixed lines with single types?

[bcardiff]

Kind of long!
I don't understand what do you mean by some kind of diff with respect this example. Could you clarify?

A "pretty" print would be something like i think,

type must be (
  Array(
    Array(JSON::Type) | 
    Array({Float64, Float64}) | 
    Bool | 
    Float32 | 
    Float64 | 
    Hash(String, JSON::Type) | 
    Int32 | 
    Int64 | 
    PG::Numeric | 
    PG::PGPath | 
    String | 
    Time | 
    {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}} | 
    {Float64, Float64, Float64} | 
    Nil
  ) | 
  Array(JSON::Type) | 
  Array({Float64, Float64}) | 
  Bool | 
  Float32 | 
  Float64 | 
  Hash(String, JSON::Type) | 
  Int32 | 
  Int64 | 
  PG::Numeric | 
  PG::PGPath | 
  String | 
  Time | 
  {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}} | 
  {Float64, Float64, Float64} | 
  Nil
), not Slice(UInt8)

But I haven't come across many of this cases.

[jhass]

Maybe swapping is enough? type is Slice(UInt8), expected ...

[mirek]

Pretty printed looks more readable... well regarding the diff it's not the best example of an error that I gave... previous error that I've lost was more interesting - after "not ..." there was a long list of types (on the left as well).

I had to copy paste it to editor, break new line on "not..." and go one by one. There was difference with single type entry.

Maybe I can reproduce it.

I think if you use aliases (sum types / nested) then for some reason it lists types on both sides. I think it shouldn't duplicate types on left hand side and right hand side (splitted by "..., not ...").

[mirek]

Btw. is the information about aliases lost at the time when the error is generated? It would be small list if aliases were preserved.

[mirek]

Ok, got the long error:

type must be (Array(Array(JSON::Type) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | String | Time | Nil) | Array(JSON::Type) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | String | Time | Nil), not (Array(Array(JSON::Type) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | String | Time | Nil) | Array({Float64, Float64}) | Bool | Char | Float32 | Float64 | Int16 | Int32 | Int64 | JSON::Any | PG::Numeric | Slice(UInt8) | String | Time | UInt32 | {Float64 | Symbol | {Float64, Float64}, Array({Float64, Float64}) | Float64 | {Float64, Float64}} | {Float64, Float64, Float64})

Aliases are (more less):

  alias PGPoint = Tuple(Float64, Float64)
  alias PGLine = Tuple(Float64, Float64, Float64)
  alias PGPolygon = Array(PGPoint)
  alias PGBox = Tuple(PGPoint, PGPoint)

  class PGPath
    def initialize(is_closed : Bool, polygon : PGPolygon)
      @is_closed = is_closed
      @polygon = polygon
    end
  end

  alias PGOther = Slice(UInt8)
  alias PGGeom = PGPoint | PGLine | PGPolygon | PGBox | PGPath
  alias PGInt = Int8 | Int16 | Int32 | Int64
  alias PGUInt = UInt8 | UInt16 | UInt32 | UInt64
  alias PGNumeric = PGInt | PGUInt | Float32 | Float64 | PG::Numeric
  alias PGScalar = Nil | PGOther | Char | String | Bool | PGNumeric | Time | JSON::Any | PGGeom
  alias PGValue = PGScalar | Array(PGScalar)

...so it could be less verbose if error message included alias names instead of resolved sum types.

[ozra]

Having a sort of Levenshtein on type-mismatches and signature matches would be a goldmine error reporting-wise! :-) Most often signature matches (slightly off topic, sorry) consists of an arg being Type? instead of Type. With a "find closest match" algo, that signature could be lifted to the top and say "did you mean..."
In the above type example that would give the same benefit, prioritizing the branches order.

[paulcsmith]

I also think @jhass has a point that showing the actual type first and then the possible types would clarify things when the list of possible types is long.

The "did you mean" idea sounds fantastic as well.

[straight-shoota]

This is quite old but the idea to improve these type mismatch error messages is really good. This can be really badly readable, especially when large union types are involved.

I'm not sure if swapping actual and expected type would have a real benefit here. I kind of like the order of showing first, what it should be. The reason for the error is that does not match this type. Showing the actual type is more like a hint at what might cause this.

Currently, the message format is type must be #{expected_type}, not #{actual_type}.
This is totally fine for simple types, like

type must be String, not Int32

or even small union types:

type must be String | Bool | Nil, not Int32

Taking the massive union type from the OP, it would already improve readability very much if there was a line break in between.

type must be (Array(Array(JSON::Type) | Array({Float64, Float64}) | Bool | Float
32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | PG::PGPa
th | String | Time | {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}
} | {Float64, Float64, Float64} | Nil) | Array(JSON::Type) | Array({Float64, Flo
at64}) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | P
G::Numeric | PG::PGPath | String | Time | {Float64 | {Float64, Float64}, Float64
 | {Float64, Float64}} | {Float64, Float64, Float64} | Nil),
not Slice(UInt8)

It could also help to use slightly different formatting for the types and the text.

But still, there is room for improvement. The error message might look like this:

type must be (Array(Array(JSON::Type) | Array({Float64, Float64}) | Bool | Float
32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | PG::PGPa
th | String | Time | {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}
} | {Float64, Float64, Float64} | Nil) | Array(JSON::Type) | Array({Float64, Flo
at64}) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | P
G::Numeric | PG::PGPath | String | Time | {Float64 | {Float64, Float64}, Float64
 | {Float64, Float64}} | {Float64, Float64, Float64} | Nil),
not (Array(Array(JSON::Type) | Array({Float64, Float64}) | Bool | Float32 | Float
64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | PG::PGPath | Stri
ng | Time | {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}} | {Floa
t64, Float64, Float64} | Nil) | Slice(UInt8) | Array(JSON::Type) | Array({Float6
4, Float64}) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int
64 | PG::Numeric | PG::PGPath | String | Time | {Float64 | {Float64, Float64}, F
loat64 | {Float64, Float64}} | {Float64, Float64, Float64} | Nil)

Do you spot the difference? 😄

It's a Slice(UInt8) somewhere in the middle. It's even hard to find if you know what you're looking for.

A simple visual improvement would be to align the leftmost character of both type strings to the same column. This allows to spot different patterns in the text flow and you can easily identify there's a difference in line 4.

type must be (Array(Array(JSON::Type) | Array({Float64, Float64}) | Bool | Float
32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | PG::PGPa
th | String | Time | {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}
} | {Float64, Float64, Float64} | Nil) | Array(JSON::Type) | Array({Float64, Flo
at64}) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | P
G::Numeric | PG::PGPath | String | Time | {Float64 | {Float64, Float64}, Float64
 | {Float64, Float64}} | {Float64, Float64, Float64} | Nil),
         not (Array(Array(JSON::Type) | Array({Float64, Float64}) | Bool | Float
32 | Float64 | Hash(String, JSON::Type) | Int32 | Int64 | PG::Numeric | PG::PGPa
th | String | Time | {Float64 | {Float64, Float64}, Float64 | {Float64, Float64}
} | {Float64, Float64, Float64} | Nil) | Slice(UInt8) | Array(JSON::Type) | Arra
y({Float64, Float64}) | Bool | Float32 | Float64 | Hash(String, JSON::Type) | In
t32 | Int64 | PG::Numeric | PG::PGPath | String | Time | {Float64 | {Float64, Fl
oat64}, Float64 | {Float64, Float64}} | {Float64, Float64, Float64} | Nil)

Additionally, the error message could just list all the individual types that are not included in the actual type (expected_types - actual_types):

invalid types: Slice(UInt8)