Compiler should Emit Warning/Notes when Deduced Type Differs from Annotated Type.

[stellarpower]

Feature Request

I had some code recursively walking through a tree. The Crystal compiler was very clever, and spotted that I was not handling the base case properly. In this code, I mapped over the node's children, and assigned this to an Array(UInt8?). However, the compiler complained that in order to assign to this, the result of the map had to be Array(UInt8?), not Array(Nil). This was because it had worked out that the nil returned from the base case was wrong, and would propagate upwards. This meant that my recursive function would always return nil, even though it was annotated to return UInt8?, and thus, the array could only ever be an array of nils.

This is great, having the compiler check my logic is the way to do things properly. However, my only issue was that it was hard to work out why this was the case. The function was annotated to return UInt8?, and the array was annotated as Array(UInt8?). If the compiler has inferred in cases like this, that the actual return type differs from what I am expecting, in effect due to unreachable code or similar issues, then I'd like to have a hint in the compiler error stack explaining what it deduced to be different form what I expected, and where this occurs. In this case, if it could tell me that it has deduced that a particular type in a union will never be retuned, then that would help find the source of the error - in particular, because this was ultimately a logic error, not a slip of the fingers or some other semantics-related issue. My code was effectively correct (and other languages would just compile this anyway), but my logic was not, so for this to show up as a type error somewhat hides the source of the issue.

Here's a toy example. Obviously just returning nil would be a bit of a blaring mistake, but it servers as a minimal reproduction for the issue.

def recurse(n) : Int32?
    nil
end

results : Array(Int32?) = (0..10).map{|n|
    recurse n
}

Compiler output:

1 ❯ shards build --debug --error-trace
...
Error target MRE failed to compile:
In src/MRE.cr:5:1

 5 | results : Array(Int32?) = (0..10).map{|n|
      ^------
Error: type must be Array(Int32 | Nil), not Array(Nil)

And what I'd like to see, is something along these lines:

1 ❯ shards build --debug --error-trace
...
Error target MRE failed to compile:
In src/MRE.cr:5:1

 5 | results : Array(Int32?) = (0..10).map{|n|
      ^------
Error: type must be Array(Int32 | Nil), not Array(Nil)

Note: result of expression deduced to have type Array(Nil)
 6 |     recurse n
         ^--------

Note: because method deduced to have return type of Nil.
 1 | def recurse(n) : Int32?
         ^---------

Warning: Method deduced to have return type differing from its explicit annotation. Its effective return type is Nil.
 1 | def recurse(n) : Int32?
                      ^-----
Note: return of type Int32 in union (Int32 | Nil) is unreachable
 1 |     nil
         ^--

I think a warning whenever an explicit type annotation is found not to match what the compiler has deduced would be good in all cases. Even if it's not causing an error elsewhere, it's probably a bug, so I think it should not go by silently.

Modern clang can be quite good at suggesting what I might have meant to do, as well as backend compiler logic has determined something that isn't in the source code; so I don't know if there's some inspiration to be had, either in error messages, or in how it's implemented within LLVM, there.

PS: This bug may well be a duplicate of another; but beyond searching for keywords like "type inference" and getting back tonnes of results, I'm not really sure what search term to use to track it down.

[straight-shoota]

I think a warning whenever an explicit type annotation is found not to match what the compiler has deduced would be good in all cases. Even if it's not causing an error elsewhere, it's probably a bug, so I think it should not go by silently.

This is a controversial topic. It's discussed in more detail in #3803

[ysbaddaden]

If I understand correctly what the example exposes is that the annotated return type doesn't coerce the actual return type. It's just a constraint that gets accepted (Nil is a subset of Int32|Nil) and the compiler will generate a recurse(Int32) : Nil function.

The warning would generate too much noise. For example it's common for an abstract method implementation to return only a subset of the annotated types.

Having the compiler explain the issue could be interesting. It's good at reporting how it reached the error, but it doesn't explain why the block is returning Nil.

[stellarpower]

So, am I right in thinking that the annotation is strictly a constraint? A bit like a concept requirement we now have in C++20. As, admittedly I haven't read up enough on the Crystal theory, it's more been a language I have picked up on the go, but, when I write a function as in the example, I expected that this means the compiler is stamping out binary code that takes those types and returns the one I specify. Is it more like you're just saying "this can return an integer, or nil, or a nullable integer - but refuse to compile this overload for anything else"? It's a generic but with a requirement on the list of permissible types? And the compiler is free to output a specialisation for just a sunset of the types, and/or an overload that handles the possible options at runtime, as would be the case here if I returned nil if rand() was less than 0.5, and some integer if greater, for example?

Also on the verbosity point - could it simply be absorbed into the error-trace option? As the compiler by default only spits out the first problem we encounter, right? If it would be too verbose to have as a warning in general cases, I think showing it in the error stack, or having an option similar to what we can do with macros, would then mean that the user is asking explicitly for more information. I was scratching my head for a good ten minutes so it's stuff I'd like to know given in this scenario my program already can't build.

I guess it perhaps covers a more general case, that if my code doesn't build and the compiler has decided something is wrong that's somewhat subtle, in order for me to fix the problem often I'll need to know more about why it made this decision and where the issue arises. I think that info is good to have, if people ask for it, in general, and it helps the developer experience when the tools catch our errors before they run something but also can then explain it to us on a way that makes implementing the fix relatively straightforward. Maybe something like the pedantic options of GCC etc. is not the best way to go, given how many, many diagnostics flags there are, but, these warnings sometimes have revealed bugs in my code that I hadn't seen, and if they're too verbose, that's fine, we can turn them off. If we want them, that's also fine, we turn them on. For reasons that are beyond me, VS code has recently inserted a semicolon after an if condition on more than one occasion, and I didn't catch it at all until I saw clang point it out to me in purple amongst all the many other warnings I can ignore. That could have been really quite nasty to debug as I didn't scan it in the source code.

[ysbaddaden]

Note: This discussion would fit better in the forum.

To answer quickly (to the extent of my knowledge): an Int32|Nil return type annotation on a def says that the method may return an Int32 or a Nil. It's fine to return either, but it's an error to return something else (e.g. String). I believe this also stands for argument types.

This doesn't stand for a fun that must be fully typed and generates an extern symbol, compatible with C.

the compiler is free to output a specialisation for just a sunset of the types

Yes, and it will actively try to specialize def methods, even on how they're actually called (as a JIT would).

It's a generic but with a requirement on the list of permissible types?

I believe it can be viewed this way. It is said that "Crystal is a big template language".

[stellarpower]

Thanks, that makes sense. The template part I have gradually learned the hard way 😂.

Do you want to merge this issue into another re warnings or compiler verbosity and close it?