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

feature: add constant declarations #423

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented Aug 25, 2024

i've added support for the const X = Y expression itself. it will resolve to declare -r X=Y, for both local and global variables, since declare -r is the same for locals as well

i also set args in main(args) to be constant. function parameters are never constant.

i don't know which is the minimal bash version for declare -r

this is implemented as `is_const` field in `VariableDecl`, so constants are technically variables that cannot be reassigned.
@b1ek b1ek requested review from Mte90 and Ph0enixKM August 25, 2024 07:09
@b1ek
Copy link
Member Author

b1ek commented Aug 25, 2024

i just checked, declare -r works for bash 4.0:

screenshot that demonstrates how it works in bash 4.0

@b1ek b1ek marked this pull request as ready for review August 26, 2024 02:09
@Mte90 Mte90 linked an issue Aug 30, 2024 that may be closed by this pull request
@Ph0enixKM
Copy link
Member

The reason why I never introduces const is because sh does not support it. So for instance on ash it fails:

image

@Ph0enixKM
Copy link
Member

When we start to support sh - we should compile it to the regular variable instead (but still never let reassign the variable in the script)

@b1ek
Copy link
Member Author

b1ek commented Sep 5, 2024

When we start to support sh - we should compile it to the regular variable instead (but still never let reassign the variable in the script)

i'd rather have it defined as a function that always returns the same literal

@Mte90
Copy link
Member

Mte90 commented Sep 5, 2024

The idea of a function that returns always the same value I think that is the best and assure that can't change.

@mks-h
Copy link
Member

mks-h commented Sep 8, 2024

I'd say go with the @Ph0enixKM idea, since functions have an additional performance penalty. It doesn't matter that a constant is technically a regular variable, so long as it is translated from an Amber constant, thus is proven to be impossible to modify.

@Mte90
Copy link
Member

Mte90 commented Sep 9, 2024

I think that we can move on with a function that return always the same value so we can assure the best compatibility.
In the future we can think for something more sophisticated.

@Ph0enixKM
Copy link
Member

In order to resolve this discussion I've created the following poll: #493

@b1ek
Copy link
Member Author

b1ek commented Sep 30, 2024

@Ph0enixKM im confused, though. afaik we don't support sh right now. why is it an issue?

@Ph0enixKM
Copy link
Member

I think that we eventually will support sh. I just wanted to settle down how are we going to solve this problem holistically (including sh even if we do not implement it's support just yet)

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good @b1ek. It would be nice to improve that error message. I'll create issues to add constants to our editor plugins

@@ -36,6 +36,10 @@ impl SyntaxModule<ParserMetadata> for VariableSet {
let variable = handle_variable_reference(meta, tok.clone(), &self.name)?;
self.global_id = variable.global_id;
self.is_ref = variable.is_ref;
// Check for constant reassignment
if variable.is_const {
return error!(meta, tok, format!("Cannot reassign constant"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this error message say a bit more about what variable cannot be reassigned?

@@ -68,7 +69,8 @@ pub enum StatementType {
Mv(Mv),
CommandModifier(CommandModifier),
Comment(Comment),
CommentDoc(CommentDoc)
CommentDoc(CommentDoc),
ConstInit(ConstInit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be helpful to get in the habit of adding a comma to the last item of multi-line Rust lists (applies to struct fields, function definition parameters, function call arguments, vec![] initialisations, etc) when you're making changes anyway. This it makes subsequent pull requests (marginally) nicer. If this had been done to CommentDoc(CommentDoc) when that was added, you would have one changed line here, not two.

let global_id = self.is_global_scope().then(|| self.gen_var_id());
let scope = self.context.scopes.last_mut().unwrap();
scope.add_var(VariableDecl {
name: name.to_string(),
kind,
global_id,
is_ref: false
is_ref: false,
is_const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more places to add trailing commas, in add_var() and add_param().

}

impl SyntaxModule<ParserMetadata> for ConstInit {
syntax_name!("Constant Initialization");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equivalent line in init.rs is:

syntax_name!("Variable Initialize");

Consider changing one or the other to match.

Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved subject to suggestions from both myself and @Ph0enixKM.

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.

[Suggestion] constant declaration
5 participants