jeremyjh / dialyxir

Mix tasks to simplify use of Dialyzer in Elixir projects.
Apache License 2.0
1.69k stars 141 forks source link

Document `plt_add_deps: true` option #477

Closed rewritten closed 1 year ago

rewritten commented 1 year ago

While :project and :transitive values are deprecated for the :plt_add_deps option, passing true is not deprecated, and it is very useful, as one often has packages that only provide functions, not a stand-alone app.

jeremyjh commented 1 year ago

true is identical to :project and thus would be deprecated for the same reasons, if it were documented. This all exists to remain compatible with configurations that are 4+ years old at this point and was probably unnecessary to support in the first place. If you don't want the app_tree transitive dependencies you should use :app_direct; the only reason I know of that you would do this is if your PLT build runs out of memory when all apps are included. I don't know of a valid reason to use :project or true.

jeremyjh commented 1 year ago

If a change is needed here at all, it would be to print the deprecation warning when true is specified.

rewritten commented 1 year ago

@jeremyjh the reason to have true is to include dependencies that are not apps. An example where it is needed is in https://github.com/absinthe-graphql/dataloader/pull/152, where it uses a function from :openetelemetry_process_propagator, which does not have an app.

The failing run: https://github.com/absinthe-graphql/dataloader/actions/runs/4123691978/jobs/7126194854

Total errors: 3, Skipped: 0, Unnecessary Skips: 0 done in 0m1.31s lib/dataloader.ex:398:unknown_function Warning: Function OpentelemetryProcessPropagator.Task.async/1 does not exist.


lib/dataloader.ex:401:unknown_function Warning: Function OpentelemetryProcessPropagator.Task.async_stream/3 does not exist.


lib/dataloader/ecto.ex:972:unknown_function Warning: Function OpentelemetryProcessPropagator.Task.async_stream/3 does not exist.


jeremyjh commented 1 year ago

@rewritten as far as I know there is no such thing as library code without an app. If you used the default :app_tree you should see :opentelemetry_process_propagator get added to the PLT. If it doesn't that might be due to being used as an implicit transitive dependency, which is probably a bug in that library but you can work around by adding that app specifically via plt_add_apps: [:opentelemetry_process_propagator]

rewritten commented 1 year ago

As a matter of fact, using :app_tree still does not solve this, because the function apparently is not in any app.

That said, I might be missing some obvious way to let dialyxir know about functions. Maybe this happens because the project itself is not an app, so dializer does not include the needed modules.

jeremyjh commented 1 year ago

@rewritten just FYI that anything missing from app_tree is probably also going to be missing from a release build; so it might be better to just add opentelemetry_process_propagator as a mix dependency in your own project.

rewritten commented 1 year ago

The dependency is declared in Mix.exs (as optional, but usages are fenced by if Code.ensure_compiled?). This is what makes mix dialyzer fail.

dataloader
├── elixir
├── logger
│   └── elixir
└── telemetry

Is there a way to take into account optional dependencies?

If I change it to be a required dependency it works:

$ mix app.tree
dataloader
├── elixir
├── logger
│   └── elixir
├── opentelemetry_process_propagator
│   ├── elixir
│   ├── logger
│   └── opentelemetry_api
│       └── elixir
└── telemetry
jeremyjh commented 1 year ago

@rewritten Optional dependencies in a library are meant to be included by the consuming project. So you need to add opentelemetry_process_propagator to your own mix.exs.

rewritten commented 1 year ago

That's fine but then dataloader cannot pass mix dialyzer.

jeremyjh commented 1 year ago

@rewritten it can add it via plt_add_apps.

rewritten commented 1 year ago

Thank you @jeremyjh for bringing me to the right path. That has worked.