Remove behaviour checking from erl_lint

[richcarl]

<tongue-in-cheek>
Since the Erlang compiler should be pure, not inspecting other beam files during compilation, and since its warnings should be dependable and deterministic, this patch removes the behaviour checking from the Erlang linter. Such checks should instead be performed by a separate tool that has a whole-system context to work with.

No backwards compatibility option is implemented, since this can only be considered a bug fix.
</tongue-in-cheek>

[psyeugenic]

Yep, I totally understand. 😃

[essen]

Perhaps it should be moved to xref rather than deleted entirely?

And more generally, it seems to be that the various tickets are about xref being hard to use, so maybe we can do something to improve that? Tools make it simpler (for example https://github.com/inaka/xref_runner/blob/master/src/xref_runner.erl), maybe this kind of improvement can be imported into xref directly, with sane defaults? What do you think?

Maybe something like xref:check(App) and xref:check(App, Opts) with the paths where to find modules being taken from ERL_LIBS/code server. If the default options don't have any possible false positives then tools can easily call it with defaults once compilation is done automatically (and optionally let the user add more options).

[josevalim]

I had originally written a reply for this but since it was tongue in cheek, I decided to not ruin the party. :) But since replies are getting serious, I thought I would add my two cents.

When using -behaviour(some_module)., I am declaring a compile-time dependency on an external module. If that module is available, it will be loaded. If preferred, it is something we could make it more explicitly documented. On the other hand, the other warning relied on function calls which never require the module to exist at compile time.

More importantly, although this check also depends on compilation ordering between modules, absence of warnings in this case means there is no bug (the ordering is correct), while absence of warnings on the previously proposed warning did not imply there were no invalid calls, it just meant we may not have caught them and that may lead to a false sense of security - "oh, no warnings, so there are no invalid references!".

Yes, I know. I must be super fun at parties.

[okeuday]

@essen I think an issue that might be good to bring up is "What does xref offer that dialyer does not? Why waste time using xref during development, if dialyzer will detect all the problems xref discovers?". I understand xref is good for programmatic processing of dependencies. An important aspect of this is not wasting a developers time with 2 tools where only 1 is necessary (for the typical developer's use, not talking about the internal guts of reltool using xref).

@josevalim As previously discussed (#1267 (comment)), the compiler should be dependable with the same output given for any ordering to make sure a single input has a single output. That approach allows the compiler to be used at a higher level in a dependable way, to be part of a dependable software stack. We don't want ambiguous output during parallel compilation.

[josevalim]

@okeuday that's only true as long as you can avoid compile time dependencies. For example, if you have a module A that depends on parse transform B, B must be compiled before A. I consider behaviours, given how they work today, also to be compile time dependencies.

[essen]

Dialyzer is neither a magic wand that can detect every problem, nor is it quick to run (like the behavior check and the other checks in xref are). Dialyzer also requires magnitudes more memory, something which you don't always have available.

For the story, my previous laptop was only able to run Dialyzer on Cowlib if I shut down most of my programs. Otherwise it would end up running out of memory. I would never run it automatically post compile considering the costs. Xref on the other hand, assuming there's no possibility of false positives in the checks I setup, I would gladly do that.

[okeuday]

@josevalim Behaviours are always a runtime dependency that might happen to have compile-time checking that we are talking about removing. When you compare parse transforms to behaviours, there is a clear dependency of the module's compilation on the parse transform(s) it uses. However, behaviours are only providing type specification information which would be more applicable to dialyzer checking, since the scope goes beyond a single module and often beyond a single Erlang/OTP application. Parse transforms create ambiguity in the compilation output, due to the lack of tracking of the transforms being used (the parse transform state and versioning), so their use will not provide a good example of dependable development.

@essen I agree that dialyzer memory use and runtime is a problem. Sometimes it is related to "experimental" (i.e., unfinished) functionality that acts as a landmine for potential use, to create undefined memory use and/or runtime. If xref is required to have usable analysis of Erlang beam files when runtime and memory use is a factor (as it is for most development), then perhaps that is the way it should be. Having two competing tools for development analysis doesn't help developers develop though, it only fragments the efforts on the separate tools and increases the potential of neglecting a tool during Erlang development that leads to development errors later (making development slower and more error-prone than it might be in other programming languages).

[essen]

They don't have the same scope. An unknown module/function is an error in xref, but not in Dialyzer, for example.

[okeuday]

@essen That is a superficial difference that could easily be resolved by making it an error in dialyzer, perhaps with a warnings as errors option. So, that is probably a bad example, but it is easy to understand that xref will always be quicker by doing less.

[josevalim]

@josevalim Behaviours are always a runtime dependency that might happen to have compile-time checking that we are talking about removing.

Not "might". It always happens today and it is documented as such. I can rely on it and I do rely on it.

Not only that, removing this check is backwards incompatible. It changes a documented compiler behaviour and it will make many available resources out there stale. For example, this is a quote straight from Joe's book:

The keyword -behaviour is used by the compiler so that it can generate warning or error messages if we forget to define the appropriate callback functions.

Sure, Erlang/OTP needs to move forward and that sometimes will cause resources to become stale. So even if this change was correct, which I don't believe it is, I doubt it would be worth the backwards compatibility cost.

However, behaviours are only providing type specification information which would be more applicable to dialyzer checking

Behaviours provide two things: a list of function/arity that must be implemented and their type specifications. I would not throw the first and immediate check away only because Dialyzer may check the specs later.

The whole point of behaviours to me is the compile-time check. Claiming they should be runtime-only and checked only by dialyzer (and similar tools) defeats a big part of their purpose and their importance in OTP.

[essen]

@okeuday It's only a bad example if you believe Dialyzer should know about all dependencies and modules you call. I don't. I don't care what Dialyzer says about how I call lager[1], or eprof, or similar. These modules and these calls are not what my program is about and the checks Dialyzer would do for them are more a hindrance than help.

That's where the scope difference is, by the way. Dialyzer does more than checking whether a function exists. I only want to ensure the functions exists for these cases. I don't want to match the returns or look too closely at the types for these calls. On the other hand I want that for every other calls.

[1] There is an infamous "Expression produces a value of type 'ok' | {'error','lager_not_running' | {'sink_not_configured','lager_event'}}, but this value is unmatched" that is best ignored by not including lager in the PLT, rather than change your source everywhere.

[okeuday]

@josevalim Arguing that documentation is a reason to avoid improvements doesn't make any sense unless Erlang/OTP is meant to be a dead platform that is unable to be improved. Documentation can always be improved, if books are popular, they can always get new editions, life goes on... Fixing legacy problems is important for moving forward, rather than depending on old broken behavior. The compile time behaviour checking can always be moved to a more neutral linter executable which may not be xref, dialyzer, or the make module, but whatever it is, it is needed as a normal Erlang development tool to handle multiple modules efficiently and consistently.

@essen You may not care about what dialyzer says, however it is required to get checking similar to what is provided by the linter of a statically typed language's compiler, so ignoring dialyzer is ignoring a very critical part of Erlang/OTP when you compare Erlang to other programming languages. This matters as systems grow in size.

[essen]

I am not trying to have a statically typed language or get close to one, I am trying to find errors in my programs. Dialyzer telling me things I don't care about will not help me find those errors that do matter. Dialyzer telling me I don't check the return value from lager will not help me remove errors when my system grow.

Anyway you are basically arguing about the "one true way", and I'm saying different people have different views and different solutions work out for different people. I'll leave it as that.

[josevalim]

@okeuday please re-read my reply. I didn't say we should never change Erlang because of the documentation. Quite the opposite, I wanted to bring attention that this feature is documented, used by developers, it is likely part of some workflows as well as many resources. Therefore changes are welcome as long as their benefits outweigh the costs. I am all in for introducing an efficient and consistent way of checking across modules but, until such mechanism is in place, the warning should stay as is. At the end of the day, this is not my call to make, and since I don't have anything else to add, I am glad to leave it at this as well.

[kostis]

Dialyzer telling me things I don't care about will not help me find those errors that do matter. Dialyzer telling me I don't check the return value from lager will not help me remove errors when my system grow.

Sorry to intervene, but these two sentences (and many others by @essen in this thread) are just misleading (if not mis-information). AFAIK, Dialyzer does not tell you this (and the other things you claim) by default. It tells you this if you ask it explicitly by enabling the -Wunmatched_returns option. If you do not like to see such kinds of warnings, simply do not enable this option! It's as simple as that. There are reasons why tools come with options which are enabled by default and other options which are not.

This is very similar to the old story with the doctor.
Patient: "Doctor, if I do this strange movement, sometimes it hurts."
Doctor: "Don't do this strange movement!"

edit: the point is that you cannot blame a tool for doing what you explicitly asked you to do.

[essen]

I've never claimed it did this by default. What I was saying is that I want this check, but not on lager because I never care about returns of log functions. So I don't add lager to the PLT and all is well in the world.

The confusion probably comes from the hypothetical "Dialyzer with missing module/functions being errors" that @okeuday mentioned, which would not fit my workflow (I wouldn't be able to leave lager out of the PLT anymore if I wanted that check). I'm happy with Dialyzer not having such a check, and instead using xref for that purpose.

[kostis]

I've never claimed it did this by default. What I was saying is that I want this check, but not on lager because I never care about returns of log functions. So I don't add lager to the PLT and all is well in the world.

The confusion probably comes from the hypothetical "Dialyzer with missing module/functions being errors" that @okeuday mentioned, which would not fit my workflow (I wouldn't be able to leave lager out of the PLT anymore if I wanted that check).

This is definitely getting off topic, but @essen did you know you can suppress an unmatched return dialyzer warning simply by matching the return of the call with an underscore (_) ? True, it requires some (small) action from the programmer, but this will allow you to have lager in the PLT, enable the check for the warnings that you want to see and not be bothered by warnings that you do not want. Would this fit your workflow?

[essen]

Yes, I am aware, and mentioned it in the discussion above ("change your source everywhere"). It's interesting for your own code because sometimes you do forget to use the returned value, but for lager it doesn't provide any value.

If you got hundreds of lager calls in the code base, going back to change them is just time better spent elsewhere. I of course know you can do things right in the first place, but you don't always have that luxury.

Know that for the specific case of lager, more often than not I've seen people simply use grep to discard that warning from reports. I find excluding lager from the PLT more useful since it doesn't eat away the exit status, so any other warnings are properly treated as errors by the CI tools.

Ultimately Dialyzer is a tool that should help me find errors, so I have no problems taking shortcuts when the Dialyzer warnings don't help me.

[psyeugenic]

I will close this PR now. I think the intentions of @richcarl's PRs are good ones. The compiler should be much better at giving helpful advice and checking what can be checked. However, the key points are determinism and scope. This sort of thing should really be discussed on the Erlang Mailing before hand.

I believe it would be beneficial if we had an OTP compiler that operates in an application context instead of an Erlang context. Output from such a compiler would be application archives and loaded to, or purged from, the beam as a single unit. We could do intra-application type checking and module inlining without fear of inconsistency. This will not bring us all the way though. For checks at system level, i.e. outside application context, we would still need an external tool.