Skip to content
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

Draft
wants to merge 10 commits into
base: staging
Choose a base branch
from

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented Aug 31, 2024

okay so i've implemented empty variable declarations for this syntax:

let variable: Text
let variable // wont work: generic not allowed

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

@b1ek b1ek marked this pull request as ready for review August 31, 2024 13:47
Copy link
Member

@KrosFire KrosFire left a 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(());
Copy link
Member

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()

Copy link
Member Author

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

@b1ek
Copy link
Member Author

b1ek commented Sep 7, 2024

Why do we use Arc here?

Mutex<T> can't be cloned, so Arc must be used so it can be cloned but pointing to the same location as the first Arc

@mks-h mks-h self-requested a review September 8, 2024 18:43
Copy link
Member

@mks-h mks-h left a 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.

@b1ek
Copy link
Member Author

b1ek commented Sep 9, 2024

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

@Mte90
Copy link
Member

Mte90 commented Sep 9, 2024

It make sense what @b1ek, probably what we can do is adding a comment right in the code with \\TODO so we can found it when we want to do that.
I think that we already have a ticket for errors alerting?

@mks-h
Copy link
Member

mks-h commented Sep 9, 2024

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.

@Mte90

This comment was marked as off-topic.

@mks-h
Copy link
Member

mks-h commented Sep 9, 2024

we can offer the feature right now

*offer a broken feature

it is normal that we optimize it's output.

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.

@Mte90
Copy link
Member

Mte90 commented Sep 9, 2024

Sorry I confused the PR with another one about constants...

About my proposal is to do what is written here #432 (comment)

@KrosFire
Copy link
Member

KrosFire commented Sep 9, 2024

Why do we use Arc here?

Mutex<T> can't be cloned, so Arc must be used so it can be cloned but pointing to the same location as the first Arc

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?

@Ph0enixKM
Copy link
Member

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.

@b1ek
Copy link
Member Author

b1ek commented Sep 14, 2024

im converting this to draft before we implement a proper solution for code scanning

@b1ek b1ek marked this pull request as draft September 14, 2024 23:18
@Ph0enixKM Ph0enixKM changed the base branch from master to staging December 19, 2024 11:15
@Ph0enixKM
Copy link
Member

Ph0enixKM commented Dec 20, 2024

I just reviewed this PR more closely and I fail to recognize the benefit of this change. What's the purpose of declaring a variable without it's variable? Not only that but also what's the benefit of asking the developer to also assign some value to it if not assigned.

I thought that this PR introduces explicit types to the variables like

let variable: Text = ""

But that's not necessary as well since we do not support any complex data type. I'd like to close this PR and just implement a warning mechanism for when variable is not used, as of right now.

@b1ek
Copy link
Member Author

b1ek commented Dec 20, 2024

@Ph0enixKM this is just an implementation, the discussion of the feature itself is in #420

@Ph0enixKM
Copy link
Member

@b1ek Ah... sorry then. I'll post to the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Declaring empty variables
5 participants