Dialyzer fixes

[waisbrot]

Related to #147

Each commit here fixes one set of problems.

The main problem was Ecto 2.1 defining a bunch of opaque types and then peeking inside of them with macros. In Ecto 2.2, the types are not marked as opaque and so you don't get clobbered by giant warning messages that you're not even responsible for. This is complicated by ecto_enum insisting on Ecto 2.1 even though it doesn't seem to actually need it. I overrode ecto_enum, pending gjaldon/ecto_enum#45 getting merged, but you may prefer to avoid this and just deal with Ecto 2.1.

I think most of the dialyzer errors after that fix are pretty readable. I'll go over my other fixes in detail.

First, Dialyzer errors tend to cascade: one little error deep in the call stack will cause problems all the way up the stack. Therefore, you should generally try to work on the deepest error first and if the error you're looking at doesn't make sense, just fix some other errors and re-run to see if things become clearer.

One error we see is

lib/open_pantry/web/controllers/user_order_controller.ex:6: Function index/2 has no local return

This basically means "according to type analysis, this function will always crash". Which really means that there's some type problem below and so we should keep looking.

Another error is

lib/open_pantry/web/controllers/user_order_controller.ex:18: The call 'Elixir.OpenPantry.UserOrder':all('user') will never return since the success typing is ([atom()]) -> any() and the contract is ([atom()]) -> ['Elixir.OpenPantry.UserOrder':t()] | []

Dialyzer is Erlang, so module names and functions look slightly different, but we can just squint. This says that you're calling OpenPantry.UserOrder.all(:user) but the all/1 function expects a list of atoms, not a single atom.

Looking at the code, it seems like the typespec got typo'd. list(atom() | atom()) which it's meant to be list(atom()) | atom().

A large, scary one is

lib/open_pantry/web/controllers/fallback_controller.ex:18: The call 'Elixir.Phoenix.Controller':render(#{'__struct__':='Elixir.Plug.Conn', 'adapter':={atom(),_}, 'assigns':=#{atom()=>_}, 'before_send':=[fun((#{'__struct__':='Elixir.Plug.Conn', 'adapter':={_,_}, 'assigns':=map(), 'before_send':=[fun((_) -> any())], _=>_}) -> #{'__struct__':='Elixir.Plug.Conn', 'adapter':={_,_}, 'assigns':=map(), 'before_send':=[fun((_) -> any())], _=>_})], 'body_params':=#{'__struct__'=>'Elixir.Plug.Conn.Unfetched', 'aspect'=>atom(), binary()=>binary() | [binary() | [any()] | #{binary()=>_}] | #{binary()=>binary() | [any()] | #{binary()=>_}}}, 'cookies':=#{'__struct__'=>'Elixir.Plug.Conn.Unfetched', 'aspect'=>atom(), binary()=>binary()}, 'halted':=_, 'host':=binary(), 'method':=binary(), 'owner':=pid(), 'params':=#{'__struct__'=>'Elixir.Plug.Conn.Unfetched', 'aspect'=>atom(), binary()=>binary() | [binary() | [any()] | #{binary()=>_}] | #{binary()=>binary() | [any()] | #{binary()=>_}}}, 'path_info':=[binary()], 'path_params':=#{binary()=>binary() | [binary() | [any()] | #{binary()=>_}] | #{binary()=>binary() | [any()] | #{binary()=>_}}}, 'peer':={{byte(),byte(),byte(),byte()} | {char(),char(),char(),char(),char(),char(),char(),char()},char()}, 'port':=char(), 'private':=#{atom()=>_}, 'query_params':=#{'__struct__'=>'Elixir.Plug.Conn.Unfetched', 'aspect'=>atom(), binary()=>binary() | [binary() | [any()] | #{binary()=>_}] | #{binary()=>binary() | [any()] | #{binary()=>_}}}, 'query_string':=binary(), 'remote_ip':={byte(),byte(),byte(),byte()} | {char(),char(),char(),char(),char(),char(),char(),char()}, 'req_cookies':=#{'__struct__'=>'Elixir.Plug.Conn.Unfetched', 'aspect'=>atom(), binary()=>binary()}, 'req_headers':=[{binary(),binary()}], 'request_path':=binary(), 'resp_body':='nil' | binary() | maybe_improper_list(binary() | maybe_improper_list(any(),binary() | []) | byte(),binary() | []), 'resp_cookies':=#{binary()=>#{}}, 'resp_headers':=[{binary(),binary()}], 'scheme':='http' | 'https', 'script_name':=[binary()], 'secret_key_base':='nil' | binary(), 'state':='chunked' | 'file' | 'sent' | 'set' | 'set_chunked' | 'set_file' | 'unset', 'status':='nil' | 1..1114111},'Elixir.OpenPantry.Web.ErrorView','404') breaks the contract ('Elixir.Plug.Conn':t(),binary() | atom(),'Elixir.Keyword':t() | map()) -> 'Elixir.Plug.Conn':t()

(sorry that's unreadable in Github...)
Anyway, it's mostly just printing the Plug.Conn struct, so you have to skim over that. The best bet is at the end:

breaks the contract ('Elixir.Plug.Conn':t(),binary() | atom(),'Elixir.Keyword':t() | map()) -> 'Elixir.Plug.Conn':t()

If you look it up, this is Phoenix.Controller.render/3 that Dialyzer is talking about, but we're passing a view so this should be Phoenix.Controller.render/4!

And finally:

lib/open_pantry/web/models/stock.ex:63: Invalid type specification for function 'Elixir.OpenPantry.Stock':stockable/1. The success typing is (#{'__meta__':=_, '__struct__':='Elixir.OpenPantry.Stock', 'aisle':=_, 'arrival':=_, 'credit_types':=_, 'credits_per_package':=_, 'expiration':=_, 'facility':=_, 'facility_id':=_, 'food':=_, 'food_group':=_, 'food_id':=_, 'id':=_, 'image':=_, 'inserted_at':=_, 'max_per_package':=_, 'max_per_person':=_, 'meal':=_, 'meal_id':=_, 'offer':=_, 'offer_id':=_, 'override_text':=_, 'packaging':=_, 'quantity':=_, 'reorder_quantity':=_, 'row':=_, 'shelf':=_, 'stock_distributions':=_, 'storage':=_, 'updated_at':=_, 'user_orders':=_, 'weight':=_}) -> #{'__meta__':=_, '__struct__':='Elixir.OpenPantry.Food' | 'Elixir.OpenPantry.Meal' | 'Elixir.OpenPantry.Offer', 'calcium'=>_, 'calories'=>_, 'calories_from_fat'=>_, 'carbohydrate'=>_, 'cho_factor'=>_, 'cholesterol'=>_, 'common_name'=>_, 'credit_types'=>_, 'description'=>_, 'dessert'=>_, 'entree'=>_, 'facilities'=>_, 'fat'=>_, 'fat_factor'=>_, 'fiber'=>_, 'food_group'=>_, 'foodgroup_code'=>_, 'id'=>_, 'inserted_at'=>_, 'longdesc'=>_, 'manufacturer_name'=>_, 'n_factor'=>_, 'name'=>_, 'ndb_no'=>_, 'pro_factor'=>_, 'protein'=>_, 'refuse'=>_, 'refuse_description'=>_, 'saturated_fat'=>_, 'scientific_name'=>_, 'shortdesc'=>_, 'side_dish1'=>_, 'side_dish2'=>_, 'sodium'=>_, 'stocks'=>_, 'sugars'=>_, 'survey'=>_, 'updated_at'=>_, 'users'=>_, 'weight'=>_}

Again, the struct expansion makes this look harder than it is. I use iTerm2 which lets me double-click to select matching delimeters, and I use that to help visualize the error.

The error says that the success type (what Dialyzer sees will actually happen) is

(%OpenPantry.Stock{}) -> %OpenPantry.Food{} | %OpenPantry.Meal{} | %OpenPantry.Offer{}

This is obvious from the function, so the given typespec was just wrong. I corrected it by defining a new type stockable(). Defining a new type to express what's going on is almost always a good idea, since it's another form of documentation.

The only remaining Dialyzer errors I see is for OpenPantry.FoodSelection.Stock. It's similar to the above, but I wasn't sure what was going on because Meal and Offer are the same as above but Food is a different module.

I hope this was helpful! Dialyzer can be frustrating to learn but it's caught some tricky errors for me in the past! (I think the render/3/render/4 error is probably a good example)

[bglusman]

Holy shit thank you! Will need to digest and run locally but looks awesome at a glance, good my started attempts weren't completely useless/off base!

[bglusman]

Hmm, failing tests, but I'll see if I can fix those up, maybe less aggressive hex updates or something. Also, the remaining error is almost certainly my experiment with multiple schemas for different contexts, there's two Food schemas, one in the admin context and one in the FoodSelection context... could drop the former, or make a food selection version of the stockable type or something.

[waisbrot]

Installing inotify-tools should fix Travis, I think.

[bglusman]

This seems to be working OK and passing travis on a branch I made, but there's a TON of new warnings I'd like to suppress or deal with before we merge this in, most notably from cowboy/plug related to this issue phoenixframework/phoenix#2437

Putting it on hold for now, but the passing branch is dialyzer_fixes and if anyone else wants to fix up some warnings before I get around to it, awesome, otherwise there's no huge rush to get this on, though it is much appreciated @waisbrot !