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
diff --git a/mk2rbc/cmd/mk2rbc.go b/mk2rbc/cmd/mk2rbc.go
index 7bb0246..bb5a680 100644
--- a/mk2rbc/cmd/mk2rbc.go
+++ b/mk2rbc/cmd/mk2rbc.go
@@ -46,8 +46,6 @@
 	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 @@
 	flagAlias("root", "d")
 	flagAlias("dry_run", "n")
 	flagAlias("convert_dependents", "r")
-	flagAlias("no_warnings", "w")
 	flagAlias("error_stat", "e")
 }
 
@@ -207,9 +204,9 @@
 		}
 	}
 
-	printStats()
 	if *errstat {
 		errorLogger.printStatistics()
+		printStats()
 	}
 	if !ok {
 		os.Exit(1)
@@ -329,17 +326,16 @@
 	}()
 
 	mk2starRequest := mk2rbc.Request{
-		MkFile:             mkFile,
-		Reader:             nil,
-		RootDir:            *rootDir,
-		OutputDir:          *outputTop,
-		OutputSuffix:       *suffix,
-		TracedVariables:    tracedVariables,
-		TraceCalls:         *traceCalls,
-		WarnPartialSuccess: !*noWarn,
-		SourceFS:           os.DirFS(*rootDir),
-		MakefileFinder:     makefileFinder,
-		ErrorLogger:        errorLogger,
+		MkFile:          mkFile,
+		Reader:          nil,
+		RootDir:         *rootDir,
+		OutputDir:       *outputTop,
+		OutputSuffix:    *suffix,
+		TracedVariables: tracedVariables,
+		TraceCalls:      *traceCalls,
+		SourceFS:        os.DirFS(*rootDir),
+		MakefileFinder:  makefileFinder,
+		ErrorLogger:     errorLogger,
 	}
 	ss, err := mk2rbc.Convert(mk2starRequest)
 	if err != nil {
@@ -417,9 +413,6 @@
 
 func printStats() {
 	var sortedFiles []string
-	if *noWarn && !*verbose {
-		return
-	}
 	for p := range converted {
 		sortedFiles = append(sortedFiles, p)
 	}
@@ -435,27 +428,22 @@
 			nOk++
 		}
 	}
-	if !*noWarn {
-		if nPartial > 0 {
-			fmt.Fprintf(os.Stderr, "Conversion was partially successful for:\n")
-			for _, f := range sortedFiles {
-				if ss := converted[f]; ss != nil && ss.HasErrors() {
-					fmt.Fprintln(os.Stderr, "  ", f)
-				}
-			}
-		}
-
-		if nFailed > 0 {
-			fmt.Fprintf(os.Stderr, "Conversion failed for files:\n")
-			for _, f := range sortedFiles {
-				if converted[f] == nil {
-					fmt.Fprintln(os.Stderr, "  ", f)
-				}
+	if nPartial > 0 {
+		fmt.Fprintf(os.Stderr, "Conversion was partially successful for:\n")
+		for _, f := range sortedFiles {
+			if ss := converted[f]; ss != nil && ss.HasErrors() {
+				fmt.Fprintln(os.Stderr, "  ", f)
 			}
 		}
 	}
-	if *verbose && (nPartial > 0 || nFailed > 0) {
-		fmt.Fprintln(os.Stderr, "Succeeded: ", nOk, " Partial: ", nPartial, " Failed: ", nFailed)
+
+	if nFailed > 0 {
+		fmt.Fprintf(os.Stderr, "Conversion failed for files:\n")
+		for _, f := range sortedFiles {
+			if converted[f] == nil {
+				fmt.Fprintln(os.Stderr, "  ", f)
+			}
+		}
 	}
 }
 
@@ -468,8 +456,8 @@
 	data map[string]datum
 }
 
-func (ebt errorSink) NewError(sourceFile string, sourceLine int, node parser.Node, message string, args ...interface{}) {
-	fmt.Fprintf(os.Stderr, "%s:%d ", sourceFile, sourceLine)
+func (ebt errorSink) NewError(el mk2rbc.ErrorLocation, node parser.Node, message string, args ...interface{}) {
+	fmt.Fprint(os.Stderr, el, ": ")
 	fmt.Fprintf(os.Stderr, message, args...)
 	fmt.Fprintln(os.Stderr)
 	if !*errstat {
diff --git a/mk2rbc/expr.go b/mk2rbc/expr.go
index 8279d2e..65fd6f7 100644
--- a/mk2rbc/expr.go
+++ b/mk2rbc/expr.go
@@ -18,8 +18,6 @@
 	"fmt"
 	"strconv"
 	"strings"
-
-	mkparser "android/soong/androidmk/parser"
 )
 
 // Represents an expression in the Starlark code. An expression has
@@ -106,7 +104,7 @@
 		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 @@
 		}
 	}
 	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 @@
 	return &v
 }
 
-// concatExpr generates epxr1 + expr2 + ... + exprN in Starlark.
+// concatExpr generates expr1 + expr2 + ... + exprN in Starlark.
 type concatExpr struct {
 	items []starlarkExpr
 }
@@ -607,8 +605,8 @@
 }
 
 type badExpr struct {
-	node    mkparser.Node
-	message string
+	errorLocation ErrorLocation
+	message       string
 }
 
 func (b *badExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) {
@@ -617,15 +615,15 @@
 	return
 }
 
-func (b *badExpr) emit(_ *generationContext) {
-	panic("implement me")
+func (b *badExpr) emit(gctx *generationContext) {
+	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")
 }
 
diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go
index ee6cf79..c55300e 100644
--- a/mk2rbc/mk2rbc.go
+++ b/mk2rbc/mk2rbc.go
@@ -157,23 +157,31 @@
 
 // Conversion request parameters
 type Request struct {
-	MkFile             string    // file to convert
-	Reader             io.Reader // if set, read input from this stream instead
-	RootDir            string    // root directory path used to resolve included files
-	OutputSuffix       string    // generated Starlark files suffix
-	OutputDir          string    // if set, root of the output hierarchy
-	ErrorLogger        ErrorLogger
-	TracedVariables    []string // trace assignment to these variables
-	TraceCalls         bool
-	WarnPartialSuccess bool
-	SourceFS           fs.FS
-	MakefileFinder     MakefileFinder
+	MkFile          string    // file to convert
+	Reader          io.Reader // if set, read input from this stream instead
+	RootDir         string    // root directory path used to resolve included files
+	OutputSuffix    string    // generated Starlark files suffix
+	OutputDir       string    // if set, root of the output hierarchy
+	ErrorLogger     ErrorLogger
+	TracedVariables []string // trace assignment to these variables
+	TraceCalls      bool
+	SourceFS        fs.FS
+	MakefileFinder  MakefileFinder
 }
 
 // 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 @@
 		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 @@
 	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
@@ -371,18 +379,17 @@
 
 // Information about the generated Starlark script.
 type StarlarkScript struct {
-	mkFile             string
-	moduleName         string
-	mkPos              scanner.Position
-	nodes              []starlarkNode
-	inherited          []*moduleInfo
-	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
+	mkFile         string
+	moduleName     string
+	mkPos          scanner.Position
+	nodes          []starlarkNode
+	inherited      []*moduleInfo
+	hasErrors      bool
+	topDir         string
+	traceCalls     bool // print enter/exit each init function
+	sourceFS       fs.FS
+	makefileFinder MakefileFinder
+	nodeLocator    func(pos mkparser.Pos) int
 }
 
 func (ss *StarlarkScript) newNode(node starlarkNode) {
@@ -561,7 +568,7 @@
 		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) 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 @@
 // 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.insertComment(fmt.Sprintf("# MK2RBC TRANSLATION ERROR: %s", message))
-	ctx.carryAsComment(failedNode)
-
+	ctx.receiver.newNode(&exprNode{ctx.newBadExpr(failedNode, message, args...)})
 	ctx.script.hasErrors = true
 }
 
 func (ctx *parseContext) wrapBadExpr(xBad *badExpr) {
-	ctx.insertComment(fmt.Sprintf("# MK2RBC TRANSLATION ERROR: %s", xBad.message))
-	ctx.carryAsComment(xBad.node)
+	ctx.receiver.newNode(&exprNode{xBad})
 }
 
 func (ctx *parseContext) loadedModulePath(path string) string {
@@ -1622,6 +1625,10 @@
 	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()
 }
@@ -1660,14 +1667,13 @@
 		return nil, fmt.Errorf("bad makefile %s", req.MkFile)
 	}
 	starScript := &StarlarkScript{
-		moduleName:         moduleNameForFile(req.MkFile),
-		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 },
+		moduleName:     moduleNameForFile(req.MkFile),
+		mkFile:         req.MkFile,
+		topDir:         req.RootDir,
+		traceCalls:     req.TraceCalls,
+		sourceFS:       req.SourceFS,
+		makefileFinder: req.MakefileFinder,
+		nodeLocator:    func(pos mkparser.Pos) int { return parser.Unpack(pos).Line },
 	}
 	ctx := newParseContext(starScript, nodes)
 	ctx.outputSuffix = req.OutputSuffix
diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go
index 41c32d1..22490a2 100644
--- a/mk2rbc/mk2rbc_test.go
+++ b/mk2rbc/mk2rbc_test.go
@@ -105,15 +105,12 @@
 PRODUCT_NAME := $(call foo1, bar)
 PRODUCT_NAME := $(call foo0)
 `,
-		expected: `# MK2RBC TRANSLATION ERROR: cannot handle invoking foo1
-# PRODUCT_NAME := $(call foo1, bar)
-# MK2RBC TRANSLATION ERROR: cannot handle invoking foo0
-# PRODUCT_NAME := $(call foo0)
-load("//build/make/core:product_config.rbc", "rblf")
+		expected: `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 @@
     $(info foo)
 endef
 `,
-		expected: `# MK2RBC TRANSLATION ERROR: define is not supported: some-macro
-# define  some-macro
-#     $(info foo)
-# endef
-load("//build/make/core:product_config.rbc", "rblf")
+		expected: `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 @@
     # Comment
     pass
   else:
-    # MK2RBC TRANSLATION ERROR: cannot set predefined variable TARGET_COPY_OUT_RECOVERY to "foo", its value should be "recovery"
-    pass
-  rblf.warning("product.mk", "partially successful conversion")
+    rblf.mk2rbc_error("product.mk:5", "cannot set predefined variable TARGET_COPY_OUT_RECOVERY to \"foo\", its value should be \"recovery\"")
 `,
 	},
 	{
@@ -859,9 +850,7 @@
   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
-  # x := $(SOONG_CONFIG_cvd_grub_config)
-  rblf.warning("product.mk", "partially successful conversion")
+  rblf.mk2rbc_error("product.mk:7", "SOONG_CONFIG_ variables cannot be referenced, use soong_config_get instead: SOONG_CONFIG_cvd_grub_config")
 `,
 	}, {
 		desc:   "soong namespace accesses",
@@ -1048,15 +1037,11 @@
 		in: `
 foo: foo.c
 	gcc -o $@ $*`,
-		expected: `# MK2RBC TRANSLATION ERROR: unsupported line rule:       foo: foo.c
-#gcc -o $@ $*
-# rule:       foo: foo.c
-# gcc -o $@ $*
-load("//build/make/core:product_config.rbc", "rblf")
+		expected: `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 @@
 		mkname: "product.mk",
 		in: `
 override FOO:=`,
-		expected: `# MK2RBC TRANSLATION ERROR: cannot handle override directive
-# override FOO :=
-load("//build/make/core:product_config.rbc", "rblf")
+		expected: `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
 `,
 	},
 }
@@ -1142,13 +1140,12 @@
 		t.Run(test.desc,
 			func(t *testing.T) {
 				ss, err := Convert(Request{
-					MkFile:             test.mkname,
-					Reader:             bytes.NewBufferString(test.in),
-					RootDir:            ".",
-					OutputSuffix:       ".star",
-					WarnPartialSuccess: true,
-					SourceFS:           fs,
-					MakefileFinder:     &testMakefileFinder{fs: fs},
+					MkFile:         test.mkname,
+					Reader:         bytes.NewBufferString(test.in),
+					RootDir:        ".",
+					OutputSuffix:   ".star",
+					SourceFS:       fs,
+					MakefileFinder: &testMakefileFinder{fs: fs},
 				})
 				if err != nil {
 					t.Error(err)
diff --git a/mk2rbc/node.go b/mk2rbc/node.go
index 3fe1a17..d38299d 100644
--- a/mk2rbc/node.go
+++ b/mk2rbc/node.go
@@ -183,6 +183,7 @@
 	value    starlarkExpr
 	mkValue  *mkparser.MakeString
 	flavor   assignmentFlavor
+	location ErrorLocation
 	isTraced bool
 	previous *assignmentNode
 }
@@ -223,18 +224,6 @@
 	}
 
 	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(":")
diff --git a/mk2rbc/variable.go b/mk2rbc/variable.go
index ded07fe..6b67a7c 100644
--- a/mk2rbc/variable.go
+++ b/mk2rbc/variable.go
@@ -228,10 +228,9 @@
 			if actualValue == expectedValue {
 				return
 			}
-			gctx.writef("# MK2RBC TRANSLATION ERROR: cannot set predefined variable %s to %q, its value should be %q",
-				pv.name(), actualValue, expectedValue)
-			gctx.newLine()
-			gctx.write("pass")
+			gctx.emitConversionError(asgn.location,
+				fmt.Sprintf("cannot set predefined variable %s to %q, its value should be %q",
+					pv.name(), actualValue, expectedValue))
 			gctx.starScript.hasErrors = true
 			return
 		}