-
Notifications
You must be signed in to change notification settings - Fork 89
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 empty variable declaration support #432
base: master
Are you sure you want to change the base?
Conversation
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.
Why do we use Arc here?
let typee = parse_type(meta)?; | ||
self.handle_add_variable(meta, &self.name.clone(), typee, tok, true)?; | ||
self.is_declare = true; | ||
return Ok(()); |
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.
Why cant we allow for this syntax?
let x: Text = fun_returning_any()
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.
we can, its just not relevant in this PR. you should create another issue for that
|
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 logic doesn't track different execution branches. Here's an examle:
fun test(arg: Num): Null {
let var: Text
if {
arg < 20: var = "Hello, World!"
arg > 30: var = "Hi, hello!"
}
echo var
}
test(10) // outputs "Hello, World!"
test(25) // outputs empty message, shouldn't be possible
test(40) // outputs "Hi, hello!"
Variable is being assigned only in two branches, but is always used. This means it also gets used when uninitialized.
so we can't actually track if its assigned in a way that would matter. i think we should remove that check for now and implement it later when we start actually analyzing code for errors. @Ph0enixKM @Mte90 what do you think |
It make sense what @b1ek, probably what we can do is adding a comment right in the code with |
I say we shouldn't implement this language feature until we can make it work as intended. There are enough workarounds for it, so this shouldn't be a problem. |
This comment was marked as off-topic.
This comment was marked as off-topic.
*offer a broken feature
This isn't about output optimization, this is about correctness and soundness of the language. Optimization is when something already works, but is made to work better. This doesn't work. |
Sorry I confused the PR with another one about constants... About my proposal is to do what is written here #432 (comment) |
I get that, but we don't use Mutex anywhere else in this project as we don't utilize multi threading. Why do we need it now? |
I agree with @mks-h. The features implemented should work as expected from the ground up. Otherwise people will start to use it in an unintended way. Thus when we introduce a fix for this issue, then it will create a breaking change. |
im converting this to draft before we implement a proper solution for code scanning |
okay so i've implemented empty variable declarations for this syntax:
it translates like this:
let variable: Text
->declare variable
and if the variable was declared but never assigned, it will throw a compile time error
closes #420