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

fix: remove variable_definition and assignment rules from pipeline #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guilhas07
Copy link
Contributor

@guilhas07 guilhas07 commented Jan 31, 2025

Problem

Rules like variable_definition, assignment, range_action,
template_action, block_action, chained_pipeline, argument_list
incorrectly allowed for variable definitions and assignments inside
them.

Solution

Remove rules variable_definition and assignment from
pipeline, using a new rule where both are supported.


Go parser doesn't error for variables definitions, assignments inside block and
chain operations, despite not working. So I couldn't create a invalid tests
for those cases.

Examples that don't trigger a parsing error:

{{block "name" $var1 := "test"}} T1 {{ end }} -> Correction: This is valid
{{ $var := 5 | print }} -> This is also a valid variable declaration...

We can hack it by mentioning the variable like so:

{{block "name" $var := 5}} {{$var}} {{end}}
{{ $var := 5 | print $var }}

but at this point I don't think it is worth it.

@qvalentin
Copy link
Collaborator

I'm not sure about the point about block nodes, it does work, if you access the variable after the block:

{{ $var1 := 2 }}
{{block "name" $var1 = "test"}} T1 {{ end }}
{{ print $var1 }}

We should try to be aligned with the parser of gotemplate and not assume additional logic.

@guilhas07
Copy link
Contributor Author

guilhas07 commented Feb 8, 2025

I'm not sure about the point about block nodes, it does work, if you access the variable after the block:

{{ $var1 := 2 }}
{{block "name" $var1 = "test"}} T1 {{ end }}
{{ print $var1 }}

We should try to be aligned with the parser of gotemplate and not assume additional logic.

I only tried to access the variable inside the block and not outside, so I completely missed that. Was my understanding also incorrect in the chained pipeline case?

Edit: My example is a valid variable declaration. So a better example would be a variable declaration in the middle of a chained pipeline.

@qvalentin
Copy link
Collaborator

Technically this should be allowed:

{{ $var := 2 | print }} 

But it would require more changes in the chained pipeline rule.

=============================================
Invalid range
=============================================
{{range $var=5}} t1 {{ end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very interesting,

this is not allowed:

{{ $var:=list }}
{{range $var=list}} t1 {{ end }}

But this is (even executes):

{{range $var:=list}} t1 {{ end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the second is an element range. I added support for that in #32

@@ -35,6 +35,21 @@ Function with arguments
(field_identifier))
(int_literal))))

====================================
Invalid Function
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be called Invalid function parameters.

Also the same thing with with variable definition could be added:

{{ print $var:=5 }}

@guilhas07
Copy link
Contributor Author

Technically this should be allowed:

{{ $var := 2 | print }} 

But it would require more changes in the chained pipeline rule.

I think it already is supported:

====================================
A with action that creates and uses a variable.
====================================
{{with $x := "output" | printf "%q"}}{{$x}}{{end}}
---
(template
(with_action
(variable_definition
(variable (identifier))
(chained_pipeline
(interpreted_string_literal)
(function_call
(identifier)
(argument_list
(interpreted_string_literal)))))
(variable (identifier))))

Problem: Rules like `variable_definition`, `assignment`, `range_action`,
`template_action`, `chained_pipeline`, `argument_list`
incorrectly allowed for variable definitions and assignments inside
them.
Solution: Remove rules `variable_definition` and `assignment` from
`pipeline`, using a new rule where both are supported.
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.

2 participants