Skip to content

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

(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