implement cfg not

[dean-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[orizi]

Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

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

impl PredicateTree {
    fn evaluate(&self, cfg_set: &CfgSet) -> bool {

doc

---

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

        })
        .collect();
    if predicates.len() == 1 { Some(predicates.into_iter().next().unwrap()) } else { None }

why only one? can't we have multiple constraints?

Code quote:

    if predicates.len() == 1 { Some(predicates.into_iter().next().unwrap()) } else { None }

---

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

                _ => {
                    diagnostics.push(PluginDiagnostic::error(
                        value.as_syntax_node().stable_ptr(),

any reason for this change?

Suggestion:

            diagnostics.push(PluginDiagnostic::error(
                &arg.arg,
                "This attribute does not support field initialization shorthands.".into(),
            ));
            None
        }
        AttributeArgVariant::Named { name, value } => {
            let value_text = match value {
                ast::Expr::ShortString(terminal) => terminal.string_value(db).unwrap_or_default(),
                ast::Expr::String(terminal) => terminal.string_value(db).unwrap_or_default(),
                _ => {
                    diagnostics.push(PluginDiagnostic::error(
                        &value,

---

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

                            PredicateTree::Not(Box::new(PredicateTree::Cfg(Cfg::name(
                                "group".to_string(),
                            ))))

what does this mean?

Code quote:

                            PredicateTree::Not(Box::new(PredicateTree::Cfg(Cfg::name(
                                "group".to_string(),
                            ))))

---

crates/cairo-lang-plugins/src/test.rs line 40 at r1 (raw file):

        panicable: "panicable",
        external_attributes_validation: "external_attributes_validation",
        cfg_not: "cfg_not"

just add it in "config"

Code quote:

        cfg_not: "cfg_not"

[orizi]

Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

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

                    arg.arg.as_syntax_node().stable_ptr(),
                    "Failed to parse argument.".into(),
                ));

doesn't this duplicate diagnostics?

Code quote:

                diagnostics.push(PluginDiagnostic::error(
                    arg.arg.as_syntax_node().stable_ptr(),
                    "Failed to parse argument.".into(),
                ));

---

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

                    if function_name == "not" {
                        let args = call.arguments(db).arguments(db).elements(db);

just ensure here there's a single arg here - as you expect for not.
and then probably recursively call.

if you need the same behavior for AttributeArg and Arg you probably need to add a trait here.

Code quote:

                        let args = call.arguments(db).arguments(db).elements(db);

[dean-starkware]

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-plugins/src/test.rs line 40 at r1 (raw file):

Previously, orizi wrote…

just add it in "config"

Done.

---

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

Previously, orizi wrote…

doc

Done.

---

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

Previously, orizi wrote…

why only one? can't we have multiple constraints?

The current implementation only supports a single predicate for simplicity.
If we choose to, we can also support and and or in PredicateTree to allow combining multiple predicates and extends the system's flexibility.

---

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

Previously, orizi wrote…

any reason for this change?

I couldn't find an explanation now when I tried to justify it, so reverted back to how it was before.

---

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

Previously, orizi wrote…

what does this mean?

This logic is ensuring that the Not operator always operates on a single predicate tree.
The fallback dummy predicate (group) is there to handle unexpected scenarios where Not would otherwise have nothing valid to negate.
There are edge cases (like malformed input) where multiple predicates might sneak in, this fallback logic ensures the function doesn't crash.

For example:
#[cfg(not(a, key = "value"))] // Incorrect: `not` should handle one logical group.

This could have been removed if we allowed only a single arg- but because we enable the not to support also the Name case- we need to handle this.

---

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

Previously, orizi wrote…

doesn't this duplicate diagnostics?

Good point, changed it.

---

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

Previously, orizi wrote…

just ensure here there's a single arg here - as you expect for not.
and then probably recursively call.

if you need the same behavior for AttributeArg and Arg you probably need to add a trait here.

But it's not the case- we infact can have more than a single arg here- if its a named one- thus this check is not valid.
I do undestand what you mean here- but it's not correct.

[orizi]

Reviewed 1 of 4 files at r3.
Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

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

Previously, dean-starkware wrote…

The current implementation only supports a single predicate for simplicity.
If we choose to, we can also support and and or in PredicateTree to allow combining multiple predicates and extends the system's flexibility.

This is not correct, previously you could have multiple constraints, you broke the current support.

---

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

Previously, dean-starkware wrote…

This logic is ensuring that the Not operator always operates on a single predicate tree.
The fallback dummy predicate (group) is there to handle unexpected scenarios where Not would otherwise have nothing valid to negate.
There are edge cases (like malformed input) where multiple predicates might sneak in, this fallback logic ensures the function doesn't crash.

For example:
#[cfg(not(a, key = "value"))] // Incorrect: `not` should handle one logical group.

This could have been removed if we allowed only a single arg- but because we enable the not to support also the Name case- we need to handle this.

Return None, and include diagnotics, this is why you have that option.

[orizi]

Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-plugins/src/test_data/config line 7 at r3 (raw file):

//! > cfg
["a", "b", ["k", "a"], ["k", "b"], ["key", "value"], ["not_key", "not_value"]]

In a different test within the file.

[orizi]

Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

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

Previously, dean-starkware wrote…

But it's not the case- we infact can have more than a single arg here- if its a named one- thus this check is not valid.
I do undestand what you mean here- but it's not correct.

I understand both have names and unnamed options, but they remain the same, so I don't understand your point.

---

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

                            diagnostics.push(PluginDiagnostic::error(
                                call.as_syntax_node().stable_ptr(),
                                "`not` operator requires at least one argument.".into(),

Exactly one.

[dean-starkware]

Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

Previously, orizi wrote…

This is not correct, previously you could have multiple constraints, you broke the current support.

Done.

---

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

Previously, orizi wrote…

Return None, and include diagnotics, this is why you have that option.

Done.

---

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

Previously, orizi wrote…

I understand both have names and unnamed options, but they remain the same, so I don't understand your point.

Done.

---

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

Previously, orizi wrote…

Exactly one.

Done.

---

crates/cairo-lang-plugins/src/test_data/config line 7 at r3 (raw file):

Previously, orizi wrote…

In a different test within the file.

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 1 of 4 files reviewed, 10 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-plugins/src/test_data/config line 302 at r4 (raw file):

}

// Invalid usage examples for diagnostics

separate the diagnostics to another test.

Code quote:

// Invalid usage examples for diagnostics

---

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

            _ => None,
        }
    }

add the enum:

enum ConfigPredicatePart {
     Cfg(Cfg),
     Call(ast::FunctionCall),
}

same should work above.

Suggestion:

    
    
    fn get_part(&self, db: &dyn SyntaxGroup) -> Option<ConfigPredicatePart> {
        match self.arg_clause(db) {
            ast::ArgClause::Named(named) => match named.value(db) {
                ast::Expr::String(terminal) => Some(ConfigPredicatePart::Cfg(Cfg::kv(named.name(db).text(db).to_string(), terminal.string_value(db))),
                _ => None,
            },
            ast::ArgClause::Unnamed(unnamed) => match unnamed.value(db) {
                ast::Expr::Path(path) => {
                    let segments = path.elements(db);
                    if let [ast::PathSegment::Simple(segment)] = &segments[..] {
                        Some(ConfigPredicatePart::Cfg(Cfg::name(segment.ident(db).text(db).to_string())))
                    } else {
                        None
                    }
                }
                ast::Expr::FunctionCall(call) => ConfigPredicatePart::FunctionCall(call),
                _ => None,
            },
            _ => None,
        }
    }

---

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

    fn stable_ptr(&self) -> cairo_lang_syntax::node::ids::SyntaxStablePtrId {
        cairo_lang_syntax::node::TypedSyntaxNode::stable_ptr(self).into()

Suggestion:

        self.stable_ptr().untyped()

---

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

        }
        vec![]
    }

and delete these.

external match should be enough now.

Code quote:

    fn is_function_call(&self, db: &dyn SyntaxGroup) -> bool {
        if let ast::ArgClause::Unnamed(unnamed) = self.arg_clause(db) {
            matches!(unnamed.value(db), ast::Expr::FunctionCall(_))
        } else {
            false
        }
    }

    fn get_function_call_args(&self, db: &dyn SyntaxGroup) -> Vec<Box<dyn ConfigArg>> {
        if let ast::ArgClause::Unnamed(unnamed) = self.arg_clause(db) {
            if let ast::Expr::FunctionCall(call) = unnamed.value(db) {
                return call
                    .arguments(db)
                    .arguments(db)
                    .elements(db)
                    .into_iter()
                    .map(|arg| Box::new(AttributeArg::from_ast(arg, db)) as Box<dyn ConfigArg>)
                    .collect();
            }
        }
        vec![]
    }

---

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

        attr.args
            .into_iter()
            .filter_map(|arg| parse_predicate_item(db, &arg, diagnostics))

Suggestion:

            .filter_map(|arg| parse_predicate_item(db, arg, diagnostics))

---

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

fn parse_predicate_item(
    db: &dyn SyntaxGroup,
    item: &dyn ConfigArg,

Suggestion:

    item: impl ConfigArg,

---

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

            return None;
        }
    }

Suggestion:

    match item.get_part(db) {
        None => {
            // add relevanat diagnostics.
            return None;
        }
        Some(ConfigPredicatePart::Cfg(cfg)) => return Some(cfg),
        Some(ConfigPredicatePart::FunctionCall(call)) => {
            // Actually extract the function call arguments and test everything.
            // No need to be in the trait implementation.
        },
    }

---

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

            .get_function_call_args(db)
            .into_iter()
            .filter_map(|arg| parse_predicate_item(db, arg.as_ref(), diagnostics))

Suggestion:

            .filter_map(|arg| parse_predicate_item(db, arg, diagnostics))

---

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

                    Some(PredicateTree::Or(args))
                }
            }

Suggestion:

            }
            "and" => {
                if args.len() >= 2 {
                    diagnostics.push(PluginDiagnostic::error(
                        item.stable_ptr(),
                        "`and` operator expects at least two arguments.".into(),
                    ));
                    None
                } else {
                    Some(PredicateTree::And(args))
                }
            }
            "or" => {
                if args.len() >= 2 {
                    diagnostics.push(PluginDiagnostic::error(
                        item.stable_ptr(),
                        "`or` operator expects at least two arguments.".into(),
                    ));
                    None
                } else {
                    Some(PredicateTree::Or(args))
                }
            }

[dean-starkware]

Reviewable status: 1 of 4 files reviewed, 10 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

Previously, orizi wrote…

add the enum:

enum ConfigPredicatePart {
     Cfg(Cfg),
     Call(ast::FunctionCall),
}

same should work above.

I think you mean ast::ExprFunctionCall.

---

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

    fn stable_ptr(&self) -> cairo_lang_syntax::node::ids::SyntaxStablePtrId {
        cairo_lang_syntax::node::TypedSyntaxNode::stable_ptr(self).into()

I don't get what you meant to do here.
The problem is that the method stable_ptr is being called recursively.
I'm sure you didn't mean that- but I can't seem to understand what were your intentions here.

---

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

fn parse_predicate_item(
    db: &dyn SyntaxGroup,
    item: &dyn ConfigArg,

Why?
I'm sure this is the right thing to do, but don't fully understand it.

[orizi]

Reviewable status: 1 of 4 files reviewed, 10 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

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

Previously, dean-starkware wrote…

I think you mean ast::ExprFunctionCall.

yes probably.

---

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

Previously, dean-starkware wrote…

I don't get what you meant to do here.
The problem is that the method stable_ptr is being called recursively.
I'm sure you didn't mean that- but I can't seem to understand what were your intentions here.

I see no way it is recursive - you are doing something wrong.

in any case -you can make your trait have the super trait TypedSyntaxNode - then you won't need this at all.

---

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

Previously, dean-starkware wrote…

Why?
I'm sure this is the right thing to do, but don't fully understand it.

because you don't need dyn at all - it would just create two instances of this function, with no dynamic function calls.

[dean-starkware]

Reviewable status: 1 of 4 files reviewed, 10 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

Previously, orizi wrote…

yes probably.

Done.

---

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

Previously, orizi wrote…

I see no way it is recursive - you are doing something wrong.

in any case -you can make your trait have the super trait TypedSyntaxNode - then you won't need this at all.

Done.

---

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

Previously, orizi wrote…

and delete these.

external match should be enough now.

Done.

---

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

Previously, orizi wrote…

because you don't need dyn at all - it would just create two instances of this function, with no dynamic function calls.

Done.

---

crates/cairo-lang-plugins/src/test_data/config line 302 at r4 (raw file):

Previously, orizi wrote…

separate the diagnostics to another test.

Done.

---

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

        attr.args
            .into_iter()
            .filter_map(|arg| parse_predicate_item(db, &arg, diagnostics))

Done.

---

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

            return None;
        }
    }

Done.

---

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

            .get_function_call_args(db)
            .into_iter()
            .filter_map(|arg| parse_predicate_item(db, arg.as_ref(), diagnostics))

Done.

---

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

                    Some(PredicateTree::Or(args))
                }
            }

Done.

[orizi]

Reviewed 1 of 4 files at r3, all commit messages.
Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

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

fn parse_predicate_item(
    db: &dyn SyntaxGroup,
    item: impl ConfigArg,

i guess you can just pass a AttributeArg here if this is now the only implementer of the trait.
(and then probably remove the stable_ptr fetching function from the trait)

Suggestion:

    item: AttributeArg,

[dean-starkware]

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

---

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

Previously, orizi wrote…

i guess you can just pass a AttributeArg here if this is now the only implementer of the trait.
(and then probably remove the stable_ptr fetching function from the trait)

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-plugins/src/test_data/config line 1 at r6 (raw file):

//! > Test ignoring of test config in the test config.

if you found the issue add a testcase for it as well.
(not same names as the actual bug, just same cause)

[dean-starkware]

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

---

crates/cairo-lang-plugins/src/test_data/config line 1 at r6 (raw file):

Previously, orizi wrote…

if you found the issue add a testcase for it as well.
(not same names as the actual bug, just same cause)

The issue was that "'test'" became 'test', instead of test.
this fixed it:
ast::Expr::ShortString(terminal) => {
terminal.string_value(db).unwrap_or_default()
}

as documented here:

cairo/crates/cairo-lang-language-server/src/lang/db/mod.rs

Line 70 in 848b90f

CfgSet::from_iter([Cfg::name("test"), Cfg::kv("target", "test")])

I'm not sure a testcase is relevant for this bug. Correct me if I'm wrong.

[orizi]

Reviewed all commit messages.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-plugins/src/test_data/config line 297 at r7 (raw file):

}

#[cfg(or(a, x))]

add an additional more complex cfg test (or of ands of nots and so on)

[dean-starkware]

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

---

crates/cairo-lang-plugins/src/test_data/config line 297 at r7 (raw file):

Previously, orizi wrote…

add an additional more complex cfg test (or of ands of nots and so on)

Done.

[orizi]

Reviewed 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)