Dialyxir - Recent Updates and Request for Feedback

[jeremyjh]

@fishcakez

Yes but I think we are talking passed each other as I was only talking about when a dep adds or removes modules.

You are right, I reread your original post and see that I missed you pointed out this is in addition to what --check_plt already does. I guess I’m not surprised --check_plt does not remove modules, but I am surprised it doesn’t add them - that would also cause problems for CLI users of dialyzer. I agree then I should also use this approach and your code for it in dialyxir.

[christopheradams]

@fishcakez

I maintain the rebar3 provider which has all the features of dialyze

Huh, is it possible to run rebar3 dialyze on a compiled Elixir _build?

[sasajuric]

@fishcakez

Importantly dialyze is no longer maintained because no one was contributing and only people complaining.

I’m a happy user of dialyze and I made one (however small) contribution

I think it’s a great tool and it pretty much just works as advertised.

One thing I’d like to have is some project config to include additional apps in PLT. The main case for this is mix. At my company we have a couple of custom mix tasks, and dialyzer complains since mix is not in the PLT. Obviously, we don’t want to add mix as a runtime dep, so our current solution is to suppress these warnings. IMO a better solution would be to explicitly request dialyze to include mix in the PLT via project config. I’ll be happy to make a PR if you’re open to this addition.

@fishcakez

For example I think phoenix users can’t use this task very well because phoenix does not generate the correct applications list.

Has anyone discussed this with the Phoenix team? If Phoenix generator added plug and poolboy in the application list (but not compile-time deps), then this issue would be solved, right? And FWIW, I agree that it’s the proper way to go.

[fishcakez]

It is not possible. It’s very tedious to maintain the same features in mix and rebar3. Rebar3 got more attention because:

• dialyzer warnings are in Erlang terms, Elixir won’t have a native feel without patching dialyzer and its only in the last year did we manage to remove compiler generated warnings in Elixir.
• Testing was much easier (rebar3 had a better test framework at time) and orders of magnitude faster to test (don’t need to add :elixir to the PLT)
• I tend to use Erlang for complicated projects that don’t require Elixir-only features to take advantage of its simpler syntax and thats when I want dialyzer the most
• People contributed to it

Edit: Oops this was supposed to be a reply to @christopheradams

[fishcakez]

@"sasajuric

I think it’s a great tool and it pretty much just works as advertised.

One thing I’d like to have is some project config to include additional apps in PLT. The main case for this is mix. At my company we have a couple of custom mix tasks, and dialyzer complains since mix is not in the PLT. Obviously, we don’t want to add mix as a runtime dep, so our current solution is to suppress these warnings. IMO a better solution would be to explicitly request dialyze to include mix in the PLT via project config. I’ll be happy to make a PR if you’re open to this addition]

At present there are two dialyzer tasks which lack the features of each other and in my opinion we have two poor tasks. As @jeremyjh is copying features (that are missing in :dialyxir) from :dialyze to :dialyxir there is no need to have :dialyze any more. The feature you want is present in :dialyxir so perhaps you should help @jeremyjh move the features you want across. The license is the same so nothing prohibits this.

@"sasajuric

Has anyone discussed this with the Phoenix team? If Phoenix generator added plug and poolboy in the application list (but not compile-time deps), then this issue would be solved, right? And FWIW, I agree that it’s the proper way to go.

I have brought it up on IRC several times where phoenix team members are involved in the conversation.

[sasajuric]

@fishcakez

At present there are two dialyzer tasks which lack the features of each other and in my opinion we have two poor tasks. As @jeremyjh is copying features (that are missing in :dialyxir) from :dialyze to :dialyxir there is no need to have :dialyze any more. The feature you want is present in :dialyxir so perhaps you should help @jeremyjh move the features you want across. The license is the same so nothing prohibits this.

The question is whether it should be the other way around. I actually opted for dialyze mostly because of some differences you mentioned earlier. It seemed to me (and still does) that it’s pretty much plug-and-play, with sensible default choices and optimized workflow. So from where I stand, it seems that dialyze should be the base, and a few features (e.g. customization of project PLT) should be merged from dialyxir.

But TBH, I took only a brief glance at dialyxir , so I’d like to learn what’s in dialyxir that’s missing from dialyze?

@fishcakez

I have brought it up on IRC several times where phoenix team members are involved in the conversation.

And what was the answer?

[fishcakez]

@sasajuric

The question is whether it should be the other way around. I actually opted for dialyze mostly because of some differences you mentioned earlier. It seemed to me (and still does) that it’s pretty much plug-and-play, with sensible default choices and optimized workflow. So from where I stand, it seems that dialyze should be the base, and a few features (e.g. customization of project PLT) should be merged from dialyxir.

Both projects are under apache 2.0 so nothing stops anyone doing this. I think it is best for the users of both if we get behind one library in the long run and @jeremyjh wants to do the work so I think we should let him and get behind dialyxir. I do not want to do this work.

@sasajuric

But TBH, I took only a brief glance at dialyxir , so I’d like to learn what’s in dialyxir that’s missing from dialyze?

I wrote a long reply above to explain the differences and help @jeremyjh to move forward, and I got some differences wrong so I’ll let someone else answer that.

@sasajuric

And what was the answer?

I don’t think they saw it as an issue because phoenix will start plug. However if transitive mode is enabled on dialyxir then plug will be added anyway, and hopefully this will become the default in the next version.

[jeremyjh]

Dialyxir permits configuration of many different things, and while its defaults are not good it can be made to work acceptably well for almost any project due to this. I think ability to have mix file configuration of warnings (both disabling default and enabling the extended checks) is a good idea; this should be captured somewhere in your projects configuration management, and not left to command line parameters unless those parameters are managed in some higher-level script.

It has other configuration which is maybe more dubious - for example it will let you configure the location of the PLT file; this is needed in dialyxir because its default is stupid. But even for dialyze which has a really smart way to handle this, there was a long open request in its issue tracker to support configuration of this. It even lets you specify the path to the beam files to analyze…does anyone use it? I don’t know if they do now- a few people did between the time mix began using the _build directory and when dialyxir was updated for that. Maybe someone, somewhere will want it.

Being able to configure the plt apps is handy - right now dialyze isn’t pulling transitive dependencies due to a memory problem it caused in some projects. Probably if I add that code back someone will have the same problem in dialyxir and they’ll be able to solve it in configuration by specifying only direct project dependencies plus adding specific transitives that they know they need. dialyxir supports this right now and because of it, its reasonably easy to dialyze a Phoenix project. Its just not defaulting the best way presently.

So my thinking now is we need something to be smart like dialyzer and configurable like dialyxir.

Right now this is the list of changes I’m pretty sure I need. Most of these things are pretty easy, I don’t see it taking quite a long time.

Dependencies

• Include OTP applications from current mix & dependency .app files
• Transitive by default
• Include prod dependencies only by default?

PLT

• dialyze style PLT file management, e.g. erlang/elixir core file copied into _build, app dependencies added only to this project local file
• dialyze style plt update functionality (add/remove individual modules from dependencies)
• warn users of :plt_file about new file scheme (allow suppression of warning with :no_warn) e.g. plt_file: {:no_warn,“local.plt”}
• use mix_home for core storage - config via :plt_core_path
• warn users who have the old ~/.dialyxir*.plt files but no new core file about backward incompatibility in the 0.4.0 release

Warnings

• remove all default warnings
• needs to be in the compatibility notice
• -Wunmatched_returns ?

dialyzer task

• invoke plt task if plt file is not found

[sasajuric]

This sounds great! I’m looking forward to having a single goto dialyzer tool.

A couple of comments and questions:

@jeremyjh

Transitive by default

I guess that’s fine, but there should be an opt-out for cases where a lot of memory is consumed, or if it takes long to build PLT.

@jeremyjh

warn users of :plt_file about new file scheme

I’m not sure what does this refer to?

@jeremyjh

remove all default warnings

By this, I suppose you mean not including by default unmatched_returns, error_handling, race_conditions, underspecs which would have to be explicitly included, right? If yes, then I agree. I think these options should be opt-in.

@jeremyjh

invoke plt task if plt file is not found

In fact, I think a PLT check should be done always. If PLT exists, it should be checked and updated if needed. In other words, running mix dialyzer should just work and produce a correct output regardless of the PLT state (not existing, outdated, or most recent).

Some switch for skipping PLT check could be useful to reduce the dialyzing time if the user is positive PLT is up to date.

Finally, I think it would be nice if dialyxir used dialyzer Erlang API instead of command line. That would probably bring little to end users, but it might give a bit more flexibility to dialyxir, for example for formatting output.

Thank you for doing this, and let me know if I can help somehow.

[jeremyjh]

@sasajuric

I guess that’s fine, but there should be an opt-out for cases where a lot of memory is consumed, or if it takes long to build PLT.

Yes you would be able to opt out of transitive dependencies and even including any dependencies as you can do today, only the default would change.

@sasajuric

warn users of :plt_file about new file scheme

I’m not sure what does this refer to?

The plt_file keyword lets you specify a plt file/path to use. Because the most common reason to do this is to opt-out of the global files, I think I should print a notice that they no longer have to use this feature for that reason. They can opt-out of the warning still.

@sasajuric

remove all default warnings

By this, I suppose you mean not including by default unmatched_returns, error_handling, race_conditions, underspecs which would have to be explicitly included, right? If yes, then I agree. I think these options should be opt-in.

I’m conflicted on unmatched_returns as I think its a really valuable warning, but yes they would all go. I’ll include unmatched_returns as the example in the documentation.

@sasajuric

invoke plt task if plt file is not found

In fact, I think a PLT check should be done always. If PLT exists, it should be checked and updated if needed. In other words, running mix dialyzer should just work and produce a correct output regardless of the PLT state (not existing, outdated, or most recent).

Some switch for skipping PLT check could be useful to reduce the dialyzing time if the user is positive PLT is up to date.

I was thinking to make it opt-in but you are right, it should be opt-out. The guiding principal here should be to prioritize returning correct, complete results by default, and let them optimize runtime with settings if they wish. I’ll keep a separate dialyzer.plt task so that if they do opt-out they can manually run it themselves.

@sasajuric

Finally, I think it would be nice if dialyxir used dialyzer Erlang API instead of command line. That would probably bring little to end users, but it might give a bit more flexibility to dialyxir, for example for formatting output.

Definitely I will for the PLT - I’m going to use quite a lot of dialyze code wholesale.

For the dialyzer task I am conflicted because right now I offer two mechanisms (config in mix or command-line arg) to pass arbitrary arguments to dialyzer. The examples of usage I give are to tweak warnings, but people could also use it for things i don’t anticipate such as using --raw and -o to send parseable output to a file. Also it is handy that you can copy paste the dialyzer command and tweak it instead of rerunning the task - at least I use that in debugging. I’m open to changing this at some point but probably prefer to keep it as is for 0.4.

@sasajuric

Thank you for doing this, and let me know if I can help somehow.

I’ll get a development branch pushed and things organized in the issue tracker so at least the larger items could be claimed by someone if you or anyone else wants to help, I’ll post an update.

[jeremyjh]

I’ve started the implementation, so far I have integrated the dialyze code for managing and updating the PLT, which seems to be working ok. This is in the develop branch. To contribute or follow progress, please subscribe to this master issue.

[jeremyjh]

In case anyone is following or finds this topic I wanted to post an update to mention 0.4.0 release is finally done and released on hex.pm. I thought it would take a couple of weeks but I kept adding features and then life happened and here it has been four months! I think the new task is much improved with an infusion of the dialyze PLT management code but I made a lot of other changes as well. I even wrote tests!

I do have further work planned; I want to get away from shelling dialyzer while still supporting the raw argument forwarding. I would also like to have a guard/watch type mode. If you have any other ideas for features feel free to open Github issues.

[christopheradams]

Great work @jeremyjh the new release works great for me so far and the local archive is a welcome addition.

For those upgrading from 0.3.5, check the README carefully, as there is now a useful distinction between :plt_apps and :plt_add_apps, and the options for plt_add_deps have changed.

I also found I have to add :mix and :eex to :plt_add_apps in order for Dialyxir to know about them.

[jeremyjh]

Hi thanks for checking it out and leaving the feedback Christopher. I meant to add a changes document to help with this, so thanks for the reminder. I will add a Wiki page tonight and plug it in the README.

Regarding :mix and :eex I was very conflicted on these - I considered continuing to include them whether they are specified or not. I believe the correct answer though (even outside of dialyxir requirements) is to add all your production run-time dependencies to your OTP application list; including core OTP and Elixir applications.

  def application do
    [applications: [:logger, :mix, :eex, :public_key]]
  end

edited: to clarify I mean adding run-time dependencies.

[michalmuskala]

Neither mix nor eex should be in the applications list. The applications list is used for runtime dependencies of the application - both mix and eex are not used at runtime - they are used exclusively at compile-time.

The exception is, of course, the runtime use of eex eval functions - in that case adding eex to the applications list is indeed the right thing to do.

[jeremyjh]

Yes of course - but Dialyzer warnings would only show up for runtime usage
in application code. One exception are Dev only runtime code - things like
the Phoenix live reloader - that will have runtime code in the Dev
environment only. So it should be added with :plt_add_apps only.

[hazardfn]

Hey @jeremyjh,

We use dialyxir and we recently tried upgrading to 0.4.0 but sadly I don’t THINK we are able to use it in our current development workflow and I will explain why a little here:

How we develop:

All of our projects have a central makefile with common tasks in it (test, dialyzer, credo etc.), this central makefile runs EVERYTHING inside of a pre-defined docker image called teamname-dev.

Docker Image:

Inside this docker image we have everything we need for running the tasks mentioned above, meaning we install erlang and elixir and then archive/install things like credo and dialyxir. It is also here where we build our base PLT…

As of 0.4.0 it seems you can no longer tell dialyxir to build a base PLT independent of a project - from what I can see it demands a mix file to be present but mix dialyzer.plt never did.

I like some of the additions and changes in 0.4.0 but unless there is a way to build a base PLT independent of a project I don’t think we can use it…

[jeremyjh]

The reason I made this change is I want to fully automate maintenance of the PLT and also discourage using a shared PLT between projects; it can lead to errors in analysis and/or constant changes to the PLT. You can still specify a :plt_file path though, if you do want to share a PLT file. I did think of the case people built it manually outside of a project but admit I did not consider your use case.

Would you use a shared PLT with all the application dependencies in it eventually, or you just want to build the core files ahead of time? I could make a task or arg that would just build the core files - or I could restore the dialyzer.plt task but I think it would need an argument passed to it telling the location of a PLT file to build, maybe even a list of applications to put into it.

[hazardfn]

Yeah all we want to do when we build the docker image is to build the core files ahead of time .

It would be great to have a way of doing this through dialyxir.

[jeremyjh]

Taking @michaelmuskala’s point I edited my original comment to clarify I only suggest to include production run-time dependencies in the :applications list. I also noted this in the upgrading wiki page .