From 6e99e591e359657f1ddefd86514495dfe14cabcc Mon Sep 17 00:00:00 2001 From: John Kerl Date: Tue, 2 Mar 2021 23:46:31 -0500 Subject: [PATCH] iterate on uninitialized-variable warnings --- go/reg-test/cases/case-dsl-warnings.sh | 47 ++++++ go/reg-test/expected/case-cli-help.sh.out | 48 ++++--- go/reg-test/expected/case-dsl-warnings.sh.out | 71 +++++++++ go/src/auxents/repl/dsl.go | 1 + go/src/auxents/repl/entry.go | 7 +- go/src/auxents/repl/verbs.go | 1 + go/src/dsl/cst/root.go | 14 +- go/src/dsl/cst/warn.go | 135 ++++++++++-------- go/src/transformers/put_or_filter.go | 39 +++-- go/todo.txt | 6 + 10 files changed, 277 insertions(+), 92 deletions(-) create mode 100644 go/reg-test/cases/case-dsl-warnings.sh create mode 100644 go/reg-test/expected/case-dsl-warnings.sh.out diff --git a/go/reg-test/cases/case-dsl-warnings.sh b/go/reg-test/cases/case-dsl-warnings.sh new file mode 100644 index 000000000..3a67cacd4 --- /dev/null +++ b/go/reg-test/cases/case-dsl-warnings.sh @@ -0,0 +1,47 @@ +# These should produce no warnings +run_mlr -n put 'y = 1' +run_mlr -n put -w 'y = 1' +run_mlr -n put -W 'y = 1' + +run_mlr -n put 'x = 3; y = x' +run_mlr -n put -w 'x = 3; y = x' +run_mlr -n put -W 'x = 3; y = x' + +run_mlr -n put -W 'for (k in $*) { print k }' +run_mlr -n put -W 'for (k,v in $*) { print k,v }' +run_mlr -n put -W 'for ((k1,k2),v in $*) { print k1,k2,v }' + +# Presence of $x not knowable until runtime; only catchable by a 'strict mode'. +run_mlr -n put 'y = $x' +run_mlr -n put -w 'y = $x' +run_mlr -n put -W 'y = $x' + +# x should be flagged as an uninitialized read +run_mlr -n put 'y = x' +run_mlr -n put -w 'y = x' +mlr_expect_fail -n put -W 'y = x' + +# Both x and y should be flagged as uninitialized reads +run_mlr -n put 'z = x + y' +run_mlr -n put -w 'z = x + y' +mlr_expect_fail -n put -W 'z = x + y' + +# This should produce no warnings +run_mlr -n put -W 'i = 0; z[i] = 1' + +# i should be flagged as an uninitialized read +mlr_expect_fail -n put -W 'z[i] = 1' + +# This should produce no warnings +run_mlr -n put -W 'func f(n) { return n+1 }' + +# m should be flagged as an uninitialized read +mlr_expect_fail -n put -W 'func f(n) { return m+1 }' +mlr_expect_fail -n put -W 'm = 0; func f(n) { return m+1 }' +mlr_expect_fail -n put -W 'subr f(n) { print m+1 }' +mlr_expect_fail -n put -W 'm = 0; subr f(n) { print m+1 }' + +# The uninit-warner isn't too smart. We expect this to not warn. (It would be +# great if someday it got smarter and warned here -- this test would need to be +# re-accepted.) +run_mlr -n put -W 'if (false) {x = 1}; print x' diff --git a/go/reg-test/expected/case-cli-help.sh.out b/go/reg-test/expected/case-cli-help.sh.out index 4b4d7be6d..a0f813a5d 100644 --- a/go/reg-test/expected/case-cli-help.sh.out +++ b/go/reg-test/expected/case-cli-help.sh.out @@ -141,9 +141,21 @@ semicolons to separate expressions.) -q Does not include the modified record in the output stream. Useful for when all desired output is in begin and/or end blocks. --p Prints the expressions's AST (abstract syntax tree), which gives - full transparency on the precedence and associativity rules of - Miller's grammar, to stdout. +-S and -F: There are no-ops in Miller 6 and above, since now type-inferencing is done + by the record-readers before filter/put is executed. Supported as no-op pass-through + flags for backward compatibility. + +-h|--help Show this message. + +Parser-info options: + +-w Print warnings about things like uninitialized variables. + +-W Same as -w, but exit the process if there are any warnings. + +-p Prints the expressions's AST (abstract syntax tree), which gives full + transparency on the precedence and associativity rules of Miller's grammar, + to stdout. -d Like -p but uses a parenthesized-expression format for the AST. @@ -155,12 +167,6 @@ semicolons to separate expressions.) -X Exit after parsing but before stream-processing. Useful with -v/-d/-D, if you only want to look at parser information. - --S and -F: There are no-ops in Miller 6 and above, since now type-inferencing is done - by the record-readers before filter/put is executed. Supported as no-op pass-through - flags for backward compatibility. - --h|--help Show this message. ================================================================ Usage: mlr flatten [options] Flattens multi-level maps to single-level ones. Example: field with name 'a' @@ -357,9 +363,21 @@ semicolons to separate expressions.) -q Does not include the modified record in the output stream. Useful for when all desired output is in begin and/or end blocks. --p Prints the expressions's AST (abstract syntax tree), which gives - full transparency on the precedence and associativity rules of - Miller's grammar, to stdout. +-S and -F: There are no-ops in Miller 6 and above, since now type-inferencing is done + by the record-readers before filter/put is executed. Supported as no-op pass-through + flags for backward compatibility. + +-h|--help Show this message. + +Parser-info options: + +-w Print warnings about things like uninitialized variables. + +-W Same as -w, but exit the process if there are any warnings. + +-p Prints the expressions's AST (abstract syntax tree), which gives full + transparency on the precedence and associativity rules of Miller's grammar, + to stdout. -d Like -p but uses a parenthesized-expression format for the AST. @@ -371,12 +389,6 @@ semicolons to separate expressions.) -X Exit after parsing but before stream-processing. Useful with -v/-d/-D, if you only want to look at parser information. - --S and -F: There are no-ops in Miller 6 and above, since now type-inferencing is done - by the record-readers before filter/put is executed. Supported as no-op pass-through - flags for backward compatibility. - --h|--help Show this message. ================================================================ Usage: mlr regularize [options] Outputs records sorted lexically ascending by keys.Options: diff --git a/go/reg-test/expected/case-dsl-warnings.sh.out b/go/reg-test/expected/case-dsl-warnings.sh.out new file mode 100644 index 000000000..fef361638 --- /dev/null +++ b/go/reg-test/expected/case-dsl-warnings.sh.out @@ -0,0 +1,71 @@ + +mlr -n put y = 1 + +mlr -n put -w y = 1 + +mlr -n put -W y = 1 + +mlr -n put x = 3; y = x + +mlr -n put -w x = 3; y = x + +mlr -n put -W x = 3; y = x + +mlr -n put -W for (k in $*) { print k } + +mlr -n put -W for (k,v in $*) { print k,v } + +mlr -n put -W for ((k1,k2),v in $*) { print k1,k2,v } + +mlr -n put y = $x + +mlr -n put -w y = $x + +mlr -n put -W y = $x + +mlr -n put y = x + +mlr -n put -w y = x +Variable name x might not have been assigned yet. + +mlr -n put -W y = x +Variable name x might not have been assigned yet. +mlr: Exiting due to warnings treated as fatal. + +mlr -n put z = x + y + +mlr -n put -w z = x + y +Variable name x might not have been assigned yet. +Variable name y might not have been assigned yet. + +mlr -n put -W z = x + y +Variable name x might not have been assigned yet. +Variable name y might not have been assigned yet. +mlr: Exiting due to warnings treated as fatal. + +mlr -n put -W i = 0; z[i] = 1 + +mlr -n put -W z[i] = 1 +Variable name i might not have been assigned yet. +mlr: Exiting due to warnings treated as fatal. + +mlr -n put -W func f(n) { return n+1 } + +mlr -n put -W func f(n) { return m+1 } +Variable name m might not have been assigned yet. +mlr: Exiting due to warnings treated as fatal. + +mlr -n put -W m = 0; func f(n) { return m+1 } +Variable name m might not have been assigned yet. +mlr: Exiting due to warnings treated as fatal. + +mlr -n put -W subr f(n) { print m+1 } +Variable name m might not have been assigned yet. +mlr: Exiting due to warnings treated as fatal. + +mlr -n put -W m = 0; subr f(n) { print m+1 } +Variable name m might not have been assigned yet. +mlr: Exiting due to warnings treated as fatal. + +mlr -n put -W if (false) {x = 1}; print x + diff --git a/go/src/auxents/repl/dsl.go b/go/src/auxents/repl/dsl.go index b5762e617..cac0aa81c 100644 --- a/go/src/auxents/repl/dsl.go +++ b/go/src/auxents/repl/dsl.go @@ -61,6 +61,7 @@ func (this *Repl) handleDSLStringAux( false, /*isFilter*/ isReplImmediate, doWarnings, + false, // warningsAreFatal ) if err != nil { return err diff --git a/go/src/auxents/repl/entry.go b/go/src/auxents/repl/entry.go index 48046a445..613f2c987 100644 --- a/go/src/auxents/repl/entry.go +++ b/go/src/auxents/repl/entry.go @@ -47,11 +47,10 @@ func ReplUsage(verbName string, o *os.File, exitCode int) { fmt.Fprint(o, `-v Prints the expressions's AST (abstract syntax tree), which gives -full transparency on the precedence and associativity rules of -Miller's grammar, to stdout. + full transparency on the precedence and associativity rules of + Miller's grammar, to stdout. --d Like -v but uses a parenthesized-expression format for the AST. Then, exits without - stream processing. +-d Like -v but uses a parenthesized-expression format for the AST. -D Like -d but with output all on one line. diff --git a/go/src/auxents/repl/verbs.go b/go/src/auxents/repl/verbs.go index 49c1d5df3..ed8ad6ae0 100644 --- a/go/src/auxents/repl/verbs.go +++ b/go/src/auxents/repl/verbs.go @@ -430,6 +430,7 @@ func handleSkipOrProcessUntil(this *Repl, dslString string, processingNotSkippin false, /*isFilter*/ true, /*isReplImmediate*/ this.doWarnings, + false, // warningsAreFatal ) if err != nil { fmt.Println(err) diff --git a/go/src/dsl/cst/root.go b/go/src/dsl/cst/root.go index eff98aabc..bc70dad32 100644 --- a/go/src/dsl/cst/root.go +++ b/go/src/dsl/cst/root.go @@ -60,6 +60,7 @@ func (this *RootNode) IngestAST( // (immediately) but not retained. isReplImmediate bool, doWarnings bool, + warningsAreFatal bool, ) error { if ast.RootNode == nil { return errors.New("Cannot build CST from nil AST root") @@ -74,9 +75,16 @@ func (this *RootNode) IngestAST( } if doWarnings { - err = WarnOnAST(ast) - if err != nil { - return err + ok := WarnOnAST(ast) + if !ok { + // Messages already printed out + if warningsAreFatal { + fmt.Printf( + "%s: Exiting due to warnings treated as fatal.\n", + lib.MlrExeName(), + ) + os.Exit(1) + } } } diff --git a/go/src/dsl/cst/warn.go b/go/src/dsl/cst/warn.go index 93e588aeb..b1da30f4a 100644 --- a/go/src/dsl/cst/warn.go +++ b/go/src/dsl/cst/warn.go @@ -1,5 +1,8 @@ // ================================================================ -// TODO: comment +// Shows warnings for things like uninitialized variables. These are things +// that are statically computable from the AST by itself -- it confines itself +// to local-variable analysis. There are other uninitialization issues +// detectable only at runtime, which would benefit from a 'strict mode'. // ================================================================ package cst @@ -12,32 +15,36 @@ import ( ) // ---------------------------------------------------------------- +// Returns true if there are no warnings. func WarnOnAST( ast *dsl.AST, -) error { +) bool { variableNamesWrittenTo := make(map[string]bool) inAssignment := false + ok := true if ast.RootNode.Children != nil { for _, astChild := range ast.RootNode.Children { - err := warnOnASTAux( + ok1 := warnOnASTAux( astChild, variableNamesWrittenTo, inAssignment, ) - if err != nil { - return err - } + // Don't end early on first warning; tree-walk to list them all. + ok = ok1 && ok } } - return nil + return ok } // ---------------------------------------------------------------- +// Example ASTs: +// // $ mlr -n put -v 'z = x + y' // DSL EXPRESSION: // z = x + y +// // AST: // * statement block // * assignment "=" @@ -49,6 +56,7 @@ func WarnOnAST( // $ mlr -n put -v 'z[i] = x + y' // DSL EXPRESSION: // z[i] = x + y +// // AST: // * statement block // * assignment "=" @@ -58,10 +66,11 @@ func WarnOnAST( // * operator "+" // * local variable "x" // * local variable "y" - +// // $ mlr -n put -v 'func f(n) { return n+1}' // DSL EXPRESSION: // func f(n) { return n+1} +// // AST: // * statement block // * function definition "f" @@ -73,108 +82,120 @@ func WarnOnAST( // * operator "+" // * local variable "n" // * int literal "1" -// -// Variable name n might not have been assigned yet. +// Returns true if there are no warnings func warnOnASTAux( astNode *dsl.ASTNode, variableNamesWrittenTo map[string]bool, inAssignment bool, -) error { +) bool { + ok := true + + // Check local-variable references, and see if they're reads or writes + // based on the AST parenting of this node. if astNode.Type == dsl.NodeTypeLocalVariable { - // xxx todo: 'z[i] = x + y' where z is set but i is not. - // z is a write but i is a read :^/ - variableName := string(astNode.Token.Lit) if inAssignment { variableNamesWrittenTo[variableName] = true } else { if !variableNamesWrittenTo[variableName] { - // xxx todo: this would be much more useful with line numbers. :( + // TODO: this would be much more useful with line numbers. :( + // That would be a big of work with the parser. Fortunately, + // Miller is designed around low-keystroke little expressions + // -- not thousands of lines of Miller-DSL source code -- so + // people can look at their few lines of Miller-DSL code and + // spot their error. fmt.Printf( "Variable name %s might not have been assigned yet.\n", variableName, ) + ok = false } } + } else if astNode.Type == dsl.NodeTypeBeginBlock { - // TODO: comment why reset + // Locals are confined to begin/end blocks and func/subr blocks. + // Reset for this part of the treewalk. variableNamesWrittenTo = make(map[string]bool) } else if astNode.Type == dsl.NodeTypeEndBlock { - // TODO: comment why reset + // Locals are confined to begin/end blocks and func/subr blocks. + // Reset for this part of the treewalk. variableNamesWrittenTo = make(map[string]bool) + } else if astNode.Type == dsl.NodeTypeFunctionDefinition { - // TODO: comment why reset - // TODO: propagate parameter-list back from first node - var err error = nil - variableNamesWrittenTo, err = noteParametersForWarnings(astNode) - if err != nil { - return err - } + // Locals are confined to begin/end blocks and func/subr blocks. Reset + // for this part of the treewalk, except mark the parameters as + // defined. + variableNamesWrittenTo = noteParametersForWarnings(astNode) } else if astNode.Type == dsl.NodeTypeSubroutineDefinition { - // TODO: comment why reset - // TODO: propagate parameter-list back from first node - var err error = nil - variableNamesWrittenTo, err = noteParametersForWarnings(astNode) - if err != nil { - return err - } + // Locals are confined to begin/end blocks and func/subr blocks. Reset + // for this part of the treewalk, except mark the parameters as + // defined. + variableNamesWrittenTo = noteParametersForWarnings(astNode) } - // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - // Treewalk + // Treewalk to check the rest of the AST below this node. if astNode.Children != nil { for i, astChild := range astNode.Children { childInAssignment := inAssignment if astNode.Type == dsl.NodeTypeAssignment && i == 0 { + // LHS of assignment statements childInAssignment = true } else if astNode.Type == dsl.NodeTypeForLoopOneVariable && i == 0 { + // The 'k' in 'for (k in $*)' childInAssignment = true } else if astNode.Type == dsl.NodeTypeForLoopTwoVariable && (i == 0 || i == 1) { + // The 'k' and 'v' in 'for (k,v in $*)' childInAssignment = true } else if astNode.Type == dsl.NodeTypeForLoopMultivariable && (i == 0 || i == 1) { + // The 'k1', 'k2', and 'v' in 'for ((k1,k2),v in $*)' childInAssignment = true } else if astNode.Type == dsl.NodeTypeParameterList { childInAssignment = true + } else if inAssignment && astNode.Type == dsl.NodeTypeArrayOrMapIndexAccess { + // In 'z[i] = 1', the 'i' is a read and the 'z' is a write. + // + // mlr --from r put -v -W 'z[i] = 1' + // DSL EXPRESSION: + // z[i]=1 + // + // AST: + // * statement block + // * assignment "=" + // * array or map index access "[]" + // * local variable "z" + // * local variable "i" + // * int literal "1" + if i == 0 { + childInAssignment = true + } else { + childInAssignment = false + } } - err := warnOnASTAux( + ok1 := warnOnASTAux( astChild, variableNamesWrittenTo, childInAssignment, ) - if err != nil { - return err - } + // Don't end early on first error; tree-walk to list them all. + ok = ok1 && ok } } - return nil + return ok } // ---------------------------------------------------------------- -// $ mlr -n put -v 'func f(n) { return n+1}' -// DSL EXPRESSION: -// func f(n) { return n+1} -// AST: -// * statement block -// * function definition "f" -// * parameter list -// * parameter -// * parameter name "n" -// * statement block -// * return "return" -// * operator "+" -// * local variable "n" -// * int literal "1" -// -// Variable name n might not have been assigned yet. - +// Given a func/subr block, find the names of its parameters. All the +// lib.InternalCodingErrorIf parts are shape-assertions to make sure this code +// is in sync with the BNF grammar which builds the AST from a Miller-DSL +// source string. func noteParametersForWarnings( astNode *dsl.ASTNode, -) (map[string]bool, error) { +) map[string]bool { variableNamesWrittenTo := make(map[string]bool) @@ -195,5 +216,5 @@ func noteParametersForWarnings( variableNamesWrittenTo[parameterName] = true } - return variableNamesWrittenTo, nil + return variableNamesWrittenTo } diff --git a/go/src/transformers/put_or_filter.go b/go/src/transformers/put_or_filter.go index a649398d5..a63b0a181 100644 --- a/go/src/transformers/put_or_filter.go +++ b/go/src/transformers/put_or_filter.go @@ -86,9 +86,21 @@ semicolons to separate expressions.) -q Does not include the modified record in the output stream. Useful for when all desired output is in begin and/or end blocks. --p Prints the expressions's AST (abstract syntax tree), which gives - full transparency on the precedence and associativity rules of - Miller's grammar, to stdout. +-S and -F: There are no-ops in Miller 6 and above, since now type-inferencing is done + by the record-readers before filter/put is executed. Supported as no-op pass-through + flags for backward compatibility. + +-h|--help Show this message. + +Parser-info options: + +-w Print warnings about things like uninitialized variables. + +-W Same as -w, but exit the process if there are any warnings. + +-p Prints the expressions's AST (abstract syntax tree), which gives full + transparency on the precedence and associativity rules of Miller's grammar, + to stdout. -d Like -p but uses a parenthesized-expression format for the AST. @@ -100,12 +112,6 @@ semicolons to separate expressions.) -X Exit after parsing but before stream-processing. Useful with -v/-d/-D, if you only want to look at parser information. - --S and -F: There are no-ops in Miller 6 and above, since now type-inferencing is done - by the record-readers before filter/put is executed. Supported as no-op pass-through - flags for backward compatibility. - --h|--help Show this message. `) if doExit { @@ -134,6 +140,7 @@ func transformerPutOrFilterParseCLI( printASTSingleLine := false exitAfterParse := false doWarnings := false + warningsAreFatal := false invertFilter := false suppressOutputRecord := false presets := make([]string, 0) @@ -203,6 +210,10 @@ func transformerPutOrFilterParseCLI( exitAfterParse = true } else if opt == "-w" { doWarnings = true + warningsAreFatal = false + } else if opt == "-W" { + doWarnings = true + warningsAreFatal = true } else if opt == "-S" { // TODO: this is a no-op in Miller 6 and above. @@ -251,6 +262,7 @@ func transformerPutOrFilterParseCLI( printASTSingleLine, exitAfterParse, doWarnings, + warningsAreFatal, invertFilter, suppressOutputRecord, recordWriterOptions, @@ -284,6 +296,7 @@ func NewTransformerPut( printASTSingleLine bool, exitAfterParse bool, doWarnings bool, + warningsAreFatal bool, invertFilter bool, suppressOutputRecord bool, recordWriterOptions *cliutil.TWriterOptions, @@ -319,7 +332,13 @@ func NewTransformerPut( os.Exit(0) } - err = cstRootNode.IngestAST(astRootNode, isFilter, false, doWarnings) // TODO: split out methods ... + err = cstRootNode.IngestAST( + astRootNode, + isFilter, + false, // isReplImmediate + doWarnings, + warningsAreFatal, + ) if err != nil { fmt.Fprintln(os.Stderr, err) return nil, err diff --git a/go/todo.txt b/go/todo.txt index b87389145..ac68e8c6e 100644 --- a/go/todo.txt +++ b/go/todo.txt @@ -53,6 +53,8 @@ TOP OF LIST: o xls note (issue #396) @ file-format docs o 'string or int' -> 'string, int, or array thereof' throughout +* https://segment.com/blog/allocation-efficiency-in-high-performance-go-services/ + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - * c/go both: o https://brandur.org/logfmt is simply DKVP w/ IFS = space (need dquot though) @@ -499,3 +501,7 @@ i https://en.wikipedia.org/wiki/Delimiter#Delimiter_collision o make a thorough audit, and warn everywhere o either do copy for all retainers, or treat inrecs as immutable ... o 'this.recordsAndContexts.PushBack(inrecAndContext)' idiom needs copy everywhere ... + +* consider -w/-W to stderr not stdout -- ? +* doc6 re warnings +* -W to mlr main also -- so it can be used with .mlrrc