-
Notifications
You must be signed in to change notification settings - Fork 697
Add local variables, closes #2752 #8740
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
Add local variables, closes #2752 #8740
Conversation
5ef96a0
to
19c20ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is great!
Just added some comment inline, in particular, i think the "discriminator" should be part of the name in the Expression
Another thing is that the discriminator should probably be global, because otherwise we might have conflicts with inlining.
There should also be tests for error in internal/compiler/tests/syntax. For example that test that the types are respected and that it is error otherwise.
let foo : int = "1"
should be an error. `property xyz : { let foo : string = 1; return foo; } should be an error, and so on.
Shadowing is allowed:
I know i said in the chat it should be allowed. But I'm thinking we shouldn't allow this at first in Slint. Make it an error for now and we can still allow it later if we think this is useful, but it is not used in other programming languages, so i wonder if we should really do that.
Anyway, great patch, thanks for contributing.
@@ -39,6 +39,37 @@ Functions can be annotated with the `pure` keyword. | |||
This indicates that the function does not cause any side effects. | |||
More details can be found in the <Link type="Purity" /> chapter. | |||
|
|||
## Local Variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be documented in the expressions-and-statements section, because it is basically available everywhere, not only in functions.
internal/compiler/expression_tree.rs
Outdated
@@ -611,13 +611,15 @@ pub enum Expression { | |||
/// Should be directly within a CodeBlock expression, and store the value of the expression in a local variable | |||
StoreLocalVariable { | |||
name: SmolStr, | |||
discriminator: Option<usize>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather the discriminator be part of the name.
The name should probably be prefixed as well so that it doesn't conflict with internal variable. Eg, if you have let foo = ...
, the name should be local_foo_1
@@ -50,6 +55,11 @@ pub fn parse_statement(p: &mut impl Parser) -> bool { | |||
return true; | |||
} | |||
|
|||
if p.peek().as_str() == "let" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we have the same problem with return
, but we should check that the next token is also an identifier. So if there is a property called let
, code like let = 3;
or let + 1
still works.
tests/cases/expr/let.slint
Outdated
if (true) { | ||
let a = a + 1; | ||
|
||
return a; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also have a version of the test without the return here? (to make sure that the original value is not changed.
34e4a19
to
84fd545
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again. This looks good to me.
Please just cleanup the patch a bit for the unnecessary changes.
I see that the C++ test is failing with a warning about unused variable. To solve this we could either:
- add [[maybe_unused]] to the variable declaration. (Although this warning is valid, it shows in the generated code instead of the Slint code)
- ignore this particular warning for this particular test somehow.
internal/compiler/generator/rust.rs
Outdated
@@ -2516,7 +2516,7 @@ fn compile_expression(expr: &Expression, ctx: &EvaluationContext) -> TokenStream | |||
} | |||
} | |||
|
|||
Expression::StoreLocalVariable { name, value } => { | |||
Expression::StoreLocalVariable { name, value, .. } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are no more extra discriminant, the patch could be a bit cleaned up for these changes.
internal/compiler/generator/cpp.rs
Outdated
@@ -3132,7 +3132,7 @@ fn compile_expression(expr: &llr::Expression, ctx: &EvaluationContext) -> String | |||
Expression::StoreLocalVariable { name, value } => { | |||
format!("auto {} = {};", ident(name), compile_expression(value, ctx)) | |||
} | |||
Expression::ReadLocalVariable { name, .. } => ident(name).to_string(), | |||
Expression::ReadLocalVariable { name, .. } => format!("{}", ident(name)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be reverted. I think clippy actually warns about use of format when to_string can be used
.push_error(format!("Redeclaration of local variables is not allowed"), &node); | ||
return Expression::Invalid; | ||
} | ||
// conflicts with something else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho we should be careful because this means that adding anything in Skint becomes a breaking change.
Imagine someone writes
let Foo = 42;
But now if I'm adding an enum called Foo in Slint, this code would break.
So I think we should only reject if it overrides a local variables (including the function parameters) but not a global one.
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0 | ||
|
||
export X := Rectangle { | ||
// ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use the new syntax in new tests.
// ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info} | ||
background: { | ||
let blue: brush = 42; | ||
// ^error{Local variable declaration conflicts with existing name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment why I think this should actually be accepted.
436ab75
to
d1993da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Looks good to me.
I'll merge soon.
Adds support for immutable local variables in .slint functions using a new
let
keyword with an optional type annotation:Shadowing is allowed:
Internally an extra discriminator field was added to the
StoreLocalVariable
andReadLocalVariable
expressions, so that shadowing support could be added in the C++ generator, as C++ does not support variable shadowing.