Skip to content

Commit

Permalink
Fix detection of subexpressions in :S and :C modifiers
Browse files Browse the repository at this point in the history
  • Loading branch information
rillig committed Jan 5, 2025
1 parent f6c59d8 commit 0d286b1
Show file tree
Hide file tree
Showing 4 changed files with 243 additions and 57 deletions.
58 changes: 4 additions & 54 deletions v23/mkline.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 1 addition & 3 deletions v23/mklinechecker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
103 changes: 103 additions & 0 deletions v23/mkwalker.go
Original file line number Diff line number Diff line change
@@ -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())
}
135 changes: 135 additions & 0 deletions v23/mkwalker_test.go
Original file line number Diff line number Diff line change
@@ -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<from$<to<}",
".if ${COND:S<from$<to<}",
".endif")

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.walkDirective(mkline)
})

t.CheckOutputLines(
"NOTE: filename.mk:1: Expression \"${INFO:S<from$<to<}\" at \"load\" time.",
"NOTE: filename.mk:2: Expression \"${COND:S<from$<to<}\" at \"load\" time.")
}

func (s *Suite) Test_MkWalker_walkExpr(c *check.C) {
t := s.Init(c)

mklines := t.NewMkLines("filename.mk",
"${LEFT:S<from$<to<}=\t${RIGHT:S<from$<to<}")

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:1: Expression \"${LEFT:S<from$<to<}\" at \"load\" time.",
"NOTE: filename.mk:1: Expression \"${RIGHT:S<from$<to<}\" at \"run\" time.")
}

func (s *Suite) Test_MkWalker_walkModifier(c *check.C) {
t := s.Init(c)

mklines := t.NewMkLines("filename.mk",
"MOD.S=\t${VAR:S<from$<to<}",
"MOD.S=\t${VAR:S<${from}$<${to}<}",
)

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:1: Expression \"${VAR:S<from$<to<}\" at \"run\" time.",
"NOTE: filename.mk:2: Expression \"${VAR:S<${from}$<${to}<}\" at \"run\" time.",
"NOTE: filename.mk:2: Expression \"${from}\" at \"run\" time.",
"NOTE: filename.mk:2: Expression \"${to}\" at \"run\" time.")
}

0 comments on commit 0d286b1

Please sign in to comment.