Skip to content

Commit

Permalink
Don't suggest to match list expressions using '=='
Browse files Browse the repository at this point in the history
Noticed by Jonathan Perkin in devel/ncurses, where pkglint wrongly
suggested replacing ${USE_NCURSES:Mwide} with ${USE_NCURSES} == wide.
  • Loading branch information
rillig committed May 7, 2024
1 parent 829f775 commit 7fef78f
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 6 deletions.
6 changes: 5 additions & 1 deletion v23/mkcondchecker.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ func (ck *MkCondChecker) checkEmpty(expr *MkExpr, fromEmpty bool, neg bool) {
s.SimplifyExpr(expr, fromEmpty, neg)
}

// checkEmptyExpr warns about 'empty(${VARNAME:Mpattern})', which should be
// 'empty(VARNAME:Mpattern)' instead, without the '${...}'.
func (ck *MkCondChecker) checkEmptyExpr(expr *MkExpr, neg bool) {
if !matches(expr.varname, `^\$.*:[MN]`) {
return
Expand All @@ -158,7 +160,7 @@ func (ck *MkCondChecker) checkEmptyExpr(expr *MkExpr, neg bool) {
"you should write either of the following:",
"",
"\t!empty(VARNAME:Mpattern)",
"\t${VARNAME:Mpattern} != \"\"",
"\t${VARNAME:U:Mpattern} != \"\"",
"",
"If the pattern cannot match the number zero,",
"you can omit the '!= \"\"', resulting in:",
Expand All @@ -180,6 +182,8 @@ func (ck *MkCondChecker) checkEmptyExpr(expr *MkExpr, neg bool) {
}
}

// checkEmptyType warns if there is a ':M' modifier in which the pattern
// doesn't match the type of the expression.
func (ck *MkCondChecker) checkEmptyType(expr *MkExpr) {
for _, modifier := range expr.modifiers {
ok, _, pattern, _ := modifier.MatchMatch()
Expand Down
5 changes: 4 additions & 1 deletion v23/mkcondsimplifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ func (s *MkCondSimplifier) SimplifyExpr(expr *MkExpr, fromEmpty bool, neg bool)
return
}
s.simplifyMatch(expr, fromEmpty, neg)
s.simplifyWord(expr, fromEmpty, neg)
vartype := G.Pkgsrc.VariableType(s.MkLines, expr.varname)
if vartype != nil && !vartype.IsList() && vartype.basicType != BtUnknown {
s.simplifyWord(expr, fromEmpty, neg)
}
}

// simplifyWord simplifies a condition like '${VAR:Mword}' in the common case
Expand Down
4 changes: 0 additions & 4 deletions v23/mkcondsimplifier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -834,10 +834,6 @@ func (s *Suite) Test_MkCondSimplifier_simplifyWord__list_and_unknown(c *check.C)
"WARN: filename.mk:10: LIST_OF_WORDS should be matched "+
"against \"[yY][eE][sS]\" or \"[nN][oO]\", "+
"not \"yes\".",
// FIXME: UNKNOWN could be a list as well.
"NOTE: filename.mk:12: UNKNOWN can be compared "+
"using the simpler \"${UNKNOWN} == yes\" "+
"instead of matching against \":Myes\".",
)
}

Expand Down
3 changes: 3 additions & 0 deletions v23/vartype.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,9 @@ func (perms ACLPermissions) HumanString() string {
condStr(perms.Contains(aclpUse), "used", ""))
}

// IsList returns whether the type is a list of something.
// A return value of false doesn't mean a single-word type,
// it could also be BtUnknown.
func (vt *Vartype) IsList() bool { return vt.options&List != 0 }
func (vt *Vartype) IsGuessed() bool { return vt.options&Guessed != 0 }
func (vt *Vartype) IsPackageSettable() bool { return vt.options&PackageSettable != 0 }
Expand Down

0 comments on commit 7fef78f

Please sign in to comment.