Allow 'doc' attribute

[wawel37]

Related to this PR in scarb

---

This change is 

[orizi]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @piotmag769, and @wawel37)

---

crates/cairo-lang-syntax/src/attribute/consts.rs line 47 at r1 (raw file):

/// An attribute to control various aspects of documentation.
pub const DOC_ATTR: &str = "doc";

i like to see it added with relevant diags on bad usage. (and basic usage.)

Code quote:

/// An attribute to control various aspects of documentation.
pub const DOC_ATTR: &str = "doc";

[piotmag769]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-syntax/src/attribute/consts.rs line 47 at r1 (raw file):

Previously, orizi wrote…

i like to see it added with relevant diags on bad usage. (and basic usage.)

Also I believe this implementation may be insufficient for diagnostics according to the PR description i.e., use with arg via #[doc(hidden)]. Is there any standard way to generate diagnostics for structured attributes? From what I know it is usually done in plugins

[maciektr]

crates/cairo-lang-syntax/src/attribute/consts.rs line 47 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Also I believe this implementation may be insufficient for diagnostics according to the PR description i.e., use with arg via #[doc(hidden)]. Is there any standard way to generate diagnostics for structured attributes? From what I know it is usually done in plugins

@piotmag769 For clarification, your point is that we should validate arguments used with this attribute (like Ori has suggested), i.e. make sure only doc(hidden) is allowed for now and return diagnostics on any other usage (e.g. doc(something: else)). Right?
Yes, I think that would normally be done as a plugin (see cfg - config plugin for example).

[piotmag769]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-syntax/src/attribute/consts.rs line 47 at r1 (raw file):

Previously, maciektr (maciektr) wrote…

@piotmag769 For clarification, your point is that we should validate arguments used with this attribute (like Ori has suggested), i.e. make sure only doc(hidden) is allowed for now and return diagnostics on any other usage (e.g. doc(something: else)). Right?
Yes, I think that would normally be done as a plugin (see cfg - config plugin for example).

Yep, that's my point

[wawel37]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-syntax/src/attribute/consts.rs line 47 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Yep, that's my point

Done.

[maciektr]

crates/cairo-lang-defs/src/db.rs line 262 at r1 (raw file):

        // TODO(orizi): Remove this once `starknet` is removed from corelib.
        STARKNET_INTERFACE_ATTR,
        DOC_ATTR,

You no longer need it, if you declare a MacroPlugin - it can declare attributes itself.

[maciektr]

crates/cairo-lang-syntax/src/attribute/consts.rs line 47 at r1 (raw file):

Previously, wawel37 (Mateusz Kowalski) wrote…

Done.

You can remove the constant from here now that you have a plugin

[maciektr]

Reviewed 1 of 2 files at r1, 3 of 5 files at r2.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @orizi, and @wawel37)

[wawel37]

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @maciektr, and @orizi)

---

crates/cairo-lang-defs/src/db.rs line 262 at r1 (raw file):

Previously, maciektr (maciektr) wrote…

You no longer need it, if you declare a MacroPlugin - it can declare attributes itself.

Done.

---

crates/cairo-lang-syntax/src/attribute/consts.rs line 47 at r1 (raw file):

Previously, maciektr (maciektr) wrote…

You can remove the constant from here now that you have a plugin

Done.

[piotmag769]

Reviewed 4 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @maciektr, @orizi, and @wawel37)

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 33 at r3 (raw file):

}

fn get_diagnostics<Item: QueryAttrs>(

The error messages should list what is possible/what was expected. E.g. "Expected arguments" doesn't tell what the possible arguments are

[piotmag769]

Reviewed 1 of 5 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @maciektr, @orizi, and @wawel37)

[wawel37]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @maciektr, @orizi, and @piotmag769)

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 33 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

The error messages should list what is possible/what was expected. E.g. "Expected arguments" doesn't tell what the possible arguments are

Done.

[orizi]

Reviewed 1 of 3 files at r4, all commit messages.
Reviewable status: 5 of 7 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @maciektr, @piotmag769, and @wawel37)

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 14 at r4 (raw file):

const SUPPORTED_ARGS_ERROR_MESSAGE: &str = "hidden";

impl MacroPlugin for DocPlugin {

i don't particularly like this as a plugin - assuming you are using it somewhere else later in the db.

we (in general) don't trust the fact that macros ran to make sure salsa db actions are valid.

how are you planning on using doc?

[maciektr]

crates/cairo-lang-plugins/src/plugins/doc.rs line 14 at r4 (raw file):

Previously, orizi wrote…

i don't particularly like this as a plugin - assuming you are using it somewhere else later in the db.

we (in general) don't trust the fact that macros ran to make sure salsa db actions are valid.

how are you planning on using doc?

For now, the plan is to skip items with doc(hidden) attributes when collecting docs in scarb-doc (drafted in software-mansion/scarb#1597).

There has been ideas about using doc to provide documentation in future, like in rust (https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html), but nothing is designed / worked on now.

We also want to use the doc(hidden) in Cairo repository (to hide docs for some auto-generated items), so that's why we want to define it here.

[piotmag769]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @maciektr, @orizi, and @wawel37)

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 33 at r3 (raw file):

Previously, wawel37 (Mateusz Kowalski) wrote…

Done.

I still think some of them are uninformative "Expected identifier". The diagnostic goes to user, it should be user friendly

[orizi]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @maciektr, and @wawel37)

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 14 at r4 (raw file):

Previously, maciektr (maciektr) wrote…

For now, the plan is to skip items with doc(hidden) attributes when collecting docs in scarb-doc (drafted in software-mansion/scarb#1597).

There has been ideas about using doc to provide documentation in future, like in rust (https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html), but nothing is designed / worked on now.

We also want to use the doc(hidden) in Cairo repository (to hide docs for some auto-generated items), so that's why we want to define it here.

so lets not make it doc for - but external-attributes-validation or something?

[maciektr]

crates/cairo-lang-plugins/src/plugins/doc.rs line 14 at r4 (raw file):

Previously, orizi wrote…

so lets not make it doc for - but external-attributes-validation or something?

But we also want to use it in code generated by say starknet plugin - so that would suggest it also needs to be defined in the compiler itself?

[orizi]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @maciektr, and @wawel37)

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 14 at r4 (raw file):

Previously, maciektr (maciektr) wrote…

But we also want to use it in code generated by say starknet plugin - so that would suggest it also needs to be defined in the compiler itself?

it is fine it is defined here - just no reason to bound a plugin that only adds additional errors to be based only on a single attribute - this is just a suggestion for a rename.

[wawel37]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @maciektr, @orizi, and @piotmag769)

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 33 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

I still think some of them are uninformative "Expected identifier". The diagnostic goes to user, it should be user friendly

Done.

---

crates/cairo-lang-plugins/src/plugins/doc.rs line 14 at r4 (raw file):

Previously, orizi wrote…

it is fine it is defined here - just no reason to bound a plugin that only adds additional errors to be based only on a single attribute - this is just a suggestion for a rename.

Done. We can name it as external attributes, but in the future, this plugin will be probably be changed to doc.

[piotmag769]

Reviewable status: 2 of 7 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @maciektr, @orizi, and @wawel37)

---

crates/cairo-lang-plugins/src/plugins/external_attributes_validation.rs line 12 at r5 (raw file):

const DOC_ATTR: &str = "doc";
const SUPPORTED_ARGS_ERROR_MESSAGE: &str = "hidden";

Not blocking, but please change it

Suggestion:

const HIDDEN_ARG: &str = "hidden";

[orizi]

Reviewed 1 of 2 files at r3, 1 of 3 files at r4, 3 of 5 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @maciektr, and @wawel37)