feat: TypeCheck integration

[bamorim]

closes #33

I decided to not even include an option as I feel options are a workaround for good experience.

My solution instead was to automatically check whether TypeCheck.Macros are required and just call it directly. If you think it is too risky, we can add an option to enable that just as an "experimental feature" and we can warn people that this will change in the future (the feature will be enabled by default). What do you think?

[bamorim]

@dvic as TypeCheck doesn't work with Elixir 1.9 I think the easiest we can do is to just stop testing with Elixir 1.9 as well. Our library should still work if you don't include TypeCheck, but Elixir 1.10 was released in Jan 2020, so I think we should be okay not testing for Elixir 1.9 now.

[dvic]

@dvic as TypeCheck doesn't work with Elixir 1.9 I think the easiest we can do is to just stop testing with Elixir 1.9 as well. Our library should still work if you don't include TypeCheck, but Elixir 1.10 was released in Jan 2020, so I think we should be okay not testing for Elixir 1.9 now.

I think that's fair 👍.

[dvic]

closes #33

I decided to not even include an option as I feel options are a workaround for good experience.

My solution instead was to automatically check whether TypeCheck.Macros are required and just call it directly. If you think it is too risky, we can add an option to enable that just as an "experimental feature" and we can warn people that this will change in the future (the feature will be enabled by default). What do you think?

I think this is a nice solution! But I guess we should publish this as a breaking change? (major version bump)? Otherwise users might miss this big change in the changelog?

[bamorim]

I think this is a nice solution! But I guess we should publish this as a breaking change? (major version bump)? Otherwise users might miss this big change in the changelog?

I agree. Maybe we can start with an experimental feature config so we can test out and follow up with a breaking change major release.

The only scenario it will break is if someone has something like that:

defmodule MySchema do
  use TypedEctoSchema
  use TypeCheck

  typed_embedded_schema do
    field :int, :int
  end

  # This will break now because we will instruct TypeCheck to generate this function
  def t, do: :ok
end

[bamorim]

Ok, gave some more though, I don't want to add config options, therefore I'll just release a pre release or a release candidate so we can test things out.

We are still in pre-1.0, so technically a minor bump can introduce breaking changes. Would you release the 1.0 or just something like 0.5?

[bamorim]

Just found out that this doesn't work with non embedded schemas because of Ecto.Schema.Metadata.t().

In fact, I just realized this will break with so many things because TypeCheck requires all types to be TypeCheck types.

I'll hold this back a little bit and I think we will need a feature flag for that as it can break in many places.

[bamorim]

We will need the overrides for the following types:

• Ecto.Schema.Metadata.t()
• Ecto.Schema.has_many(t)
• Ecto.Schema.has_one(t)
• Ecto.Schema.belongs_to(t)
• Ecto.Schema.many_to_many(t)
• Ecto.Association.NotLoaded.t()
• Decimal.t()

[bamorim]

@dvic added a feature flag config and docs so we can safely merge this and start working on the overrides. Maybe it is worth creating a library just for the overrides like type_check_ecto that adds overrides for all of these.

[bamorim]

@Qqwy what are your thoughts on this? What you would suggest?

[dvic]

@dvic added a feature flag config and docs so we can safely merge this and start working on the overrides. Maybe it is worth creating a library just for the overrides like type_check_ecto that adds overrides for all of these.

Cool! I'm ok with both options, but how would this work with the current approach? (i.e., detecting whether or not to use TypeCheck by checking the requires). As far as I can see you have to configure the overrides with use TypeCheck, overrides: [{&Original.t/0, &Replacement.t/0}] so we would have to inject that ourselves when the experimental flag is turned on? (and offer an option for adding additional options to TypeCheck)

As an alternative we could also choose to have something like

      defmodule InteropWithTypeCheck do
         use TypedEctoSchema.TypeCheck
         use TypedEctoSchema

         typed_embedded_schema do
           field(:year, :number)
         end
       end

to control this at module level, then we wouldn't need the experimental flag anymore as well as the type_check: true option.

[bamorim]

@dvic My idea would be to just have another list of overwrites so we could do:

defmodule InteropWithTypeCheck do
  use TypeCheck, overrides: EctoTypeCheck.overrides()
  use TypedEctoSchema
end

That being said, what could be an interesting proposal for typecheck would be to have modules that would return either a replacement or nil so people could configure like:

config :type_check,
  override_providers: [TypeCheck.Overrides, EctoTypeCheck]

Because the current implementation requires either calling the overrides function and passing into use TypeCheck or doing something crazy like

config :type_check, overrides: [
  {&Original.t/0, &Replacement.t/0}
]

However, I think a simpler alternative would be to have our own types and change the type mapper to return this types directly instead of relying on type check override feature. I'll give that approach a try.

[Qqwy]

The best (most flexbile while still being explicit and not overly boilerplate-y) pattern if you have a lot of overrides or other options to pass to TypeCheck is to have a single module, say MyProject.TypeCheck in which you define your own __using__.

This is similar to how e.g. MyApp.Repo wraps Ecto's Ecto.Repo and Phoenix's YourAppWeb module works.

[Qqwy]

I like the idea of checking whether TypeCheck was imported/used in the same module inside the definition of the typed_ecto_schema macro.

The best thing to check for probably, is the existence of the TypeCheck.Options module attribute. The existence of this can be seen as a rigid piece of 'public API' which future versions of TypeCheck will not change :+1:.

[bamorim]

Just to give an update on that for those waiting: I'm looking for a way to make it easier to generate the overrides for foreign libraries, so far I managed to create a script that automatically generates overrides for one file. As soon as I have something I'm happy with and I have all the required overrides for Ecto types I'll update you here.

[bamorim]

@dvic I think I have something worth looking now. It was actually easier than I thought. The code for sure can be improved and maybe we should think wether we want to include all these overrides as part of this library os as a different library.

@Qqwy I'd love to know your opinion on this method of generating overrides automatically. It is pretty naive for now, but maybe it can be an interesting library for TypeCheck or even part of TypeCheck itself with some improvements.

[bamorim]

@dvic One problematic thing though is that the generated overrides are dependent on the specific Ecto version, so by extracting into other libraries (like type_check_ecto, for example), would allow us to generate specific "bindings" for specific Ecto versions and publish all of them, so you could pick your specific generated bindings depending on the version of Ecto you have in your application.

[dvic]

@dvic I think I have something worth looking now. It was actually easier than I thought. The code for sure can be improved and maybe we should think wether we want to include all these overrides as part of this library os as a different library.

Nice! I gave it a quick glance (don't have the bandwidth to do a proper review this week) but it looks good to me!

@dvic One problematic thing though is that the generated overrides are dependent on the specific Ecto version, so by extracting into other libraries (like type_check_ecto, for example), would allow us to generate specific "bindings" for specific Ecto versions and publish all of them, so you could pick your specific generated bindings depending on the version of Ecto you have in your application.

Nice! One thing we could also do is support a limited set of Ecto versions and use Application.spec(:ecto, :vsn) to determine the Ecto version at compile time? (not sure if this helps)

[bamorim]

@jtormey would you be willing to test this in your application to see how well it behaves?

[jtormey]

Absolutely! This is incredibly exciting and I'm amazed at how quickly you all pulled this together, very appreciated 🙂

[jtormey]

I've started testing this out in our project and have hit a couple issues, dropping them here but will continue to see what I run into in the meantime.

1. Compilation error when enabling type_check globally (config :typed_ecto_schema, type_check: true in config.exs). Does not occur when this option is not specified, this may be user error.

Error details

==> typed_ecto_schema Compiling 25 files (.ex) == Compilation error in file lib/typed_ecto_schema/overrides/ecto/association.ex == ** (ArgumentError) errors were found at the given arguments: * 3rd argument: not a proper list (stdlib 3.17.1) :lists.keyfind(:debug, 1, [{:overrides, [{{Decimal, :coefficient, 0}, {TypedEctoSchema.Overrides.Decimal, :coefficient, 0}}, {{Decimal, :decimal, 0}, {TypedEctoSchema.Overrides.Decimal, :decimal, 0}}, {{Decimal, :exponent, 0}, {TypedEctoSchema.Overrides.Decimal, :exponent, 0}}, {{Decimal, :rounding, 0}, {TypedEctoSchema.Overrides.Decimal, :rounding, 0}}, {{Decimal, :sign, 0}, {TypedEctoSchema.Overrides.Decimal, :sign, 0}}, {{Decimal, :signal, 0}, {TypedEctoSchema.Overrides.Decimal, :signal, 0}}, {{Decimal, :t, 0}, {TypedEctoSchema.Overrides.Decimal, :t, 0}}, {{Decimal.Context, :t, 0}, {TypedEctoSchema.Overrides.Decimal.Context, :t, 0}}, {{Ecto.Adapter, :adapter_meta, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter, :adapter_meta, 0}}, {{Ecto.Adapter, :t, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter, :t, 0}}, {{Ecto.Adapter.Queryable, :adapter_meta, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Queryable, :adapter_meta, 0}}, {{Ecto.Adapter.Queryable, :cached, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Queryable, :cached, 0}}, {{Ecto.Adapter.Queryable, :options, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Queryable, :options, 0}}, {{Ecto.Adapter.Queryable, :prepared, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Queryable, :prepared, 0}}, {{Ecto.Adapter.Queryable, :query_cache, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Queryable, :query_cache, 0}}, {{Ecto.Adapter.Queryable, :query_meta, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Queryable, :query_meta, 0}}, {{Ecto.Adapter.Queryable, :selected, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Queryable, :selected, 0}}, {{Ecto.Adapter.Schema, :adapter_meta, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :adapter_meta, 0}}, {{Ecto.Adapter.Schema, :constraints, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :constraints, 0}}, {{Ecto.Adapter.Schema, :fields, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :fields, 0}}, {{Ecto.Adapter.Schema, :filters, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :filters, 0}}, {{Ecto.Adapter.Schema, :on_conflict, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :on_conflict, 0}}, {{Ecto.Adapter.Schema, :options, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :options, 0}}, {{Ecto.Adapter.Schema, :placeholders, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :placeholders, 0}}, {{Ecto.Adapter.Schema, :returning, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :returning, 0}}, {{Ecto.Adapter.Schema, :schema_meta, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Schema, :schema_meta, 0}}, {{Ecto.Adapter.Transaction, :adapter_meta, 0}, {TypedEctoSchema.Overrides.Ecto.Adapter.Transaction, :adapter_meta, 0}}, {{Ecto.Association, :t, 0}, {TypedEctoSchema.Overrides.Ecto.Association, :t, 0}}, {{Ecto.Association.NotLoaded, :t, 0}, {TypedEctoSchema.Overrides.Ecto.Association.NotLoaded, :t, 0}}, {{Ecto.Changeset, :action, 0}, {TypedEctoSchema.Overrides.Ecto.Changeset, :action, 0}}, {{Ecto.Changeset, :constraint, 0}, {TypedEctoSchema.Overrides.Ecto.Changeset, :constraint, 0}}, {{Ecto.Changeset, :data, 0}, {TypedEctoSchema.Overrides.Ecto.Changeset, :data, 0}}, {{Ecto.Changeset, :error, 0}, {TypedEctoSchema.Overrides.Ecto.Changeset, :error, 0}}, {{Ecto.Changeset, :t, 0}, {TypedEctoSchema.Overrides.Ecto.Changeset, :t, 0}}, {{Ecto.Changeset, :t, 1}, {TypedEctoSchema.Overrides.Ecto.Changeset, :t, 1}}, {{Ecto.Changeset, :types, 0}, {TypedEctoSchema.Overrides.Ecto.Changeset, :types, 0}}, {{Ecto.Changeset.Relation, :t, 0}, {TypedEctoSchema.Overrides.Ecto.Changeset.Relation, :t, 0}}, {{Ecto.Multi, :changes, 0}, {TypedEctoSchema.Overrides.Ecto.Multi, :changes, 0}}, {{Ecto.Multi, :fun, 1}, {TypedEctoSchema.Overrides.Ecto.Multi, :fun, 1}}, {{Ecto.Multi, :merge, 0}, {TypedEctoSchema.Overrides.Ecto.Multi, :merge, 0}}, {{Ecto.Multi, :name, 0}, {TypedEctoSchema.Overrides.Ecto.Multi, :name, 0}}, {{Ecto.Multi, :run, 0}, {TypedEctoSchema.Overrides.Ecto.Multi, :run, 0}}, {{Ecto.Multi, :t, 0}, {TypedEctoSchema.Overrides.Ecto.Multi, :t, ...}}, {{Ecto.ParameterizedType, :opts, ...}, {TypedEctoSchema.Overrides.Ecto.ParameterizedType, ...}}, {{Ecto.ParameterizedType, ...}, {...}}, {{...}, ...}, {...}, ...]} | true]) (elixir 1.13.4) lib/keyword.ex:353: Keyword.get/3 (type_check 0.12.1) lib/type_check/options.ex:127: TypeCheck.Options."new (overridable 1)"/1 (type_check 0.12.1) lib/type_check/spec.ex:1: TypeCheck.Options.new/1 lib/typed_ecto_schema/overrides/ecto/association.ex:4: (module) could not compile dependency :typed_ecto_schema, "mix compile" failed. Errors may have been logged above. You can recompile this dependency with "mix deps.compile typed_ecto_schema", update it with "mix deps.update typed_ecto_schema" or clean it with "mix deps.clean typed_ecto_schema"

2. Compilation error when referencing the type of a schema within the schema module (is this a TypeCheck limitation?). The same function works when defined in a different module, and references the schema module.

Sample code:

defmodule Upside.TypesSchema do
  use TypedEctoSchema.TypeCheck
  use TypedEctoSchema

  typed_schema "types", type_check: true do
    field :my_field, :string
  end

  @spec! change(__MODULE__.t()) :: Ecto.Changeset.t()
  def change(type) do
    type
    |> Ecto.Changeset.change()
  end
end

Error details

Compiling 1 file (.ex) == Compilation error in file lib/upside/types.ex == ** (UndefinedFunctionError) function Upside.TypesSchema.t/0 is undefined (function not available) Upside.TypesSchema.t() (stdlib 3.17.1) erl_eval.erl:685: :erl_eval.do_apply/6 (elixir 1.13.4) lib/code.ex:797: Code.eval_quoted/3 (type_check 0.12.1) lib/type_check/type.ex:96: TypeCheck.Type.build_unescaped/4 (elixir 1.13.4) lib/enum.ex:1593: Enum."-map/2-lists^map/1-0-"/2 (type_check 0.12.1) lib/type_check/macros.ex:247: anonymous fn/3 in TypeCheck.Macros.create_spec_defs/3 (elixir 1.13.4) lib/enum.ex:2396: Enum."-reduce/3-lists^foldl/2-0-"/3 (type_check 0.12.1) lib/type_check/macros.ex:243: TypeCheck.Macros.create_spec_defs/3 (type_check 0.12.1) expanding macro: TypeCheck.Macros.__before_compile__/1 lib/upside/types.ex:1: Upside.TypesSchema (module)

[bamorim]

For the second issue, can you try changing the order of the use calls?

[jtormey]

Changing the order of the use calls produces the same error unfortunately 🙁

[bamorim]

@jtormey thanks for reporting that, I'll check workarounds for that when I have some time.

[Qqwy]

The first issue seems to be caused by the way the list of overrides is being built. It ends up being [{Foo, [a: 1]}, | true], that is the true is used as sentinel for the list.

The second issue might be caused by somewhat of a technical limitation inside TypeCheck because of the way Elixir modules are compiled: in the module body (i.e. outside of the body of the functions) we do not have access to types that are compiled in the same module yet, so TypeCheck creates a separate 'internal' module that only contains all types, and compiles all usage of types in the module body using those. A lot of work has happened to ensure that when someon writes MyModule.t that it will use TypeCheck.Internals.UserTypes.MyModule.t() in the module body. The only times this goes wrong is when writing certain kinds of macros that use it.

@bamorim you might want to try to use __MODULE__.t() or maybe a plain t() instead somewhere in the code-generating code.

[Qqwy]

Absolutely amazing work in this PR by the way! :green_heart: