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

Should we provide a specific way to document attributes? #23149

Closed
e-kayrakli opened this issue Aug 30, 2023 · 6 comments · Fixed by #25884
Closed

Should we provide a specific way to document attributes? #23149

e-kayrakli opened this issue Aug 30, 2023 · 6 comments · Fixed by #25884

Comments

@e-kayrakli
Copy link
Contributor

I'll start with context. We had assertOnGpu() standalone function that was documented in the GPU module docs. We deprecated that in favor of @assertOnGpu attribute on the loop. So, now, how can we document the new attribute? In the short term, I don't think it is a big deal to add a header under GPU technote.

In general, should there be some special support for documenting attributes in chpldoc? While sticking something in a module code can be appealing and approachable as it can be similar to what we have been doing, it seems wrong. Attributes are a compiler/tool concept. So, attributes that we have should be documented in the compiler code where we "define" them. But how?

Another question about this is how it should be rendered. An easy solution is to have an attribute index where we list every documented attribute. But it is also appealing to drop in attribute documentation into other rst files. For example, it would be great if the assertOnGpu documentation was in the GPU technote. If the same thing was redundantly in the attribute index, that wouldn't bother me. But maybe we can keep the index as just list of links where attributes are documented in related places like in spec, relevant module docs, or technotes.

Arguably, there should be a style that can set aside attributes from other things we're documenting today.

@lydia-duncan
Copy link
Member

lydia-duncan commented Aug 30, 2023

I recommend two strategies (combined):

  1. A page dedicated to attributes we support. This probably should go in the spec? Or in the documentation of the feature it is most relevant to?
  2. chpldoc documentation rules if/when we allow user-defined attributes.

@DanilaFe
Copy link
Contributor

I am interested in prototyping this. I actually think that we can do this using procedures. Since Chapel attributes support actuals, and have names, I think we can create special 'attribute procedures', which are excluded from function resolution (perhaps they implicitly have a where false block) and are named the same thing as an attribute.

/** This attribute does bla bla bla... */
@attribute
proc assertOnGpu /* where false? */ {}

/** This attribute does bla bla bla */
@attribute(namespace="gpu")
proc blockSize(size: integral) {}

This:

  • Makes it easy to mark where we want documentation for attributes to show up
  • Allows users to document their custom attributes in a similar way
  • Has the added support for documenting attribute formals

@lydia-duncan
Copy link
Member

You had me until "allows users to document their custom attributes in a similar way".

We haven't finalized the syntax for (or even if we want to support) custom attributes, and the only way to add an attribute today is to modify the compiler, so far as I know. Therefore I think we should hide any thing that might bleed implementation details and imply that users can define their own until it is actually possible for them to do so - this will avoid getting user questions about "why isn't this working, I wrote it like the docs". It will also allow us to transition from this shortcut to support that reflects the true complexity of the feature when custom attributes are possible.

@lydia-duncan
Copy link
Member

That said, I think what you've proposed is an excellent way to get support ready quickly, and I would be interested in being present in a meeting to brainstorm what exactly the output should look like

@DanilaFe
Copy link
Contributor

When I said "user provided custom attributes" I meant tool attributes -- and I thought we have already "added support for it", in that we can pass a flag to the compiler to allow certain toolspaced attributes. Either way, we could disallow the @attribute annotation (or pragma) from being used outside of module code.

@lydia-duncan
Copy link
Member

Ooooh, I see. You mean for things that are recognized and responded to in programs outside of the Chapel compiler. And it makes sense that those would not be scenarios where a user is as likely to see an example and try it in a way that would frustrate them. I buy that.

DanilaFe added a commit that referenced this issue Sep 5, 2024
Closes #23149.

This PR implements my initial vision for documenting attributes based on
procedures that implement them. In PR
#25826, I made GPU attributes
be backed by Chapel functions. One of my goals with this approach was to
be able to document these attributes using the existing Chapel mechanism
(doc comments attached to procedures). This PR does so, by adding a new
chpldoc attribute (`@chpldoc.attributeSignature`) that marks a function
as defining the signature (formals etc.) of an attribute.

Since the functions in the GPU module that back attributes are internal
(not intended to be called directly), they would normally be skipped by
chpldoc's output. This PR adjusts the behavior of chpldoc to not skip
the functions in this case.

It also adds documentation to the various attributes using this new
mechanism.

This PR depends on
chapel-lang/sphinxcontrib-chapeldomain#97, which
implements the RST-to-HTML conversion of the new 'annotation' directive
chpldoc will emit.

<img width="725" alt="Screen Shot 2024-09-05 at 10 16 43 AM"
src="https://github.com/user-attachments/assets/bcc3fbe8-301f-4727-ae92-be2968d4e0f4">

Reviewed by @e-kayrakli, @mppf, and @lydia-duncan -- thanks!

## Testing

- [x] paratest (just in case)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants