stavro / arc_ecto

An integration with Arc and Ecto.
255 stars 149 forks source link

Dialyzer error #109

Open OvermindDL1 opened 5 years ago

OvermindDL1 commented 5 years ago

For the given code:

  def changeset(struct, params \\ %{}) do
    struct
    |> cast(params, [:id, :type, :data, :removed_at])
    |> cast_attachments(params, [:file_id])
    |> validate_required([:id, :type, :file_id])
  end

The cast_attachments(params, [:file_id]) expression is causing a dialyzer failure of (some formatting so it's actually readable):

The pattern
  #{'__meta__':=_}
can never match the type
  #{'__struct__':='Elixir.Ecto.Changeset',
    'action':='delete' | 'ignore' | 'insert' | 'nil' | 'replace' | 'update',
    'changes':=#{atom()=>_},
    'constraints':=[
      #{'constraint':=binary(),
        'error_message':=binary(),
        'error_type':=atom(),
        'field':=atom(),
        'match':='exact' | 'prefix' | 'suffix', 'type':='check' | 'exclusion' | 'foreign_key' | 'unique'
      }
    ],
    'data':='nil' | map(),
    'empty_values':=_,
    'errors':=[{atom(),{_,_}}],
    'filters':=#{atom()=>_}, 'params':='nil' | #{binary()=>_},
    'prepare':=[fun((_) -> any())],
    'repo':=atom(),
    'repo_opts':=[{atom(),_}],
    'required':=[atom()],
    'types':='nil' | #{atom()=>atom() | {'array',_} | {'embed',map()} | {'in',_} | {'map',_}},
    'valid?':=boolean(),
    'validations':=[{atom(),_}]
  }

In arc_ecto's schema.ex file lines 15 to 18 are:

      scope = case changeset_or_data do
        %Ecto.Changeset{} -> Ecto.Changeset.apply_changes(changeset_or_data)
        %{__meta__: _} -> changeset_or_data
      end

Because this is a defmacro the whole body is getting inlined, which is what's causing this error. There is not really a need for it to be a defmacro and it could/should probably just be a normal def, in which case the inferred spec then should work. Alternatively you can change the case changeset_or_data do to be wrapped be something like case Arc.Ecto.Schema.wrap(changeset_or_data) do to opaque it, but then you could just do the work there as well. Likely all around easier and better for cast_attachments to just not be a macro at all however. :-)

OvermindDL1 commented 5 years ago

Interestingly a PR implementing the second suggestion appears to be at https://github.com/stavro/arc_ecto/pull/106 but has not been accepted or rejected yet a month later. Although that does fix it and keeps the API, it's probably better to just bump the API version and change the defmacro to just dev, since a cross module call is being performed anyway it doesn't save anything otherwise and can actually slow down the function calling cast_attachments due to ballooning its opcode size (not that it really matters on this path). :-)