Add array predicate expressions#11989
Conversation
b937cff to
db7743e
Compare
1ded7e9 to
acc6892
Compare
1807503 to
db9fe67
Compare
| => x > 0 | ||
| ) && !self.ints.all(x => x > 1 | ||
| ) && !self.ints.any(x => x == 0 | ||
| ) && self.ints.any(x => x == 5 | ||
| ) && self.ints.any(x => x < 5 | ||
| ); |
There was a problem hiding this comment.
The formatting throughout this file is wrong.
We will have to adjust the formatter to handle the new Predicates syntax (assuming this is what caused the issue).
There was a problem hiding this comment.
Done:
- Added formatter support for closure expressions.
- Re-ran
slint-lsp formaton the test case, sox => ...now formats cleanly.
|
|
||
| let shadowing = x == "hello world" && s == 42 && p == 42px; | ||
|
|
||
| ints && strings && people && groups && nested_named && precedence && empty && internal_name_shadowing && shadowing |
There was a problem hiding this comment.
I'm not a fan of this style of test (I know we have a couple of these already).
Folding dozens of test cases into a single boolean, makes it very difficult to determine afterwards what caused a failure (especially in CI).
I would much prefer individual out properties per test and have them asserted one after the other in the cpp/rust tests.
(Side note: @ogoffart We should consider adding an assert statement to Slint itself, or to expose the assert as a global callback in the tests somehow - not in this PR though).
| age: int, | ||
| } | ||
|
|
||
| export component TestCase { |
There was a problem hiding this comment.
We are missing test-cases for predicates that capture the surrounding scope.
Please add a separate test for predicates that capture other properties to ensure the predicates are re-evaluated when the properties change.
Because at the moment, the predicates cannot be copied, we don't yet have to resolve the lifetime issues that are brought up here: #1328 (comment) .
But we should still test that the property bindings remain reactive.
There was a problem hiding this comment.
@tilladam and if you have any input on how to handle the lifetime issues that olivier outlined, that would be good to know as well...
| /// `implements Interface.Foo` | ||
| ImplementsSpecifier -> [ QualifiedName ], | ||
| /// `x => x > 0` | ||
| Predicate -> [DeclaredIdentifier, Expression], |
There was a problem hiding this comment.
at the moment these are indeed predicates because they only take a singular value and may only return a bool as a result.
However, from the previous discussions it seems almost certain that at some point these will become full closures/lambdas (e.g for model filtering/mapping). So I think it makes sense to already start calling them that now, even if they only support Boolean values for the moment.
As we likely want to capture the surrounding environment, I'm in favor of calling them "Closures".
| PredicateArgumentsLookup, | ||
| ( | ||
| ArgumentsLookup, | ||
| LocalVariableLookup, |
There was a problem hiding this comment.
Regarding shadowing: My current understanding from this code is that any local variable will shadow the predicate arguments, right?
Which might lead to this incorrect behavior:
let x = 5;
[1, 2, 3].any(x => x == 2); // Problem: returns false, as x refers to the let x = 5; aboveMaybe I'm missing something here, but the stacks for local variables and predicate variables might have to be one stack, instead of two.
In any case, we should have a test for this.
| } | ||
|
|
||
| fn from_predicate_node(node: syntax_nodes::Predicate, ctx: &mut LookupCtx) -> Expression { | ||
| if !ctx.predicates_allowed { |
There was a problem hiding this comment.
This predicates_allowed flag is quite the hack that I think won't scale.
It also effectively makes the .any() and .all() functions macros that enable additional syntax.
In my opinion, they should just be functions that take a predicate type.
That does mean that predicates would be allowed anywhere a normal expression is allowed.
Which is not really a problem as there is no way to evaluate them.
They are just instances of a non-nameable type without any functions on them.
But it would allow us to test that this syntax doesn't produce problems if used in other contexts.
We can (and should) then also add tests where we use them in different contexts:
let y = x => 5; // doesn't do anything, but should be valid. (creates a "useless statement warning").If we special case this now, it may be difficult to remove this special casing again.
| /// y => y == 42 | ||
| /// z => true | ||
| /// ``` | ||
| fn parse_predicate(p: &mut impl Parser) { |
There was a problem hiding this comment.
Note: The tree-sitter grammar will need updating.
| let arr = #arr_expression; | ||
| arr.model_tracker().track_row_count_changes(); | ||
| (0..arr.row_count()).any(|index| -> bool { | ||
| let row_data = arr.row_data_tracked(index).unwrap_or_default(); | ||
| (|#arg_name: #arg_ty| { #predicate_expression })(row_data) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
We try to keep as much code as possible out of the generated code and in core to improve compile times.
So please make the any and all functions functions in i-slint-core and use them here (like you do for C++).
db9fe67 to
af3aa24
Compare
af3aa24 to
0ed8ef1
Compare
- Rename Type::Predicate -> Type::Closure in the Node.js value binding (was a missed spot from the langtype rename; broke node_test_* CI). - Replace the closure_arg_type ambient flag on LookupCtx with an explicit argument routed through a from_argument_expression_node helper. Closures now resolve in any expression position; only call sites that structurally constrain the arg type (.any / .all) demand a bool body, so misuses surface as a single conversion error at the consumer instead of cascading errors inside the body. - Drop a redundant base.clone() in the member-function call path. - Strip the internal local_ prefix when pretty-printing closure expressions in both the expression tree and LLR. - Reword the closure body type error to "Closure body must be of type bool" for clarity. - Add a same-name same-type shadowing test (closure-arg-shadows-same-type-local) plus split the JS asserts in array_predicates.slint per-property to match the C++ / Rust harness granularity. - Extend the syntax test with closure-in-no-context positions to lock in that they parse cleanly and only the consuming site reports the type error.
0ed8ef1 to
c75f944
Compare
This continues and rebases the array predicate work originally started by Avery Townsend (@codeshaunted) in #8941.
The branch adds
array.any(name => condition)andarray.all(name => condition)support for Slint arrays/models, including compiler lowering, Rust/C++ code generation, interpreter evaluation, formatter support, docs, and tests.I checked the discussion and inline review comments on #8941 before opening this PR. The branch addresses the requested follow-ups:
x => ...).expandedspelling inparser.rs.Tests run:
SLINT_TEST_FILTER=array_predicates cargo test -p test-driver-rustSLINT_TEST_FILTER=array_predicates cargo test -p test-driver-interpretercargo test -p i-slint-compiler --features display-diagnostics --test syntax_testscargo clippy -p i-slint-compiler -p slint-interpreter --tests