Generate runtime conversion diagnostics

Instead of inserting a comment with error description into the generated code,
generate a call to a function that will print out a conversion error when executed.
Do not print generic "partially converted" message anymore.
Remove --verbose and --no_warnings options.

Bug: 204062171
Test: internal
Change-Id: Ib126e16dc76f49635e4939e670922f2561781049

mk2rbc/cmd/mk2rbc.go

@@ -46,8 +46,6 @@ var (
 	dryRun   = flag.Bool("dry_run", false, "dry run")
 	recurse  = flag.Bool("convert_dependents", false, "convert all dependent files")
 	mode     = flag.String("mode", "", `"backup" to back up existing files, "write" to overwrite them`)
-	noWarn   = flag.Bool("no_warnings", false, "don't warn about partially failed conversions")
-	verbose  = flag.Bool("v", false, "print summary")
 	errstat  = flag.Bool("error_stat", false, "print error statistics")
 	traceVar = flag.String("trace", "", "comma-separated list of variables to trace")
 	// TODO(asmundak): this option is for debugging
@@ -75,7 +73,6 @@ func init() {
 	flagAlias("root", "d")
 	flagAlias("dry_run", "n")
 	flagAlias("convert_dependents", "r")
-	flagAlias("no_warnings", "w")
 	flagAlias("error_stat", "e")
 }
 
@@ -207,9 +204,9 @@ func main() {
 		}
 	}
 
-	printStats()
 	if *errstat {
 		errorLogger.printStatistics()
+		printStats()
 	}
 	if !ok {
 		os.Exit(1)
@@ -336,7 +333,6 @@ func convertOne(mkFile string) (ok bool) {
 		OutputSuffix:    *suffix,
 		TracedVariables: tracedVariables,
 		TraceCalls:      *traceCalls,
-		WarnPartialSuccess: !*noWarn,
 		SourceFS:        os.DirFS(*rootDir),
 		MakefileFinder:  makefileFinder,
 		ErrorLogger:     errorLogger,
@@ -417,9 +413,6 @@ func writeGenerated(path string, contents string) error {
 
 func printStats() {
 	var sortedFiles []string
-	if *noWarn && !*verbose {
-		return
-	}
 	for p := range converted {
 		sortedFiles = append(sortedFiles, p)
 	}
@@ -435,7 +428,6 @@ func printStats() {
 			nOk++
 		}
 	}
-	if !*noWarn {
 	if nPartial > 0 {
 		fmt.Fprintf(os.Stderr, "Conversion was partially successful for:\n")
 		for _, f := range sortedFiles {
@@ -453,10 +445,6 @@ func printStats() {
 			}
 		}
 	}
-	}
-	if *verbose && (nPartial > 0 || nFailed > 0) {
-		fmt.Fprintln(os.Stderr, "Succeeded: ", nOk, " Partial: ", nPartial, " Failed: ", nFailed)
-	}
 }
 
 type datum struct {
@@ -468,8 +456,8 @@ type errorSink struct {
 	data map[string]datum
 }
 
-func (ebt errorSink) NewError(sourceFile string, sourceLine int, node parser.Node, message string, args ...interface{}) {
+func (ebt errorSink) NewError(el mk2rbc.ErrorLocation, node parser.Node, message string, args ...interface{}) {
-	fmt.Fprintf(os.Stderr, "%s:%d ", sourceFile, sourceLine)
+	fmt.Fprint(os.Stderr, el, ": ")
 	fmt.Fprintf(os.Stderr, message, args...)
 	fmt.Fprintln(os.Stderr)
 	if !*errstat {

mk2rbc/expr.go

@@ -18,8 +18,6 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
-
-	mkparser "android/soong/androidmk/parser"
 )
 
 // Represents an expression in the Starlark code. An expression has
@@ -106,7 +104,7 @@ func (xi *interpolateExpr) emit(gctx *generationContext) {
 		format += "%s" + strings.ReplaceAll(chunk, "%", "%%")
 	}
 	gctx.writef("%q %% ", format)
-	emitarg := func(arg starlarkExpr) {
+	emitArg := func(arg starlarkExpr) {
 		if arg.typ() == starlarkTypeList {
 			gctx.write(`" ".join(`)
 			arg.emit(gctx)
@@ -116,12 +114,12 @@ func (xi *interpolateExpr) emit(gctx *generationContext) {
 		}
 	}
 	if len(xi.args) == 1 {
-		emitarg(xi.args[0])
+		emitArg(xi.args[0])
 	} else {
 		sep := "("
 		for _, arg := range xi.args {
 			gctx.write(sep)
-			emitarg(arg)
+			emitArg(arg)
 			sep = ", "
 		}
 		gctx.write(")")
@@ -414,7 +412,7 @@ func newStringListExpr(items []string) *listExpr {
 	return &v
 }
 
-// concatExpr generates epxr1 + expr2 + ... + exprN in Starlark.
+// concatExpr generates expr1 + expr2 + ... + exprN in Starlark.
 type concatExpr struct {
 	items []starlarkExpr
 }
@@ -607,7 +605,7 @@ func (cx *callExpr) emitListVarCopy(gctx *generationContext) {
 }
 
 type badExpr struct {
-	node    mkparser.Node
+	errorLocation ErrorLocation
 	message       string
 }
 
@@ -617,15 +615,15 @@ func (b *badExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool)
 	return
 }
 
-func (b *badExpr) emit(_ *generationContext) {
+func (b *badExpr) emit(gctx *generationContext) {
-	panic("implement me")
+	gctx.emitConversionError(b.errorLocation, b.message)
 }
 
 func (_ *badExpr) typ() starlarkType {
 	return starlarkTypeUnknown
 }
 
-func (b *badExpr) emitListVarCopy(gctx *generationContext) {
+func (_ *badExpr) emitListVarCopy(_ *generationContext) {
 	panic("implement me")
 }
 

mk2rbc/mk2rbc.go

@@ -165,7 +165,6 @@ type Request struct {
 	ErrorLogger     ErrorLogger
 	TracedVariables []string // trace assignment to these variables
 	TraceCalls      bool
-	WarnPartialSuccess bool
 	SourceFS        fs.FS
 	MakefileFinder  MakefileFinder
 }
@@ -173,7 +172,16 @@ type Request struct {
 // ErrorLogger prints errors and gathers error statistics.
 // Its NewError function is called on every error encountered during the conversion.
 type ErrorLogger interface {
-	NewError(sourceFile string, sourceLine int, node mkparser.Node, text string, args ...interface{})
+	NewError(el ErrorLocation, node mkparser.Node, text string, args ...interface{})
+}
+
+type ErrorLocation struct {
+	MkFile string
+	MkLine int
+}
+
+func (el ErrorLocation) String() string {
+	return fmt.Sprintf("%s:%d", el.MkFile, el.MkLine)
 }
 
 // Derives module name for a given file. It is base name
@@ -249,10 +257,6 @@ func (gctx *generationContext) emit() string {
 		node.emit(gctx)
 	}
 
-	if ss.hasErrors && ss.warnPartialSuccess {
-		gctx.newLine()
-		gctx.writef("%s(%q, %q)", cfnWarning, filepath.Base(ss.mkFile), "partially successful conversion")
-	}
 	if gctx.starScript.traceCalls {
 		gctx.newLine()
 		gctx.writef(`print("<%s")`, gctx.starScript.mkFile)
@@ -307,6 +311,10 @@ func (gctx *generationContext) newLine() {
 	gctx.writef("%*s", 2*gctx.indentLevel, "")
 }
 
+func (gctx *generationContext) emitConversionError(el ErrorLocation, message string) {
+	gctx.writef(`rblf.mk2rbc_error("%s", %q)`, el, message)
+}
+
 type knownVariable struct {
 	name      string
 	class     varClass
@@ -379,7 +387,6 @@ type StarlarkScript struct {
 	hasErrors      bool
 	topDir         string
 	traceCalls     bool // print enter/exit each init function
-	warnPartialSuccess bool
 	sourceFS       fs.FS
 	makefileFinder MakefileFinder
 	nodeLocator    func(pos mkparser.Pos) int
@@ -561,7 +568,7 @@ func (ctx *parseContext) handleAssignment(a *mkparser.Assignment) {
 		return
 	}
 	_, isTraced := ctx.tracedVariables[name]
-	asgn := &assignmentNode{lhs: lhs, mkValue: a.Value, isTraced: isTraced}
+	asgn := &assignmentNode{lhs: lhs, mkValue: a.Value, isTraced: isTraced, location: ctx.errorLocation(a)}
 	if lhs.valueType() == starlarkTypeUnknown {
 		// Try to divine variable type from the RHS
 		asgn.value = ctx.parseMakeString(a, a.Value)
@@ -1024,10 +1031,10 @@ func (ctx *parseContext) parseCondition(check *mkparser.Directive) starlarkNode
 func (ctx *parseContext) newBadExpr(node mkparser.Node, text string, args ...interface{}) starlarkExpr {
 	message := fmt.Sprintf(text, args...)
 	if ctx.errorLogger != nil {
-		ctx.errorLogger.NewError(ctx.script.mkFile, ctx.script.nodeLocator(node.Pos()), node, text, args...)
+		ctx.errorLogger.NewError(ctx.errorLocation(node), node, text, args...)
 	}
 	ctx.script.hasErrors = true
-	return &badExpr{node, message}
+	return &badExpr{errorLocation: ctx.errorLocation(node), message: message}
 }
 
 func (ctx *parseContext) parseCompare(cond *mkparser.Directive) starlarkExpr {
@@ -1545,18 +1552,14 @@ func (ctx *parseContext) carryAsComment(failedNode mkparser.Node) {
 // records that the given node failed to be converted and includes an explanatory message
 func (ctx *parseContext) errorf(failedNode mkparser.Node, message string, args ...interface{}) {
 	if ctx.errorLogger != nil {
-		ctx.errorLogger.NewError(ctx.script.mkFile, ctx.script.nodeLocator(failedNode.Pos()), failedNode, message, args...)
+		ctx.errorLogger.NewError(ctx.errorLocation(failedNode), failedNode, message, args...)
 	}
-	message = fmt.Sprintf(message, args...)
+	ctx.receiver.newNode(&exprNode{ctx.newBadExpr(failedNode, message, args...)})
-	ctx.insertComment(fmt.Sprintf("# MK2RBC TRANSLATION ERROR: %s", message))
-	ctx.carryAsComment(failedNode)
-
 	ctx.script.hasErrors = true
 }
 
 func (ctx *parseContext) wrapBadExpr(xBad *badExpr) {
-	ctx.insertComment(fmt.Sprintf("# MK2RBC TRANSLATION ERROR: %s", xBad.message))
+	ctx.receiver.newNode(&exprNode{xBad})
-	ctx.carryAsComment(xBad.node)
 }
 
 func (ctx *parseContext) loadedModulePath(path string) string {
@@ -1622,6 +1625,10 @@ func (ctx *parseContext) hasNamespaceVar(namespaceName string, varName string) b
 	return ok
 }
 
+func (ctx *parseContext) errorLocation(node mkparser.Node) ErrorLocation {
+	return ErrorLocation{ctx.script.mkFile, ctx.script.nodeLocator(node.Pos())}
+}
+
 func (ss *StarlarkScript) String() string {
 	return NewGenerateContext(ss).emit()
 }
@@ -1664,7 +1671,6 @@ func Convert(req Request) (*StarlarkScript, error) {
 		mkFile:         req.MkFile,
 		topDir:         req.RootDir,
 		traceCalls:     req.TraceCalls,
-		warnPartialSuccess: req.WarnPartialSuccess,
 		sourceFS:       req.SourceFS,
 		makefileFinder: req.MakefileFinder,
 		nodeLocator:    func(pos mkparser.Pos) int { return parser.Unpack(pos).Line },

mk2rbc/mk2rbc_test.go

@@ -105,15 +105,12 @@ def init(g, handle):
 PRODUCT_NAME := $(call foo1, bar)
 PRODUCT_NAME := $(call foo0)
 `,
-		expected: `# MK2RBC TRANSLATION ERROR: cannot handle invoking foo1
+		expected: `load("//build/make/core:product_config.rbc", "rblf")
-# PRODUCT_NAME := $(call foo1, bar)
-# MK2RBC TRANSLATION ERROR: cannot handle invoking foo0
-# PRODUCT_NAME := $(call foo0)
-load("//build/make/core:product_config.rbc", "rblf")
 
 def init(g, handle):
   cfg = rblf.cfg(handle)
-  rblf.warning("product.mk", "partially successful conversion")
+  rblf.mk2rbc_error("product.mk:2", "cannot handle invoking foo1")
+  rblf.mk2rbc_error("product.mk:3", "cannot handle invoking foo0")
 `,
 	},
 	{
@@ -207,15 +204,11 @@ define some-macro
     $(info foo)
 endef
 `,
-		expected: `# MK2RBC TRANSLATION ERROR: define is not supported: some-macro
+		expected: `load("//build/make/core:product_config.rbc", "rblf")
-# define  some-macro
-#     $(info foo)
-# endef
-load("//build/make/core:product_config.rbc", "rblf")
 
 def init(g, handle):
   cfg = rblf.cfg(handle)
-  rblf.warning("product.mk", "partially successful conversion")
+  rblf.mk2rbc_error("product.mk:2", "define is not supported: some-macro")
 `,
 	},
 	{
@@ -282,9 +275,7 @@ def init(g, handle):
     # Comment
     pass
   else:
-    # MK2RBC TRANSLATION ERROR: cannot set predefined variable TARGET_COPY_OUT_RECOVERY to "foo", its value should be "recovery"
+    rblf.mk2rbc_error("product.mk:5", "cannot set predefined variable TARGET_COPY_OUT_RECOVERY to \"foo\", its value should be \"recovery\"")
-    pass
-  rblf.warning("product.mk", "partially successful conversion")
 `,
 	},
 	{
@@ -859,9 +850,7 @@ def init(g, handle):
   rblf.soong_config_namespace(g, "cvd")
   rblf.soong_config_set(g, "cvd", "launch_configs", "cvd_config_auto.json")
   rblf.soong_config_append(g, "cvd", "grub_config", "grub.cfg")
-  # MK2RBC TRANSLATION ERROR: SOONG_CONFIG_ variables cannot be referenced, use soong_config_get instead: SOONG_CONFIG_cvd_grub_config
+  rblf.mk2rbc_error("product.mk:7", "SOONG_CONFIG_ variables cannot be referenced, use soong_config_get instead: SOONG_CONFIG_cvd_grub_config")
-  # x := $(SOONG_CONFIG_cvd_grub_config)
-  rblf.warning("product.mk", "partially successful conversion")
 `,
 	}, {
 		desc:   "soong namespace accesses",
@@ -1048,15 +1037,11 @@ def init(g, handle):
 		in: `
 foo: foo.c
 	gcc -o $@ $*`,
-		expected: `# MK2RBC TRANSLATION ERROR: unsupported line rule:       foo: foo.c
+		expected: `load("//build/make/core:product_config.rbc", "rblf")
-#gcc -o $@ $*
-# rule:       foo: foo.c
-# gcc -o $@ $*
-load("//build/make/core:product_config.rbc", "rblf")
 
 def init(g, handle):
   cfg = rblf.cfg(handle)
-  rblf.warning("product.mk", "partially successful conversion")
+  rblf.mk2rbc_error("product.mk:2", "unsupported line rule:       foo: foo.c\n#gcc -o $@ $*")
 `,
 	},
 	{
@@ -1064,14 +1049,27 @@ def init(g, handle):
 		mkname: "product.mk",
 		in: `
 override FOO:=`,
-		expected: `# MK2RBC TRANSLATION ERROR: cannot handle override directive
+		expected: `load("//build/make/core:product_config.rbc", "rblf")
-# override FOO :=
-load("//build/make/core:product_config.rbc", "rblf")
 
 def init(g, handle):
   cfg = rblf.cfg(handle)
+  rblf.mk2rbc_error("product.mk:2", "cannot handle override directive")
   g["override FOO"] = ""
-  rblf.warning("product.mk", "partially successful conversion")
+`,
+	},
+	{
+		desc:   "Bad expression",
+		mkname: "build/product.mk",
+		in: `
+ifeq (,$(call foobar))
+endif
+`,
+		expected: `load("//build/make/core:product_config.rbc", "rblf")
+
+def init(g, handle):
+  cfg = rblf.cfg(handle)
+  if rblf.mk2rbc_error("build/product.mk:2", "cannot handle invoking foobar"):
+    pass
 `,
 	},
 }
@@ -1146,7 +1144,6 @@ func TestGood(t *testing.T) {
 					Reader:         bytes.NewBufferString(test.in),
 					RootDir:        ".",
 					OutputSuffix:   ".star",
-					WarnPartialSuccess: true,
 					SourceFS:       fs,
 					MakefileFinder: &testMakefileFinder{fs: fs},
 				})

mk2rbc/node.go

@@ -183,6 +183,7 @@ type assignmentNode struct {
 	value    starlarkExpr
 	mkValue  *mkparser.MakeString
 	flavor   assignmentFlavor
+	location ErrorLocation
 	isTraced bool
 	previous *assignmentNode
 }
@@ -223,18 +224,6 @@ func (in *ifNode) emit(gctx *generationContext) {
 	}
 
 	gctx.newLine()
-	if bad, ok := in.expr.(*badExpr); ok {
-		gctx.write("# MK2STAR ERROR converting:")
-		gctx.newLine()
-		gctx.writef("#   %s", bad.node.Dump())
-		gctx.newLine()
-		gctx.writef("# %s", bad.message)
-		gctx.newLine()
-		// The init function emits a warning if the conversion was not
-		// fullly successful, so here we (arbitrarily) take the false path.
-		gctx.writef("%sFalse:", ifElif)
-		return
-	}
 	gctx.write(ifElif)
 	in.expr.emit(gctx)
 	gctx.write(":")

mk2rbc/variable.go

@@ -228,10 +228,9 @@ func (pv predefinedVariable) emitSet(gctx *generationContext, asgn *assignmentNo
 			if actualValue == expectedValue {
 				return
 			}
-			gctx.writef("# MK2RBC TRANSLATION ERROR: cannot set predefined variable %s to %q, its value should be %q",
+			gctx.emitConversionError(asgn.location,
-				pv.name(), actualValue, expectedValue)
+				fmt.Sprintf("cannot set predefined variable %s to %q, its value should be %q",
-			gctx.newLine()
+					pv.name(), actualValue, expectedValue))
-			gctx.write("pass")
 			gctx.starScript.hasErrors = true
 			return
 		}