Skip to content
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

Add argless #annotations overload #9326

Merged
merged 11 commits into from
Oct 23, 2022

Conversation

Blacksmoke16
Copy link
Member

Resolves #9322. Was pretty simple so just went and did it.

@bcardiff
Copy link
Member

I'm not convinced that name is the proposed way to check the annotation type.

I'm not convinced that getting all the annotations together is useful. Wouldn't you need to know the type to know what properties and semantics give to the annotation? If so, why the methods that return only the annotation of a given type is not enough? You will query based on the type you want to give semantics to.

I've expressed this before, that querying annotation should have a design cycle regarding how the type hierarchy plays along. Are annotations attached to descendant types? Can we query for exact on inherited annotations? C# reflection API is a good source for inspiration here. Without discussing those things I think that all these reflection methods are experimental.

I thought the issue was about making the annotation information available on runtime.

@Blacksmoke16
Copy link
Member Author

I'm not convinced that name is the proposed way to check the annotation type.

This was more so to return the name of the annotation (i.e. whats in between the @[]), rather than its type. Similar to TypeNode#name or MetaVar#name.

why the methods that return only the annotation of a given type is not enough? You will query based on the type you want to give semantics to.

I thought the issue was about making the annotation information available on runtime.

The idea behind #9322 (and by extension this PR) was to provide a simple workaround for the lack of runtime reflection by exposing the data required to get a similar effect. For example, building out a hash (at compile time) of the annotations and data within the annotations, then using this hash at runtime; can look at my example in the original issue for example code.

I've expressed this before, that querying annotation should have a design cycle regarding how the type hierarchy plays along.

Annotations in general could use some love. There are plenty of optimizations, improvements, and such that could make them even more useful. However, I think that can happen independently of this PR. The API for #all_annotations wouldn't change. The data the gets returned by it could, or additional functionality could be added in the future without changing current semantics.

@Blacksmoke16
Copy link
Member Author

Bump.

spec/compiler/semantic/annotation_spec.cr Outdated Show resolved Hide resolved
spec/compiler/semantic/annotation_spec.cr Outdated Show resolved Hide resolved
spec/compiler/semantic/annotation_spec.cr Outdated Show resolved Hide resolved
spec/compiler/semantic/annotation_spec.cr Outdated Show resolved Hide resolved
spec/compiler/semantic/annotation_spec.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
@Blacksmoke16
Copy link
Member Author

Bump again. Any thoughts on this @RX14 @waj @asterite?

@Blacksmoke16
Copy link
Member Author

Bump again 🙈.

@Blacksmoke16
Copy link
Member Author

Don't suppose I can get another review on this?

@asterite
Copy link
Member

But it's not clear that this is what we want, so first we must discuss this instead of reviewing this PR. But it's unlikely this will happen before 1.0. And after 1.0 it's not a breaking change, so...

straight-shoota pushed a commit that referenced this pull request Nov 15, 2021
Adds:

* `Annotation#name`
  * #9326 converts the name to a `MacroId` on behalf of the user. This PR returns the `Path` directly (like `Generic`).
* `{Case,When}#exhaustive?`
* `Call#global?`
* `Def#{abstract?,free_vars}`
@Blacksmoke16 Blacksmoke16 changed the title Add #all_annotations, and Annotation#name macro methods Add #all_annotations macro method Sep 5, 2022
@Blacksmoke16 Blacksmoke16 changed the title Add #all_annotations macro method Add #all_annotations macro method Sep 5, 2022
@Blacksmoke16
Copy link
Member Author

Annotation#name has since been added via #10811. I merged in master and updated this PR to only add #all_annotations. Assuming that's still something we want to consider adding.

I personally would find this method useful, but if there are other higher level discussions happening related to this (e.g. maybe #9802) feel free to close.

@Blacksmoke16 Blacksmoke16 changed the title Add #all_annotations macro method Add argless #annotations overload Sep 5, 2022
@straight-shoota straight-shoota added this to the 1.7.0 milestone Oct 3, 2022
@straight-shoota straight-shoota merged commit 09868bb into crystal-lang:master Oct 23, 2022
@Blacksmoke16 Blacksmoke16 deleted the #all_annotations branch October 23, 2022 22:38
lbguilherme pushed a commit to lbguilherme/crystal that referenced this pull request Oct 24, 2022
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Accessing all annotations on an ivar/type/def
6 participants