Annotate @doc metadata when something is a guard

[lackac]

Related to #7845.

This is a draft implementation after doing some experimentation with various ways of doing this. The current approach introduces the least amount of changes to make it work.

Other approaches:

1. use :defguard/:defguardp as kind and move the @doc guard: true call to define/4. store_definition would be called with :defmacro/:defmacrop.

This would have the added benefit of having more precise error messages in the assert_* functions.

2. use :defguard/:defguardp

This would be the most invasive change as there are a lot of places where we do something for :defmacro/:defmacrop that would have to be extended for guards. However, this would allow us to add guard: true to the metadata in Module.compile_definition_attributes/6 which could in turn allow making guard a reserved metadata key. This last benefit is lost however if we need to add @doc guard: true to builtin guards like is_binary which are defined as plain functions.

Another thing to consider is whether to make the guard metadata key reserved. If we don't that might result in confusing behaviour if someone starts using it for other purposes (however unlikely). If we do, that will require a workaround for builtin guards.

[josevalim]

I like the current approach. As I don't think we should add a new kind due to the complexity of the changes (and likely breaking changes too).

Another thing to consider is whether to make the guard metadata key reserved.

I would make it reserved. The check could see if the current module is Elixir.Kernel and, if not, raise accordingly. In case we don't have the current module when we check right now, maybe we could do the check later, when we are storing the metadata in the docs table instead of when it is set.

[lackac]

@josevalim I agree, I wasn't comfortable starting on the more invasive changes.

I would make it reserved. The check could see if the current module is Elixir.Kernel and, if not, raise accordingly.

We have access to the module, but it's going to be a bit more complex than that. Currently we're doing the check in Module.preprocess_doc_meta/4 which is called from Module.put_attribute/6. With this approach we can't distinguish between a @doc guard: true call that comes from regular module source and one that is generated by defguard.

I'm thinking of creating an internal (but not private) function put_doc_metadata that would effectively replace the put_attribute clause that we're currently using now. This way we'll be able to call this from do_at for regular @doc keyword() calls with a flag that enables validation of reserved keys while we can also call it from other internal places to skip validation (e.g. defguard).

Do you think this would be the right approach?

[josevalim]

Oh, I see. I don't like having private functions like that for Elixir only because it means folks can't have their own defguard abstraction. I think we need to bite the bullet here and not make :guard reserved then.

[lackac]

Annotated all the existing guards and added tests.

The current behaviour of IEx is that it uses defguard in the signature for a guard that is defined as a macro (all that use defguard, and Bitwise operators). I think that using defguard for functions could be misleading. Their docs still state that they are allowed in guards, so I wouldn't list guard: true in the metadata section either. We can make different decisions in ExDoc where this could be used as a grouping mechanism.

[fertapric]

The current behaviour of IEx is that it uses defguard in the signature for a guard that is defined as a macro (all that use defguard, and Bitwise operators). I think that using defguard for functions could be misleading. Their docs still state that they are allowed in guards, so I wouldn't list guard: true in the metadata section either. We can make different decisions in ExDoc where this could be used as a grouping mechanism.

Maybe it's better to mark all the functions and macros that can be used as guards with @doc guard: true and use defmacro as signature for defguard?

OR...

Here is a crazy? idea

What do you think about having both @doc guard: true (used with defguard) and @doc allowed_in_guard: true for Kernel, Bitwise and custom macros?

If we go down this path, we could mark functions and macros accordingly. In addition, IEx could use defguard as signature for @doc guard: true and allowed_in_guard: true as metadata for the rest (displayed close to since: and deprecated:).

[josevalim]

I think guard: true is enough to derive all of those scenarios, since custom guards can only be macros. Any function guard is built-in.

[josevalim]

That said, we may go with :allowed_in_guard if we believe it is a clearer name than :guard. When it comes to the implementation though, this patch is perfect. :)

So, :guard or :allowed_in_guard?

[fertapric]

I think guard: true is enough to derive all of those scenarios, since custom guards can only be macros. Any function guard is built-in.

Right. Any macro marked with @doc allowed_in_guard: true and outside Kernel (and Bitwise) should be a defguard :)

To clarify, my main concern was those macros in Kernel that are allowed in guards but are not marked with @doc allowed_in_guard: true, like Kernel.is_nil/1. I would prefer to mark those to the detriment of displaying the signature ofdefguard in IEx with defmacro :)

So, :guard or :allowed_in_guard?

I think :allowed_in_guard is clearer.

[josevalim]

To clarify, my main concern was those macros in Kernel that are allowed in guards but are not marked with @doc allowed_in_guard: true, like Kernel.is_nil/1.

Good point. We should also mark any macro allowed in guards, such as is_nil/1, as @doc guard: true. It can even show up as defguard in IEx. The only reason why we can't use defguard itself is for bootstrap reasons. From the user perspective, it is exactly the same as defguard.

[lackac]

I tried marking all existing guards with `@doc guard: true`. Let me know if I missed any. Currently on mobile and it would be difficult to check.

…

On 2018. Jul 13., Fri at 17:53, José Valim ***@***.***> wrote: To clarify, my main concern was those macros in Kernel that are allowed in guards but are not marked with @doc <https://github.com/doc> allowed_in_guard: true, like Kernel.is_nil/1. We should also mark any guard, such as is_nil/1, as @doc guard: true. It can even show up as defguard in the docs. The only reason why we can't use defguard itself is for bootstrap reasons. From the user perspective, it is exactly the same as defguard. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7893 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAFSGxq5HL6tlAB80V9BasvRzegXlqOks5uGMJ3gaJpZM4VOGqp> .

[fertapric]

@lackac we are talking about this part:

The current behaviour of IEx is that it uses defguard in the signature for a guard that is defined as a macro (all that use defguard, and Bitwise operators). I think that using defguard for functions could be misleading. Their docs still state that they are allowed in guards, so I wouldn't list guard: true in the metadata section either.

The idea is to mark any macro allowed in guards, even if IEx does not display the proper signature.

It can even show up as defguard in IEx. The only reason why we can't use defguard itself is for bootstrap reasons. From the user perspective, it is exactly the same as defguard.

However, I would go with defmacro in IEx. I think some developers might think that macros with the defguard signature in IEx are only allowed inside guards. For example, I actually checked if Elixir was raising any errors if the defguard was used outside of a guard. On the other hand, defmacro + allowed_in_guard: true (or guard: true) in IEx seems easier to understand :)

[josevalim]

Here are the ones I found: in/2, and/2, or/2 and is_nil/1.

There are other things that are allowed in guards, such as sigils and left..right, but I don't think those are what people consider to be "guards". For this reason, maybe guard: true is a better name than allowed_in_guards: true, as what is allowed in guards is a bit more than the guards themselves.

I guess then it becomes slightly debatable if we should say that and, or and in should be tagged as guards. But I would say yes, especially because they return booleans.

[michalmuskala]

in is not always allowed, so there's some caveats to that.

[fertapric]

There are other things that are allowed in guards, such as sigils and left..right, but I don't think those are what people consider to be "guards". For this reason, maybe guard: true is a better name than allowed_in_guards: true, as what is allowed in guards is a bit more than the guards themselves.

guard: true 👌 (you changed my mind)

[josevalim]

in is not always allowed, so there's some caveats to that.

@michalmuskala good point. But the error messages are good in this case so I would put it on the list for documentation purposes and let the error message guide users. I think that's better than not tagging it. Thoughts?

[fertapric]

Last comment, promise 😅

I would go with defmacro here when displaying macros marked with @doc guard: true. As I mentioned in one of the comments, some developers might think that macros with the defguard signature are only allowed inside guards (like the macros in Bitwise and Kernel). I actually checked if Elixir was raising any errors if a guard defined with defguard was used outside of a guard.

Could defmacro + guard: true be easier to understand? (this would require to print guard: true here)

Sorry for commenting this again, I just wanted to be sure this was not overlooked :)

[josevalim]

Given both defmacro and def can be used in guards, I guess showing it as guard: true is the best way to go as it would apply in both cases. 👍

[josevalim]

❤️ 💚 💙 💛 💜