From f87a794800f99bdb20164b6a3b42080fa618b827 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 16 Feb 2018 17:37:22 -0800 Subject: [PATCH] hclsyntax: check for and report incorrect peeker stack discipline The peeker has an "include newlines" stack which the parser manipulates to switch between the newline-sensitive and non-sensitive scanning modes. If the parser code fails to manage this stack correctly (for example, due to a missed call to PopIncludeNewlines) then this causes very confusing downstream errors that are otherwise difficult to debug. As an extra debug tool for when errors _are_ detected, when this problem is encountered during tests we are able to produce a visualization of the pushes and pops to help the test developer see which pushes and pops seem out of place. This is a lot of ugly extra code but it's usually disabled and seems worth it to allow us to catch quickly bugs that would otherwise be quite difficult to diagnose. --- hcl/hclsyntax/peeker.go | 104 +++++++++++++++++++++++++++++++++++ hcl/hclsyntax/peeker_test.go | 5 ++ hcl/hclsyntax/public.go | 27 +++++++++ 3 files changed, 136 insertions(+) diff --git a/hcl/hclsyntax/peeker.go b/hcl/hclsyntax/peeker.go index b8171ff..5a4b50e 100644 --- a/hcl/hclsyntax/peeker.go +++ b/hcl/hclsyntax/peeker.go @@ -1,15 +1,38 @@ package hclsyntax import ( + "bytes" + "fmt" + "path/filepath" + "runtime" + "strings" + "github.com/hashicorp/hcl2/hcl" ) +// This is set to true at init() time in tests, to enable more useful output +// if a stack discipline error is detected. It should not be enabled in +// normal mode since there is a performance penalty from accessing the +// runtime stack to produce the traces, but could be temporarily set to +// true for debugging if desired. +var tracePeekerNewlinesStack = false + type peeker struct { Tokens Tokens NextIndex int IncludeComments bool IncludeNewlinesStack []bool + + // used only when tracePeekerNewlinesStack is set + newlineStackChanges []peekerNewlineStackChange +} + +// for use in debugging the stack usage only +type peekerNewlineStackChange struct { + Pushing bool // if false, then popping + Frame runtime.Frame + Include bool } func newPeeker(tokens Tokens, includeComments bool) *peeker { @@ -97,6 +120,18 @@ func (p *peeker) includingNewlines() bool { } func (p *peeker) PushIncludeNewlines(include bool) { + if tracePeekerNewlinesStack { + // Record who called us so that we can more easily track down any + // mismanagement of the stack in the parser. + callers := []uintptr{0} + runtime.Callers(2, callers) + frames := runtime.CallersFrames(callers) + frame, _ := frames.Next() + p.newlineStackChanges = append(p.newlineStackChanges, peekerNewlineStackChange{ + true, frame, include, + }) + } + p.IncludeNewlinesStack = append(p.IncludeNewlinesStack, include) } @@ -104,5 +139,74 @@ func (p *peeker) PopIncludeNewlines() bool { stack := p.IncludeNewlinesStack remain, ret := stack[:len(stack)-1], stack[len(stack)-1] p.IncludeNewlinesStack = remain + + if tracePeekerNewlinesStack { + // Record who called us so that we can more easily track down any + // mismanagement of the stack in the parser. + callers := []uintptr{0} + runtime.Callers(2, callers) + frames := runtime.CallersFrames(callers) + frame, _ := frames.Next() + p.newlineStackChanges = append(p.newlineStackChanges, peekerNewlineStackChange{ + false, frame, ret, + }) + } + return ret } + +// AssertEmptyNewlinesStack checks if the IncludeNewlinesStack is empty, doing +// panicking if it is not. This can be used to catch stack mismanagement that +// might otherwise just cause confusing downstream errors. +// +// This function is a no-op if the stack is empty when called. +// +// If newlines stack tracing is enabled by setting the global variable +// tracePeekerNewlinesStack at init time, a full log of all of the push/pop +// calls will be produced to help identify which caller in the parser is +// misbehaving. +func (p *peeker) AssertEmptyIncludeNewlinesStack() { + if len(p.IncludeNewlinesStack) != 1 { + // Should never happen; indicates mismanagement of the stack inside + // the parser. + if p.newlineStackChanges != nil { // only if traceNewlinesStack is enabled above + panic(fmt.Errorf( + "non-empty IncludeNewlinesStack after parse with %d calls unaccounted for:\n%s", + len(p.IncludeNewlinesStack)-1, + formatPeekerNewlineStackChanges(p.newlineStackChanges), + )) + } else { + panic(fmt.Errorf("non-empty IncludeNewlinesStack after parse: %#v", p.IncludeNewlinesStack)) + } + } +} + +func formatPeekerNewlineStackChanges(changes []peekerNewlineStackChange) string { + indent := 0 + var buf bytes.Buffer + for _, change := range changes { + funcName := change.Frame.Function + if idx := strings.LastIndexByte(funcName, '.'); idx != -1 { + funcName = funcName[idx+1:] + } + filename := change.Frame.File + if idx := strings.LastIndexByte(filename, filepath.Separator); idx != -1 { + filename = filename[idx+1:] + } + + switch change.Pushing { + + case true: + buf.WriteString(strings.Repeat(" ", indent)) + fmt.Fprintf(&buf, "PUSH %#v (%s at %s:%d)\n", change.Include, funcName, filename, change.Frame.Line) + indent++ + + case false: + indent-- + buf.WriteString(strings.Repeat(" ", indent)) + fmt.Fprintf(&buf, "POP %#v (%s at %s:%d)\n", change.Include, funcName, filename, change.Frame.Line) + + } + } + return buf.String() +} diff --git a/hcl/hclsyntax/peeker_test.go b/hcl/hclsyntax/peeker_test.go index ab5f905..626d77b 100644 --- a/hcl/hclsyntax/peeker_test.go +++ b/hcl/hclsyntax/peeker_test.go @@ -5,6 +5,11 @@ import ( "testing" ) +func init() { + // see the documentation of this variable for more information + tracePeekerNewlinesStack = true +} + func TestPeeker(t *testing.T) { tokens := Tokens{ { diff --git a/hcl/hclsyntax/public.go b/hcl/hclsyntax/public.go index e527d63..cf0ee29 100644 --- a/hcl/hclsyntax/public.go +++ b/hcl/hclsyntax/public.go @@ -20,6 +20,12 @@ func ParseConfig(src []byte, filename string, start hcl.Pos) (*hcl.File, hcl.Dia parser := &parser{peeker: peeker} body, parseDiags := parser.ParseBody(TokenEOF) diags = append(diags, parseDiags...) + + // Panic if the parser uses incorrect stack discipline with the peeker's + // newlines stack, since otherwise it will produce confusing downstream + // errors. + peeker.AssertEmptyIncludeNewlinesStack() + return &hcl.File{ Body: body, Bytes: src, @@ -54,6 +60,13 @@ func ParseExpression(src []byte, filename string, start hcl.Pos) (Expression, hc }) } + parser.PopIncludeNewlines() + + // Panic if the parser uses incorrect stack discipline with the peeker's + // newlines stack, since otherwise it will produce confusing downstream + // errors. + peeker.AssertEmptyIncludeNewlinesStack() + return expr, diags } @@ -65,6 +78,12 @@ func ParseTemplate(src []byte, filename string, start hcl.Pos) (Expression, hcl. parser := &parser{peeker: peeker} expr, parseDiags := parser.ParseTemplate() diags = append(diags, parseDiags...) + + // Panic if the parser uses incorrect stack discipline with the peeker's + // newlines stack, since otherwise it will produce confusing downstream + // errors. + peeker.AssertEmptyIncludeNewlinesStack() + return expr, diags } @@ -85,6 +104,14 @@ func ParseTraversalAbs(src []byte, filename string, start hcl.Pos) (hcl.Traversa expr, parseDiags := parser.ParseTraversalAbs() diags = append(diags, parseDiags...) + + parser.PopIncludeNewlines() + + // Panic if the parser uses incorrect stack discipline with the peeker's + // newlines stack, since otherwise it will produce confusing downstream + // errors. + peeker.AssertEmptyIncludeNewlinesStack() + return expr, diags }