christopheradams

christopheradams

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

0
Post #21
sasajuric

sasajuric

Author of Elixir In Action

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

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.

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.

0
Post #22
fishcakez

fishcakez

Ecto Core Team

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

1
Post #23
fishcakez

fishcakez

Ecto Core Team

[quote="sasajuric, post:23, topic:1003]
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]
[/quote]

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.

[quote="sasajuric, post:23, topic:1003]
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.
[/quote]

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

0
Post #24
sasajuric

sasajuric

Author of Elixir In Action

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?

And what was the answer? :slight_smile:

0
Post #25
fishcakez

fishcakez

Ecto Core Team

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 :smile: and get behind dialyxir. I do not want to do this work.

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

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.

0
Post #26
jeremyjh

jeremyjh OP

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
2
Post #27
sasajuric

sasajuric

Author of Elixir In Action

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

A couple of comments and questions:

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.

I’m not sure what does this refer to?

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.

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.

1
Post #28
jeremyjh

jeremyjh OP

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.

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.

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.

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.

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.

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.

1
Post #29
jeremyjh

jeremyjh OP

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.

1
Post #30