carvel-dev / ytt

YAML templating tool that works on YAML structure instead of text
https://carvel.dev/ytt
Apache License 2.0
1.63k stars 137 forks source link

Unindent does not match any outer indentation level #120

Closed nairb774 closed 4 years ago

nairb774 commented 4 years ago

I stumbled across an odd interaction between ytt and starlark. The following is a minimal reproducing snippet:

#@ for a in []:
#@   for b in []:
#@     for c in []:

---

#@     end
#@   end
#@ end

Which results in the error (from adding a sample to pkg/yamltemplate/filetests):

        - unindent does not match any outer indentation level
            stdin:8 | #@   end
        code:
        src:  tmpl: code: | srccode
        ????:    1: __ytt_tpl23_set_ctx_type("yaml") |
        ????:    2: __ytt_tpl23_start_ctx("yaml") |
        ????:    3: __ytt_tpl23_start_node(1) # *yamlmeta.DocumentSet |
           1:    4: __ytt_tpl23_start_node(2) # *yamlmeta.Document | #@ for a in []:
           1:    5: __ytt_tpl23_set_node(2) # *yamlmeta.Document | #@ for a in []:
           1:    6: for a in []: | #@ for a in []:
           2:    7:   for b in []: | #@   for b in []:
           3:    8:     for c in []: | #@     for c in []:
           5:    9: __ytt_tpl23_start_node(3) # *yamlmeta.Document | ---
           5:   10: __ytt_tpl23_set_node(3) # *yamlmeta.Document | ---
           7:   11:     end | #@     end
           8:   12:   end | #@   end
           9:   13: end | #@ end
        ????:   14: __ytt_tpl23_start_node(4) # *yamlmeta.Document |
        ????:   15: __ytt_tpl23_set_node(4) # *yamlmeta.Document |
        ????:   16: __ytt_tpl23_end_ctx(None) |

The following works correctly, though I would expect it to be wrong. Notice the second end is indented to match the first (innermost). So long as they have the same indentation, things seem to kind of work:

#@ for a in []:
#@   for b in []:
#@     for c in []:

---

#@     end
#@     end
#@ end

Last thing that I noticed is that it seems to be related to the --- within the loop.

cppforlife commented 4 years ago

very interesting, will take a look. thanks for submitting this.

pivotaljohn commented 4 years ago

We (i.e. @pivotal-dean and I) took a closer look at this symptom, today.

TL;DR. the original Starlark scanner detects an indentation error, even though it is not an error for ytt's augmenting scanner. We are considering two strategies: either remove the original scanner's error checking or insert whitespace to satisfy it.

Here's what we know:

The error we're seeing here is a result of the original scanner detecting what it believes is an error in indentation:

  1. the first injected Instruction (__ytt_tpl23_start_node(3) is fully outdented — so, the original scanner decides that we've closed all prior a levels of indentation;
  2. the second injected Instruction (_ytt_tpl23_set_node(3)) is also fully outdented, no indentation issue here;
  3. the end on line 7 is indented 4 spaces — so, the original scanner records that we have one legit indent at 4 spaces;
  4. the end on line 8 is indented 2 spaces (an outdent with respect to the prior line). However, the original scanner knows of no indent at 2 spaces. This is an indentation error since every outdent must correspond to a prior indent.

The bug is that this behavior is not ytt's intent: it attempts to ignore Starlark's original scanner's tracking of whitespace indentation. ...

We have come up with a few of ideas to address this:

  1. trim the whitespace on all templated code (although... this removes the author's ability to bake-in indentation of multi-line strings)
  2. in our fork of Starlark, remove the original scanner's error checking of indentation
  3. insert whitespace for generated Instructions

We're continuing to consider other options. This is what we've seen and considered so far.

(updated: reconsidering trimming source lines + disallowing multi-line strings in favor of string concatenation)

nairb774 commented 4 years ago

I spent a good 30 to 45 minutes trying to figure out the bug in the code before filing. After reading your description, I had no chance in that time frame :) Especially given how the repeated indentation worked (4 spaces, 4 spaces, 0 spaces), I was looking for an off-by-one error somewhere since all of the test samples always seemed to stop at 2 levels deep. Regardless, love the project.

pivotaljohn commented 4 years ago

Really appreciate your putting in that effort. The issue is well described. Having the distilled example made it easier for us to reproduce and hone-in on the essential parts.

Yeah, this took us a while to unpack the situation, ourselves (we didn't lean on @cppforlife's sherpa skills for this one).

cppforlife commented 4 years ago

fix will be included in v0.28.0

cppforlife commented 4 years ago

included in https://github.com/k14s/ytt/releases/tag/v0.28.0

flutterq commented 3 years ago

All possible working solution [Solved] IndentationError: unindent does not match any outer indentation level