-
-
Notifications
You must be signed in to change notification settings - Fork 8
RFC 0017: Improved Annotations #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| end | ||
| ``` | ||
|
|
||
| Working title for a class/struct with an `@[Annotation]` annotation is a "class-based annotation". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions here. I guess another option is doing something like what PHP did and go with @[Attribute] to make the distinction more clear.
|
This pull request has been mentioned on Crystal Forum. There might be relevant details there: https://forum.crystal-lang.org/t/rfc-annotations-metadata-declaration-dsl/1312/14 |
| E.g. `@[NotBlank(allow_nil: "foo")]` or `@[NotBlank("foo")]` would result in a compile time error like `@[NotBlank] parameter 'allow_nil' expects Bool, not String` that is pointing at the invalid arg. | ||
|
|
||
| Class-based annotations may be applied to any language construct that accepts annotations by default. | ||
| Unlike previous existing annotations however, class-based annotations may only applied once to the same item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up implementing metadata in the initial draft of this just to avoid the case of releasing this feature, someone adds same annotation to same thing multiple times, then we introduce repeatable and have to make it default to true. Having it out of the box allows the default to be the safer false value.
text/0017-improved-annotations.md
Outdated
| To start, it'll allow the following fields, but allows for future expansion: | ||
|
|
||
| - `repeatable : Bool` - to allow an annotation to be applied multiple times to the same item, defaulting to `false` | ||
| - `targets : Array(String)` - to control what targets this annotation can be applied to ("method", "property", "class", "parameter"), defaulting to any target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to ideas on how to handle this. This is straightforward enough but 🤷.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea would be like targets: Annotation::Target[:method, :property].
text/0017-improved-annotations.md
Outdated
|
|
||
| The `#annotations` method returns annotations matching the provided type. | ||
| The provided type doesn't have to be a class-based annotation itself; but instead could be an non-annotation abstract parent type, or even a module. | ||
| A new optional `is_a` parameter (default `false`) expands the search to include annotations whose types inherit from or include the provided type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the bigger open questions. I just felt we needed this to be opt-in, otherwise you may always have to deal with getting more than you want, but still need a way to allow including child types too. Not 100% sold on this name tho.
Having the provided type be a module is also kinda interesting. But modules are also stored in the #ancestors array so kinda just get it for free.
…eter default values if defined Re-word some sentences
text/0017-improved-annotations.md
Outdated
| end | ||
| ``` | ||
|
|
||
| The compiler validates arguments provided to the annotation based on the constructor(s) of the class-based annotation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on Discord a bit, I think this warrants a bit more thought. Specifically how current annotations can accept any macro ASTNode that isn't really usable within a runtime constructor. As it stands with the PoC implementation you can do stuff like:
@[Annotation]
class Foo
def self.new(test : Crystal::Macros::Def); end
endOr how def self.new(*args, **kwargs); end could also be used to get existing annotation semantics that allow anything. But neither of these "feel" right mixing compile time only types with runtime constructors.
I'm now thinking it would be better to fully de-couple the constructors of the runtime type and that of the annotation, possibly with some sort of pseudo-overload mechanic. The annotation could still store whatever internally, but the provided args (both named and positional) would have to match one of the annotation constructors. Type validation would be a bit easier for these since they'd be using the macro ASTNode types vs having to map from the runtime types.
For ease of use, could maybe still use runtime constructors as a fallback if no annotation-constructor(s) are present. But if any annotation constructor(s) are present, they must be used and can no longer fallback on the runtime ones.
This would help with the compile-time only/zero-overhead use case that existing annotations currently serve, but with added benefit of input argument validation.
| end | ||
| ``` | ||
|
|
||
| Class-based annotations may have one or more `macro annotated` macros defined within it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sold on this name, but just went with this for now.
|
|
Thanks for the feedback! I just want to clarify that this proposal is not allowing annotations to be queried at runtime of course. They still are a compile-time only thing. The proposal is mainly allowing other types to be used as annotations, and exposing a new macro method to generate a Call to instantiate that type with the args stored in the annotation at runtime. It does add two meanings to a given type, but the runtime and compile-time sides are still separate things. To me it just makes sense that you can use one type as the source of truth. Mainly so you don't have to think about I have to use x to annotate something but y when I'm doing it manually. I do think being explicit and having the macro be like I would argue that a beginner isn't likely going to be using annotations anyway, so seems reasonable to treat them as more of an intermediate/advanced concept. I made this RFC as kind of how I'd like to see/use annotations. There is definitely scope that could be cut to make things simpler; e.g. falling back on the runtime constructors, the pseudo-overload mechanic of the macro annotation constructors, using default values from the used overload, etc. |
I think being able to interact with runtime types from within macro land on a deeper level could be an improvement to the language in general. However, I don't think this should be limited to annotations and I think this requires a more holistic approach. Why should only annotations gain this special treatment of deducting macro behaviour from runtime behaviour? If it's possible for the compiler to map runtime behaviour to macro behaviour, why shouldn't I be able to instantiate or use structs as values inside macros? |
Probably just because this was the first time that the idea was brought up. I'm not really aware of any other discussions around this idea. As I mentioned we could ultimately defer that part of the implementation and handle it as part of a dedicated RFC/track of work. Would really just mean you have to define the macro constructors which would be a bit annoying, but not the end of the world. |
Preview: https://github.com/crystal-lang/rfcs/blob/improved-annotations/text/0017-improved-annotations.md
Proof of concept PR: crystal-lang/crystal#16527
Am planning on play testing this with Athena Validator, but in the meantime looking to gather some feedback :)