Prevented starknet plugin from code generation on items not on config.

[orizi]

Fixes #4897

---

This change is 

[mkaput]

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @orizi, and @yuvalsw)

a discussion (no related file):
I don't want to block the work on this, but I am super afraid of this change. It's just a matter of time when similar situation will pop in a different plugin or something. Shouldn't there be a way to do some kind of plugin recursion?

---

[orizi]

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @mkaput, and @yuvalsw)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

I don't want to block the work on this, but I am super afraid of this change. It's just a matter of time when similar situation will pop in a different plugin or something. Shouldn't there be a way to do some kind of plugin recursion?

There is already recursion, but we originally run on text.

---

[yuvalsw]

Reviewed 5 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware, @mkaput, and @orizi)

---

crates/cairo-lang-starknet/src/plugin/starknet_module/component.rs line 54 at r2 (raw file):

pub(super) fn generate_component_specific_code(
    db: &dyn SyntaxGroup,
    metadata: &MacroPluginMetadata<'_>,

move after diagnostics

---

crates/cairo-lang-starknet/src/plugin/starknet_module/contract.rs line 302 at r2 (raw file):

pub(super) fn generate_contract_specific_code(
    db: &dyn SyntaxGroup,
    metadata: &MacroPluginMetadata<'_>,

after diagnostics

---

crates/cairo-lang-starknet/src/plugin/starknet_module/mod.rs line 179 at r2 (raw file):

    let mut extra_uses = OrderedHashMap::default();
    for item in body.items(db).elements(db) {
        if should_drop(db, metadata.cfg_set, &item, &mut vec![]) {

Let's add an non-dropped-items iteration function, and add a note comment on the original .items() that it should not be used for plugins directly.
This will also mostly resolve Marek's concern.

[orizi]

Reviewable status: 3 of 13 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @yuvalsw)

---

crates/cairo-lang-starknet/src/plugin/starknet_module/mod.rs line 179 at r2 (raw file):

Previously, yuvalsw wrote…

Let's add an non-dropped-items iteration function, and add a note comment on the original .items() that it should not be used for plugins directly.
This will also mostly resolve Marek's concern.

Done.

[yuvalsw]

Reviewed 7 of 10 files at r3, 3 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

}

/// Iterator over items in a scope that would remain in config.

Suggestion:

/// Iterator over the items that are included in the given config set, among the given items.

---

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

/// Iterator over items in a scope that would remain in config.
pub struct InCfg<'a, ScopeItem: QueryAttrs, ScopeItems: Iterator<Item = ScopeItem>> {

The word "scope" everywhere is not necessary if you get the list (iterator) of items as a param.

---

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

/// Iterator over items in a scope that would remain in config.
pub struct InCfg<'a, ScopeItem: QueryAttrs, ScopeItems: Iterator<Item = ScopeItem>> {

Better name please?

Code quote:

InCfg

---

crates/cairo-lang-starknet/src/plugin/embeddable.rs line 121 at r4 (raw file):

    };
    let mut data = EntryPointsGenerationData::default();
    for item in InCfg::new(db, metadata.cfg_set, body.items(db).elements(db)) {

Would be nice to have it like this

Suggestion:

  for item in InCfg::new(db, metadata.cfg_set, body) {

---

crates/cairo-lang-starknet/src/plugin/starknet_module/mod.rs line 179 at r2 (raw file):

Previously, orizi wrote…

Done.

Can you also do the note-comment part from above?

[orizi]

Reviewable status: 3 of 13 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @yuvalsw)

---

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

Previously, yuvalsw wrote…

The word "scope" everywhere is not necessary if you get the list (iterator) of items as a param.

Done.

---

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

Previously, yuvalsw wrote…

Better name please?

Done.

---

crates/cairo-lang-starknet/src/plugin/embeddable.rs line 121 at r4 (raw file):

Previously, yuvalsw wrote…

Would be nice to have it like this

Done.

---

crates/cairo-lang-starknet/src/plugin/starknet_module/mod.rs line 179 at r2 (raw file):

Previously, yuvalsw wrote…

Can you also do the note-comment part from above?

?

[yuvalsw]

Reviewed 10 of 10 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-plugins/src/plugins/config.rs line 59 at r6 (raw file):

}

/// Iterator over the items that are included in the given config set, among the given items.

Suggestion:

ven config set, among the given items in `iterator`.

---

crates/cairo-lang-plugins/src/plugins/config.rs line 60 at r6 (raw file):

/// Iterator over the items that are included in the given config set, among the given items.
pub struct ItemsInCfg<'a, Item: QueryAttrs> {

does it still need to be pub?

Code quote:

pub 

---

crates/cairo-lang-plugins/src/plugins/config.rs line 69 at r6 (raw file):

        Self { db, cfg_set, iterator: items.into_iter() }
    }
}

if my default implementation suggestion (below) applies, then this is used only once and can be removed.
Even if not, I don't see its value as it simply assigns all the values (and the ctor can be more readable with named args).

Code quote:

impl<'a, Item: QueryAttrs> ItemsInCfg<'a, Item> {
    fn new(db: &'a dyn SyntaxGroup, cfg_set: &'a CfgSet, items: Vec<Item>) -> Self {
        Self { db, cfg_set, iterator: items.into_iter() }
    }
}

---

crates/cairo-lang-plugins/src/plugins/config.rs line 99 at r6 (raw file):

        cfg_set: &'a CfgSet,
    ) -> ItemsInCfg<'a, Self::Item> {
        ItemsInCfg::new(db, cfg_set, self.items(db).elements(db))

I think this function impl can be moved to the trait (as it's the same for all), and then the impls are very short (only the type Item required).

Code quote:

ItemsInCfg::new(db, cfg_set, self.items(db).elements(db))

---

crates/cairo-lang-starknet/src/plugin/embeddable.rs line 121 at r4 (raw file):

Previously, orizi wrote…

Done.

Much better!

---

crates/cairo-lang-starknet/src/plugin/starknet_module/mod.rs line 179 at r2 (raw file):

Previously, orizi wrote…

?

"and add a note comment on the original .items() that it should not be used for plugins directly."
So that anyone who may want to do it and want to use .items() directly will (probably) see the comment which will point them to the good way to do it.

[orizi]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @yuvalsw)

---

crates/cairo-lang-plugins/src/plugins/config.rs line 60 at r6 (raw file):

Previously, yuvalsw wrote…

does it still need to be pub?

yes - it is returned by the trait function.

---

crates/cairo-lang-starknet/src/plugin/starknet_module/mod.rs line 179 at r2 (raw file):

Previously, yuvalsw wrote…

"and add a note comment on the original .items() that it should not be used for plugins directly."
So that anyone who may want to do it and want to use .items() directly will (probably) see the comment which will point them to the good way to do it.

it is on the generated ast code...

also - in general we need it just for elements where we go up and then back down.
when processing others - we always use config plugin first - and only then other plugins - so we won't process non-config elements.

[orizi]

Reviewable status: 6 of 15 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @yuvalsw)

---

crates/cairo-lang-plugins/src/plugins/config.rs line 69 at r6 (raw file):

Previously, yuvalsw wrote…

if my default implementation suggestion (below) applies, then this is used only once and can be removed.
Even if not, I don't see its value as it simply assigns all the values (and the ctor can be more readable with named args).

Done.

---

crates/cairo-lang-plugins/src/plugins/config.rs line 99 at r6 (raw file):

Previously, yuvalsw wrote…

I think this function impl can be moved to the trait (as it's the same for all), and then the impls are very short (only the type Item required).

Done.

[gilbens-starkware]

Reviewed 3 of 6 files at r1, 3 of 10 files at r6, 9 of 9 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yuvalsw)

[gilbens-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yuvalsw)

[yuvalsw]

As discussed f2f, please re-check all places - we don't need the filtered iter when we are traversing the tree only through impls, traits and the current (sub)module. If we potentially go through another submodule, we might have the problem and need the filtered iter.

Reviewed 7 of 9 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-plugins/src/plugins/config.rs line 69 at r6 (raw file):

Previously, orizi wrote…

Done.

I still see it. I meant ::new() can be removed. It is used once.

---

crates/cairo-lang-plugins/src/plugins/config.rs line 82 at r7 (raw file):

/// Trait for AST nodes that contain items, providing an iterator over the items that are included
/// in the cfg.
pub trait HasItemsInCfgEx {

why ex?

---

crates/cairo-lang-syntax/src/node/helpers.rs line 564 at r7 (raw file):

}

/// Trait for getting the items of a node that contains items as a vector.

suggestion

Suggestion:

/// Trait for getting the items of a body-item (an item that contains items), as a vector.

---

crates/cairo-lang-syntax/src/node/helpers.rs line 571 at r7 (raw file):

    /// Use with caution, as this will include items that may be filtered out by plugins.
    /// Do note that plugins that directly ran on this node without going up on the syntax tree may
    /// assume that out of config items were already filtered out.

Suggestion:

    /// Returns the items of the body-item as a vector.
    /// Use with caution, as this includes items that may be filtered out by plugins.
    /// Do note that plugins that directly run on this body-item without going/checking up on the syntax tree may
    /// assume that e.g. out-of-config items were already filtered out.

[yuvalsw]

Also, please doc it where you don't use the filtered iter. Until we fix it in a better way (e.g. allow generation of code inside an item and not only beside it), we need to make sure that new plugins (that will copy code from existing ones) will note this issue and handle it correctly. Use a one sentence summary of our conversation to explain the rule and its rational.

[orizi]

Reviewable status: 13 of 15 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @yuvalsw)

---

crates/cairo-lang-plugins/src/plugins/config.rs line 69 at r6 (raw file):

Previously, yuvalsw wrote…

I still see it. I meant ::new() can be removed. It is used once.

Done.

---

crates/cairo-lang-plugins/src/plugins/config.rs line 82 at r7 (raw file):

Previously, yuvalsw wrote…

why ex?

It is a general marker for externsion trait.

[orizi]

that seems to me like comments without a clear effect.
i think the doc on the actual function should suffice - if not we should call the function an extremely_long_documenting_name.

Reviewable status: 13 of 15 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @yuvalsw)

[yuvalsw]

I meant something like:
in items_vec(): doc when it's ok to use and when it's not. E.g.: don't use on an item that is not the original plugin's context, unless you are sure that while traversing the AST to get to it from the original plugin's context, you did not go through another submodule.

This explains when to use it. But then also explain why it's ok to use it when you do - the same as when you use .unwrap() when you're sure it won't panic.
Note that even with this, one might use .items().elements() and miss this potential bug.

It took a conversation of a few minutes for us to conclude the exact cases, a future plugin writer is probably not going to get into the details but copy from another plugin. I think it's worth helping to prevent this bug.

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)

---

crates/cairo-lang-plugins/src/plugins/config.rs line 82 at r7 (raw file):

Previously, orizi wrote…

It is a general marker for externsion trait.

I don't think AaaEx it is needed when there is no Aaa, but ok

[orizi]

true - but having this in every usage makes no sense - having this in the doc - does.
If we wanna make sure we have this doc at every usage, we need to have a bad name instead.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)

[yuvalsw]

There are only 4 uses: dispatcher, embeddable, generate_trait (go through items of impl/trait) and in starknet_module/mod.rs (which looks specifically for a struct).
I meant adding something like:
// OK to use .items_vec() as x is a trait. See items_vec for more details.

Also adding the comment on items_vec itself is important. See my suggestion above.

Reviewed 2 of 9 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)

[yuvalsw]

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

---

crates/cairo-lang-syntax/src/node/helpers.rs line 572 at r8 (raw file):

    /// Do note that plugins that directly run on this body-item without going/checking up on the
    /// syntax tree may assume that e.g. out-of-config items were already filtered out.
    fn items_vec(&self, db: &dyn SyntaxGroup) -> Vec<Self::Item>;

adding a blocking comment here as my other comment is nonblocking.

[orizi]

as we discussed - for general usage - it is already correct - we are just adding a comment - that you say should be copied everywhere.
if it should be copied everywhere - it should be part of the name. if it shouldn't (which is what I think) the name should remain as is, and the doc on the function should be enough.

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

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)

---

crates/cairo-lang-syntax/src/node/helpers.rs line 572 at r8 (raw file):

Previously, yuvalsw wrote…

adding a blocking comment here as my other comment is nonblocking.

answered.

[yuvalsw]

I didn't say it should be copied everywhere, but that the usages need to explain why it's ok to use, in each case. Again, the same as you would doc where you use .unwrap() and you know it won't panic.

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

[orizi]

i see no differentiation between places where you would add this - and places where you won't - and the reasoning in all places would be the same, unlike the unwrap case.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)

---

crates/cairo-lang-syntax/src/node/helpers.rs line 572 at r8 (raw file):

Previously, orizi wrote…

answered.

.

[yuvalsw]

The reasoning is not the same. One is a trait, one is an impl, in another one you look specifically for a struct. In the future there may be one where it's a submodule, but for some reason you know it's ok.
Anyway, as discussed f2f, unblocking as I don't see this discussion converging.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)

[yuvalsw]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)