From 0d286b10b6ff7a0ed7560df9a43a936b4346e2c6 Mon Sep 17 00:00:00 2001 From: Roland Illig Date: Sun, 5 Jan 2025 22:17:43 +0100 Subject: [PATCH] Fix detection of subexpressions in :S and :C modifiers --- v23/mkline.go | 58 ++-------------- v23/mklinechecker_test.go | 4 +- v23/mkwalker.go | 103 +++++++++++++++++++++++++++++ v23/mkwalker_test.go | 135 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 243 insertions(+), 57 deletions(-) create mode 100644 v23/mkwalker.go create mode 100644 v23/mkwalker_test.go diff --git a/v23/mkline.go b/v23/mkline.go index 2621dd39..b9ad413b 100644 --- a/v23/mkline.go +++ b/v23/mkline.go @@ -845,63 +845,13 @@ func (mkline *MkLine) VariableNeedsQuoting(mklines *MkLines, expr *MkExpr, varty // ForEachUsed calls the action for each variable that is used in the line. func (mkline *MkLine) ForEachUsed(action func(expr *MkExpr, time EctxTime)) { - switch { - - case mkline.IsVarassign(): - mkline.ForEachUsedText(mkline.Varname(), EctxLoadTime, action) - mkline.ForEachUsedText(mkline.Value(), mkline.Op().Time(), action) - - case mkline.IsDirective(): - mkline.forEachUsedDirective(action) - - case mkline.IsShellCommand(): - mkline.ForEachUsedText(mkline.ShellCommand(), EctxRunTime, action) - - case mkline.IsDependency(): - mkline.ForEachUsedText(mkline.Targets(), EctxLoadTime, action) - mkline.ForEachUsedText(mkline.Sources(), EctxLoadTime, action) - - case mkline.IsInclude(): - mkline.ForEachUsedText(mkline.IncludedFile().String(), EctxLoadTime, action) - } -} - -func (mkline *MkLine) forEachUsedDirective(action func(expr *MkExpr, time EctxTime)) { - switch mkline.Directive() { - case "error", "for", "info", "warning": - mkline.ForEachUsedText(mkline.Args(), EctxLoadTime, action) - case "if", "elif": - if cond := mkline.Cond(); cond != nil { - cond.Walk(&MkCondCallback{ - Expr: func(expr *MkExpr) { - mkline.ForEachUsedExpr(expr, EctxLoadTime, action) - }}) - } - } + walker := NewMkWalker(mkline, action) + walker.WalkLine(mkline) } func (mkline *MkLine) ForEachUsedText(text string, time EctxTime, action func(expr *MkExpr, time EctxTime)) { - if !contains(text, "$") { - return - } - - tokens, _ := NewMkLexer(text, nil).MkTokens() - for _, token := range tokens { - if token.Expr != nil { - mkline.ForEachUsedExpr(token.Expr, time, action) - } - } -} - -func (mkline *MkLine) ForEachUsedExpr(expr *MkExpr, time EctxTime, action func(expr *MkExpr, time EctxTime)) { - varname := expr.varname - if !expr.IsExpression() { - action(expr, time) - } - mkline.ForEachUsedText(varname, time, action) - for _, mod := range expr.modifiers { - mkline.ForEachUsedText(mod.String(), time, action) - } + walker := NewMkWalker(mkline, action) + walker.WalkText(text, time) } // UnquoteShell removes one level of double and single quotes, diff --git a/v23/mklinechecker_test.go b/v23/mklinechecker_test.go index 3d556217..86c60080 100644 --- a/v23/mklinechecker_test.go +++ b/v23/mklinechecker_test.go @@ -1114,9 +1114,7 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveFor(c *check.C) { "WARN: for.mk:11: Use ${PATH:Q} instead of ${PATH}.", // TODO: Why not? The variable is guaranteed to be defined at this point. - "WARN: for.mk:15: DISTFILES should not be used at load time in any file.", - // FIXME: Do not treat "$/" as an expression. - "WARN: for.mk:15: Variable \"/\" is used but not defined.") + "WARN: for.mk:15: DISTFILES should not be used at load time in any file.") } func (s *Suite) Test_MkLineChecker_checkDirectiveFor__continuation(c *check.C) { diff --git a/v23/mkwalker.go b/v23/mkwalker.go new file mode 100644 index 00000000..3d19b56c --- /dev/null +++ b/v23/mkwalker.go @@ -0,0 +1,103 @@ +package pkglint + +// MkWalker walks through a makefile line or a text snippet from such a line, +// visiting the expressions and their subexpressions. +type MkWalker struct { + Diag Autofixer + Expr func(expr *MkExpr, time EctxTime) +} + +func NewMkWalker(diag Autofixer, expr func(expr *MkExpr, time EctxTime)) *MkWalker { + return &MkWalker{diag, expr} +} + +// WalkLine calls the action for each variable that is used in the line. +func (w *MkWalker) WalkLine(mkline *MkLine) { + switch { + + case mkline.IsVarassign(): + w.WalkText(mkline.Varname(), EctxLoadTime) + w.WalkText(mkline.Value(), mkline.Op().Time()) + + case mkline.IsDirective(): + w.walkDirective(mkline) + + case mkline.IsShellCommand(): + w.WalkText(mkline.ShellCommand(), EctxRunTime) + + case mkline.IsDependency(): + w.WalkText(mkline.Targets(), EctxLoadTime) + w.WalkText(mkline.Sources(), EctxLoadTime) + + case mkline.IsInclude(): + w.WalkText(mkline.IncludedFile().String(), EctxLoadTime) + } +} + +func (w *MkWalker) WalkText(text string, time EctxTime) { + if !contains(text, "$") { + return + } + + tokens, _ := NewMkLexer(text, nil).MkTokens() + for _, token := range tokens { + if token.Expr != nil { + w.walkExpr(token.Expr, time) + } + } +} + +func (w *MkWalker) walkDirective(mkline *MkLine) { + switch mkline.Directive() { + case "error", "for", "info", "warning": + w.WalkText(mkline.Args(), EctxLoadTime) + case "if", "elif": + if cond := mkline.Cond(); cond != nil { + cond.Walk(&MkCondCallback{ + Expr: func(expr *MkExpr) { + w.walkExpr(expr, EctxLoadTime) + }}) + } + } +} + +func (w *MkWalker) walkExpr(expr *MkExpr, time EctxTime) { + varname := expr.varname + if !expr.IsExpression() { + w.Expr(expr, time) + } + w.WalkText(varname, time) + for _, mod := range expr.modifiers { + w.walkModifier(mod, time) + } +} + +func (w *MkWalker) walkModifier(mod MkExprModifier, time EctxTime) { + if !contains(mod.String(), "$") { + return + } + if mod.HasPrefix("@") { + // XXX: Probably close enough for most practical cases. + // If not, implement bmake's ParseModifierPartBalanced. + w.WalkText(mod.String(), time) + return + } + if mod.HasPrefix("?") || mod.HasPrefix("D") || mod.HasPrefix("U") || mod.HasPrefix("!") { + // XXX: Probably close enough, but \$ and $$ differ. + w.WalkText(mod.String(), time) + return + } + if ok, _, from, to, _ := mod.MatchSubst(); ok { + // XXX: Probably close enough, but \$ and $$ differ. + w.WalkText(from, time) + // XXX: Probably close enough, but \$ and $$ differ. + w.WalkText(to, time) + return + } + if ok, _, pattern, _ := mod.MatchMatch(); ok { + // XXX: Probably close enough, but \$ and $$ differ. + w.WalkText(pattern, time) + return + } + w.Diag.Errorf("Modifier \"%s\" not implemented.", mod.String()) +} diff --git a/v23/mkwalker_test.go b/v23/mkwalker_test.go new file mode 100644 index 00000000..54aa7d56 --- /dev/null +++ b/v23/mkwalker_test.go @@ -0,0 +1,135 @@ +package pkglint + +import "gopkg.in/check.v1" + +func (s *Suite) Test_NewMkWalker(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, "LEFT.$p=\t${RIGHT}") + action := func(expr *MkExpr, time EctxTime) { + mkline.Notef("Expression \"%s\" at \"%s\" time.", expr.String(), time.String()) + } + + walker := NewMkWalker(mkline, action) + walker.WalkLine(mkline) + + t.CheckOutputLines( + "WARN: filename.mk:123: $p is ambiguous. Use ${p} if you mean a Make variable or $$p if you mean a shell variable.", + "NOTE: filename.mk:123: Expression \"${p}\" at \"load\" time.", + "NOTE: filename.mk:123: Expression \"${RIGHT}\" at \"run\" time.") +} + +func (s *Suite) Test_MkWalker_WalkLine(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("filename.mk", + MkCvsID, + "LEFT.$p=\t${RIGHT}", + ".for i in ${LEFT.$p}", + ".if ${left}", + "\t${COMMAND}", + "${TARGET}: ${SOURCE}", + ".include \"${DIR}/Makefile\"") + + mklines.ForEach(func(mkline *MkLine) { + action := func(expr *MkExpr, time EctxTime) { + mkline.Notef("Expression \"%s\" at \"%s\" time.", expr.String(), time.String()) + } + walker := NewMkWalker(mkline, action) + walker.WalkLine(mkline) + }) + + t.CheckOutputLines( + "WARN: filename.mk:2: $p is ambiguous. Use ${p} if you mean a Make variable or $$p if you mean a shell variable.", + "NOTE: filename.mk:2: Expression \"${p}\" at \"load\" time.", + "NOTE: filename.mk:2: Expression \"${RIGHT}\" at \"run\" time.", + "NOTE: filename.mk:3: Expression \"${LEFT.$p}\" at \"load\" time.", + "NOTE: filename.mk:3: Expression \"${p}\" at \"load\" time.", + "NOTE: filename.mk:4: Expression \"${left}\" at \"load\" time.", + "NOTE: filename.mk:5: Expression \"${COMMAND}\" at \"run\" time.", + "NOTE: filename.mk:6: Expression \"${TARGET}\" at \"load\" time.", + "NOTE: filename.mk:6: Expression \"${SOURCE}\" at \"load\" time.", + "NOTE: filename.mk:7: Expression \"${DIR}\" at \"load\" time.") +} + +func (s *Suite) Test_MkWalker_WalkText(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("filename.mk", + MkCvsID, + "\t${COMMAND}") + + mklines.ForEach(func(mkline *MkLine) { + action := func(expr *MkExpr, time EctxTime) { + mkline.Notef("Expression \"%s\" at \"%s\" time.", expr.String(), time.String()) + } + walker := NewMkWalker(mkline, action) + walker.WalkLine(mkline) + }) + + t.CheckOutputLines( + "NOTE: filename.mk:2: Expression \"${COMMAND}\" at \"run\" time.") +} + +func (s *Suite) Test_MkWalker_walkDirective(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("filename.mk", + ".info ${INFO:S