change(export_used_types): don't warn on behaviour callbacks

[bormilan]

Description

I implemented a way to get the callbacks for the behaviors in the module and subtract them from the function used in the rule's logic. This approach ensures that all callback types are ignored.

Additionally, there was a test that expected a warning from a module that no longer generates one. So, I removed that assertion, but I'm not sure if that was the correct decision.
Closes #308.

• I have read and understood the contributing guidelines

[elbrujohalcon]

I'm not sure about this fix.
This will still fail on the following scenarios:

• On gen_statem, ct_suite, and other behaviours with dynamic callbacks, until Can we add a way to specify dynamic callbacks? erlang/otp#4847 is taken care of.
• On custom behaviours, defined by the user in the same app that we're analyzing.

I'm not rejecting the PR entirely, but I'll leave it in the hands of @paulo-ferraz-oliveira to decide if this improving is worth merging or not.

[elbrujohalcon]

A try…catch is needed here, just in case someone introduces a typo (-behaviour(gen_srvr).) or uses a behaviour that's defined outside of OTP (-behaviour(my_behaviour).).

[bormilan]

You are absolutely right. It was my best shot to solve this issue, until the issue erlang/otp#4847 will be solved.

I'm curious what do you think about it, or do you think that we should wait for the mentioned otp change request to be solved?

[elbrujohalcon]

You are absolutely right. It was my best shot to solve this issue, until the issue erlang/otp#4847 will be solved.

I'm curious what do you think about it, or do you think that we should wait for the mentioned otp change request to be solved?

I think we should wait… but let's hear what @paulo-ferraz-oliveira has to say.

[paulo-ferraz-oliveira]

I wouldn't mind waiting for the OTP changes, but these can take a while to happen...

[paulo-ferraz-oliveira]

@elbrujohalcon, otoh, what would prevent us from implementing an incomplete solution (I guess it's what this PR is doing), for the time being, that didn't take dynamic callbacks into account? We could add this caveat to the documentation, and create a task to deal with the rest when the OTP issue is solved.

[elbrujohalcon]

@paulo-ferraz-oliveira it has to be super-clear that this is a partial solution (in the docs and everywhere else), so that we don't get complaints about incorrect or missing warnings.
I'm not saying that we shouldn't do it. In my mind it's not worthy, but if you think it's useful, as long as you don't create false hopes, go for it!

[paulo-ferraz-oliveira]

We could make it as an option, then? I don't mind not having it, but I think the benefits outweigh the disadvantages. And how to you foresee complaints? This will warn less, not more, right?

[elbrujohalcon]

@paulo-ferraz-oliveira either…

• We only avoid the warning if we know the behaviour (i.e., we have a list of known behaviours) and somebody creates a behaviour on top of gen_server and gets the warning on missing exported types. Or…
• We just avoid the warning for any behaviour (i.e., if the module defines a behaviour any exported function can be a callback) and someone doesn't get the warning that they're expecting the rule to emit.

It's not a big deal, but yeah… keep it as an option, that's a good idea. Be clear in the docs about the option that it's heuristically implemented and it doesn't cover all the cases (or if you go the other way… it covers too many cases) and it would be fine with me.

[elbrujohalcon]

The way in which this is implemented now will only ignore non-dynamic callbacks from modules implementing behaviours that are already known at compile time (i.e. OTP behaviours without dynamic callbacks).
@paulo-ferraz-oliveira / @bormilan: You should decide if this is what you want.
Alternatively, you can just ignore the module entirely if it implements an unknown behaviour, or a behaviour that we know that it has dynamic callbacks (e.g. gen_statem).

[elbrujohalcon]

It's good, @bormilan … but please add:

• A note to the rule docs explaining that the rule will ignore regular (i.e. not dynamic) callbacks of OTP behaviours (And maybe other behaviours if their behaviour_info is accesible at compile time).
• A module implementing gen_statem that fails the rule because a non-exported type is used in the spec of dynamic callback.
• A module implementing a custom behaviour that fails the rule because a non-exported type is used in a regular callback.

[bormilan]

I'm not sure if this implementation is worth pursuing. The concerns you've raised are entirely valid, and if we need to discuss an issue for this long, it may be a sign that we should reconsider proceeding.

Regarding the suggestion to "ignore the module entirely if it implements unknown behavior or behavior with dynamic callbacks," I believe that adding this could be beneficial, but it's only part of the overall goal.

In conclusion, I am uncertain about my feelings on this matter. Perhaps it is a sign to wait before moving forward. I began working on this task because I am focused on the 4.0.0 milestone, not out of personal interest.

If we decide to hold off, we can move this issue to a different milestone.

[elbrujohalcon]

@bormilan I changed the milestone in the issue to "eventually"… and took this chance to reorganize tickets a bit… so I added a few more to the 4.0.0 milestone. All of them qualify as low-hanging fruit in principle.

[bormilan]

Thank you! I will do this too, but I feel it's better to wait.