summaryrefslogtreecommitdiff
path: root/pkgtools/pkglint/files/mkvarusechecker.go
blob: bb4f9700ed62522653fa2b4e194de2e80bf9cb66 (plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
package pkglint

import "strings"

type MkVarUseChecker struct {
	use     *MkVarUse
	vartype *Vartype

	MkLines *MkLines
	MkLine  *MkLine
}

func NewMkVarUseChecker(use *MkVarUse, mklines *MkLines, mkline *MkLine) *MkVarUseChecker {
	vartype := G.Pkgsrc.VariableType(mklines, use.varname)

	return &MkVarUseChecker{use, vartype, mklines, mkline}
}

// CheckVaruse checks a single use of a variable in a specific context.
func (ck *MkVarUseChecker) Check(vuc *VarUseContext) {
	if ck.use.IsExpression() {
		return
	}

	ck.checkUndefined()
	ck.checkPermissions(vuc)

	ck.checkVarname(vuc.time)
	ck.checkModifiers()
	ck.checkQuoting(vuc)

	ck.checkBuildDefs()
	ck.checkDeprecated()

	NewMkLineChecker(ck.MkLines, ck.MkLine).
		checkTextVarUse(ck.use.varname, ck.vartype, vuc.time)
}

func (ck *MkVarUseChecker) checkUndefined() {
	varuse := ck.use
	vartype := ck.vartype
	varname := varuse.varname

	switch {
	case !G.Opts.WarnExtra,
		// Well-known variables are probably defined by the infrastructure.
		vartype != nil && !vartype.IsGuessed(),
		// TODO: At load time, check ck.MkLines.loadVars instead of allVars.
		ck.MkLines.allVars.IsDefinedSimilar(varname),
		ck.MkLines.forVars[varname],
		ck.MkLines.allVars.Mentioned(varname) != nil,
		G.Pkg != nil && G.Pkg.vars.IsDefinedSimilar(varname),
		containsVarRef(varname),
		G.Pkgsrc.vartypes.IsDefinedCanon(varname),
		varname == "":
		return
	}

	if ck.MkLines.once.FirstTimeSlice("used but not defined", varname) {
		ck.MkLine.Warnf("%s is used but not defined.", varname)
	}
}

func (ck *MkVarUseChecker) checkModifiers() {
	if len(ck.use.modifiers) == 0 {
		return
	}

	ck.checkModifiersSuffix()
	ck.checkModifiersRange()

	// TODO: Add checks for a single modifier, among them:
	// TODO: Suggest to replace ${VAR:@l@-l${l}@} with the simpler ${VAR:S,^,-l,}.
	// TODO: Suggest to replace ${VAR:@l@${l}suffix@} with the simpler ${VAR:=suffix}.
	// TODO: Investigate why :Q is not checked at this exact place.
}

func (ck *MkVarUseChecker) checkModifiersSuffix() {
	varuse := ck.use
	vartype := ck.vartype

	if !varuse.modifiers[0].IsSuffixSubst() || vartype == nil || vartype.IsList() {
		return
	}

	ck.MkLine.Warnf("The :from=to modifier should only be used with lists, not with %s.", varuse.varname)
	ck.MkLine.Explain(
		"Instead of (for example):",
		"\tMASTER_SITES=\t${HOMEPAGE:=repository/}",
		"",
		"Write:",
		"\tMASTER_SITES=\t${HOMEPAGE}repository/",
		"",
		"This expresses the intention of the code more clearly.")
}

// checkModifiersRange suggests to replace
// ${VAR:S,^,__magic__,1:M__magic__*:S,^__magic__,,} with the simpler ${VAR:[1]}.
func (ck *MkVarUseChecker) checkModifiersRange() {
	varuse := ck.use
	mods := varuse.modifiers

	if len(mods) != 3 {
		return
	}

	m, _, from, to, options := mods[0].MatchSubst()
	if !m || from != "^" || !matches(to, `^\w+$`) || options != "1" {
		return
	}

	magic := to
	m, positive, pattern, _ := mods[1].MatchMatch()
	if !m || !positive || pattern != magic+"*" {
		return
	}

	m, _, from, to, options = mods[2].MatchSubst()
	if !m || from != "^"+magic || to != "" || options != "" {
		return
	}

	fix := ck.MkLine.Autofix()
	fix.Notef("The modifier %q can be written as %q.", varuse.Mod(), ":[1]")
	fix.Explain(
		"The range modifier is much easier to understand than the",
		"complicated regular expressions, which were needed before",
		"the year 2006.")
	fix.Replace(varuse.Mod(), ":[1]")
	fix.Apply()
}

func (ck *MkVarUseChecker) checkVarname(time VucTime) {
	varname := ck.use.varname
	if varname == "@" {
		ck.MkLine.Warnf("Please use %q instead of %q.", "${.TARGET}", "$@")
		ck.MkLine.Explain(
			"It is more readable and prevents confusion with the shell variable",
			"of the same name.")
	}

	if varname == "LOCALBASE" && !G.Infrastructure && time == VucRunTime {
		fix := ck.MkLine.Autofix()
		fix.Warnf("Please use PREFIX instead of LOCALBASE.")
		fix.ReplaceRegex(`\$\{LOCALBASE\b`, "${PREFIX", 1)
		fix.Apply()
	}
}

// checkPermissions checks the permissions when a variable is used,
// be it in a variable assignment, in a shell command, a conditional, or
// somewhere else.
//
// See checkVarassignLeftPermissions.
func (ck *MkVarUseChecker) checkPermissions(vuc *VarUseContext) {
	if !G.Opts.WarnPerm {
		return
	}
	if G.Infrastructure {
		// As long as vardefs.go doesn't explicitly define permissions for
		// infrastructure files, skip the check completely. This avoids
		// many wrong warnings.
		return
	}

	if trace.Tracing {
		defer trace.Call(vuc)()
	}

	// This is the type of the variable that is being used. Not to
	// be confused with vuc.vartype, which is the type of the
	// context in which the variable is used (often a ShellCommand
	// or, in an assignment, the type of the left hand side variable).
	varname := ck.use.varname
	vartype := ck.vartype
	if vartype == nil {
		if trace.Tracing {
			trace.Step1("No type definition found for %q.", varname)
		}
		return
	}

	if vartype.IsGuessed() {
		return
	}

	// Do not warn about unknown infrastructure variables.
	// These have all permissions to prevent warnings when they are used.
	// But when other variables are assigned to them it would seem as if
	// these other variables could become evaluated at load time.
	// And this is something that most variables do not allow.
	if vuc.vartype != nil && vuc.vartype.basicType == BtUnknown {
		return
	}

	basename := ck.MkLine.Basename
	if basename == "hacks.mk" {
		return
	}

	effPerms := vartype.EffectivePermissions(basename)
	if effPerms.Contains(aclpUseLoadtime) {
		ck.checkUseAtLoadTime(vuc.time)

		// Since the variable may be used at load time, it probably
		// may be used at run time as well. If it weren't, that would
		// be a rather strange permissions set.
		return
	}

	// At this point the variable must not be used at load time.
	// Now determine whether it is directly used at load time because
	// the context already says so or, a little trickier, if it might
	// be used at load time somewhere in the future because it is
	// assigned to another variable, and that variable is allowed
	// to be used at load time.
	directly := vuc.time == VucLoadTime
	indirectly := !directly && vuc.vartype != nil &&
		vuc.vartype.Union().Contains(aclpUseLoadtime)

	if !directly && !indirectly && effPerms.Contains(aclpUse) {
		// At this point the variable is either used at run time, or the
		// time is not known.
		return
	}

	if directly || indirectly {
		// At this point the variable is used at load time although that
		// is not allowed by the permissions. The variable could be a tool
		// variable, and these tool variables have special rules.
		tool := G.ToolByVarname(ck.MkLines, varname)
		if tool != nil {

			// Whether a tool variable may be used at load time depends on
			// whether bsd.prefs.mk has been included before. That file
			// examines the tools that have been added to USE_TOOLS up to
			// this point and makes their variables available for use at
			// load time.
			if !tool.UsableAtLoadTime(ck.MkLines.Tools.SeenPrefs) {
				ck.warnToolLoadTime(varname, tool)
			}
			return
		}
	}

	if ck.MkLines.once.FirstTimeSlice("checkPermissions", varname) {
		ck.warnPermissions(vuc.vartype, varname, vartype, directly, indirectly)
	}
}

func (ck *MkVarUseChecker) warnPermissions(
	vucVartype *Vartype, varname string, vartype *Vartype, directly, indirectly bool) {

	mkline := ck.MkLine

	anyPerms := vartype.Union()
	if !anyPerms.Contains(aclpUse) && !anyPerms.Contains(aclpUseLoadtime) {
		mkline.Warnf("%s should not be used in any file; it is a write-only variable.", varname)
		ck.explainPermissions(varname, vartype)
		return
	}

	if indirectly {
		// Some of the guessed variables may be used at load time. But since the
		// variable type and these permissions are guessed, pkglint should not
		// issue the following warning, since it is often wrong.
		if vucVartype.IsGuessed() {
			return
		}

		mkline.Warnf("%s should not be used indirectly at load time (via %s).",
			varname, mkline.Varname())
		ck.explainPermissions(varname, vartype,
			"The variable on the left-hand side may be evaluated at load time,",
			"but the variable on the right-hand side should not.",
			"Because of the assignment in this line, the variable might be",
			"used indirectly at load time, before it is guaranteed to be",
			"properly initialized.")
		return
	}

	needed := aclpUse
	if directly {
		needed = aclpUseLoadtime
	}
	alternativeFiles := vartype.AlternativeFiles(needed)

	loadTimeExplanation := func() []string {
		return []string{
			"Many variables, especially lists of something, get their values incrementally.",
			"Therefore it is generally unsafe to rely on their",
			"value until it is clear that it will never change again.",
			"This point is reached when the whole package Makefile is loaded and",
			"execution of the shell commands starts; in some cases earlier.",
			"",
			"Additionally, when using the \":=\" operator, each $$ is replaced",
			"with a single $, so variables that have references to shell",
			"variables or regular expressions are modified in a subtle way."}
	}

	switch {
	case alternativeFiles == "" && directly:
		mkline.Warnf("%s should not be used at load time in any file.", varname)
		ck.explainPermissions(varname, vartype, loadTimeExplanation()...)

	case alternativeFiles == "":
		mkline.Warnf("%s should not be used in any file.", varname)
		ck.explainPermissions(varname, vartype, loadTimeExplanation()...)

	case directly:
		mkline.Warnf(
			"%s should not be used at load time in this file; "+
				"it would be ok in %s.",
			varname, alternativeFiles)
		ck.explainPermissions(varname, vartype, loadTimeExplanation()...)

	default:
		mkline.Warnf(
			"%s should not be used in this file; it would be ok in %s.",
			varname, alternativeFiles)
		ck.explainPermissions(varname, vartype)
	}
}

func (ck *MkVarUseChecker) explainPermissions(varname string, vartype *Vartype, intro ...string) {
	if !G.Logger.Opts.Explain {
		return
	}

	// TODO: Starting with the second explanation, omit the common part. Instead, only list the permission rules.

	var expl []string

	if len(intro) > 0 {
		expl = append(expl, intro...)
		expl = append(expl, "")
	}

	expl = append(expl,
		"The allowed actions for a variable are determined based on the file",
		"name in which the variable is used or defined.",
		sprintf("The rules for %s are:", varname),
		"")

	for _, rule := range vartype.aclEntries {
		perms := rule.permissions.HumanString()

		files := rule.matcher.originalPattern
		if files == "*" {
			files = "any file"
		}

		if perms != "" {
			expl = append(expl, sprintf("* in %s, it may be %s", files, perms))
		} else {
			expl = append(expl, sprintf("* in %s, it should not be accessed at all", files))
		}
	}

	expl = append(expl,
		"",
		"If these rules seem to be incorrect, please ask on the tech-pkg@NetBSD.org mailing list.")

	ck.MkLine.Explain(expl...)
}

func (ck *MkVarUseChecker) checkUseAtLoadTime(time VucTime) {
	if time != VucLoadTime {
		return
	}
	if ck.vartype.IsAlwaysInScope() || ck.MkLines.Tools.SeenPrefs {
		return
	}
	if G.Pkg != nil && G.Pkg.seenPrefs {
		return
	}
	mkline := ck.MkLine
	basename := mkline.Basename
	if basename == "builtin.mk" {
		return
	}

	if ck.vartype.IsPackageSettable() {
		// For package-settable variables, the explanation below
		// doesn't make sense since including bsd.prefs.mk won't
		// define any package-settable variables.
		return
	}

	if !ck.MkLines.once.FirstTime("bsd.prefs.mk") {
		return
	}

	include := condStr(
		basename == "buildlink3.mk",
		"mk/bsd.fast.prefs.mk",
		"mk/bsd.prefs.mk")
	currInclude := G.Pkgsrc.File(NewPkgsrcPath(NewPath(include)))

	mkline.Warnf("To use %s at load time, .include %q first.",
		ck.use.varname, mkline.Rel(currInclude))
	mkline.Explain(
		"The user-settable variables and several other variables",
		"from the pkgsrc infrastructure are only available",
		"after the preferences have been loaded.",
		"",
		"Before that, these variables are undefined.")
}

// warnToolLoadTime logs a warning that the tool ${varname}
// should not be used at load time.
func (ck *MkVarUseChecker) warnToolLoadTime(varname string, tool *Tool) {
	// TODO: While using a tool by its variable name may be ok at load time,
	//  doing the same with the plain name of a tool is never ok.
	//  "VAR!= cat" is never guaranteed to call the correct cat.
	//  Even for shell builtins like echo and printf, bmake may decide
	//  to skip the shell and execute the commands via execve, which
	//  means that even echo is not a shell-builtin anymore.

	// TODO: Replace "parse time" with "load time" everywhere.

	if tool.Validity == AfterPrefsMk {
		ck.MkLine.Warnf("To use the tool ${%s} at load time, bsd.prefs.mk has to be included before.", varname)
		return
	}

	if ck.MkLine.Basename == "Makefile" {
		pkgsrcTool := G.Pkgsrc.Tools.ByName(tool.Name)
		if pkgsrcTool != nil && pkgsrcTool.Validity == Nowhere {
			// The tool must have been added too late to USE_TOOLS,
			// i.e. after bsd.prefs.mk has been included.
			ck.MkLine.Warnf("To use the tool ${%s} at load time, it has to be added to USE_TOOLS before including bsd.prefs.mk.", varname)
			return
		}
	}

	ck.MkLine.Warnf("The tool ${%s} cannot be used at load time.", varname)
	ck.MkLine.Explain(
		"To use a tool at load time, it must be declared in the package",
		"Makefile by adding it to USE_TOOLS.",
		"After that, bsd.prefs.mk must be included.",
		"Adding the tool to USE_TOOLS at any later time has no effect,",
		"which means that the tool can only be used at run time.",
		"That's the rule for the package Makefiles.",
		"",
		"Since any other .mk file can be included from anywhere else, there",
		"is no guarantee that the tool is properly defined for using it at",
		"load time (see above for the tricky rules).",
		"Therefore the tools can only be used at run time,",
		"except in the package Makefile itself.")
}

// checkVarUseWords checks whether a variable use of the form ${VAR}
// or ${VAR:modifiers} is allowed in a certain context.
func (ck *MkVarUseChecker) checkQuoting(vuc *VarUseContext) {
	if !G.Opts.WarnQuoting || vuc.quoting == VucQuotUnknown {
		return
	}

	varUse := ck.use
	vartype := ck.vartype

	needsQuoting := ck.MkLine.VariableNeedsQuoting(ck.MkLines, varUse, vartype, vuc)
	if needsQuoting == unknown {
		return
	}

	mod := varUse.Mod()

	// In GNU configure scripts, a few variables need to be passed through
	// the :M* modifier before they reach the configure scripts. Otherwise
	// the leading or trailing spaces will lead to strange caching errors
	// since the GNU configure scripts cannot handle these space characters.
	//
	// When doing checks outside a package, the :M* modifier is needed for safety.
	needMstar := (G.Pkg == nil || G.Pkg.vars.IsDefined("GNU_CONFIGURE")) &&
		matches(varUse.varname, `^(?:.*_)?(?:CFLAGS|CPPFLAGS|CXXFLAGS|FFLAGS|LDFLAGS|LIBS)$`)

	mkline := ck.MkLine
	if mod == ":M*:Q" && !needMstar {
		if !vartype.IsGuessed() {
			mkline.Notef("The :M* modifier is not needed here.")
		}

	} else if needsQuoting == yes {
		ck.checkQuotingQM(mod, needMstar, vuc)
	}

	if hasSuffix(mod, ":Q") && needsQuoting == no {
		ck.warnRedundantModifierQ(mod)
	}
}

func (ck *MkVarUseChecker) checkQuotingQM(mod string, needMstar bool, vuc *VarUseContext) {
	vartype := ck.vartype
	varname := ck.use.varname

	modNoQ := strings.TrimSuffix(mod, ":Q")
	modNoM := strings.TrimSuffix(modNoQ, ":M*")
	correctMod := modNoM + condStr(needMstar, ":M*:Q", ":Q")

	if correctMod == mod+":Q" && vuc.IsWordPart && !vartype.IsShell() {

		isSingleWordConstant := func() bool {
			if G.Pkg == nil {
				return false
			}

			varinfo := G.Pkg.redundant.vars[varname]
			if varinfo == nil || !varinfo.vari.IsConstant() {
				return false
			}

			value := varinfo.vari.ConstantValue()
			return len(ck.MkLine.ValueFields(value)) == 1
		}

		if vartype.IsList() && isSingleWordConstant() {
			// Do not warn in this special case, which typically occurs
			// for BUILD_DIRS or similar package-settable variables.

		} else if vartype.IsList() {
			ck.warnListVariableInWord()
		} else {
			ck.warnMissingModifierQInWord()
		}

	} else if mod != correctMod {
		if vuc.quoting == VucQuotPlain {
			ck.fixQuotingModifiers(correctMod, mod)
		} else {
			ck.warnWrongQuotingModifiers(correctMod, mod)
		}

	} else if vuc.quoting != VucQuotPlain {
		ck.warnModifierQInQuotes(mod)
	}
}

func (ck *MkVarUseChecker) warnListVariableInWord() {
	mkline := ck.MkLine

	mkline.Warnf("The list variable %s should not be embedded in a word.",
		ck.use.varname)
	mkline.Explain(
		"When a list variable has multiple elements, this expression expands",
		"to something unexpected:",
		"",
		"Example: ${MASTER_SITE_SOURCEFORGE}directory/ expands to",
		"",
		"\thttps://mirror1.sf.net/ https://mirror2.sf.net/directory/",
		"",
		"The first URL is missing the directory.",
		"To fix this, write",
		"\t${MASTER_SITE_SOURCEFORGE:=directory/}.",
		"",
		"Example: -l${LIBS} expands to",
		"",
		"\t-llib1 lib2",
		"",
		"The second library is missing the -l.",
		"To fix this, write ${LIBS:S,^,-l,}.")
}

func (ck *MkVarUseChecker) warnMissingModifierQInWord() {
	mkline := ck.MkLine

	mkline.Warnf("The variable %s should be quoted as part of a shell word.",
		ck.use.varname)
	mkline.Explain(
		"This variable can contain spaces or other special characters.",
		"Therefore it should be quoted by replacing ${VAR} with ${VAR:Q}.")
}

func (ck *MkVarUseChecker) fixQuotingModifiers(correctMod string, mod string) {
	varname := ck.use.varname

	fix := ck.MkLine.Autofix()
	fix.Warnf("Please use ${%s%s} instead of ${%s%s}.", varname, correctMod, varname, mod)
	fix.Explain(
		seeGuide("Echoing a string exactly as-is", "echo-literal"))
	fix.Replace("${"+varname+mod+"}", "${"+varname+correctMod+"}")
	fix.Apply()
}

func (ck *MkVarUseChecker) warnWrongQuotingModifiers(correctMod string, mod string) {
	mkline := ck.MkLine
	varname := ck.use.varname

	mkline.Warnf("Please use ${%s%s} instead of ${%s%s} and make sure"+
		" the variable appears outside of any quoting characters.", varname, correctMod, varname, mod)
	mkline.Explain(
		"The :Q modifier only works reliably when it is used outside of any",
		"quoting characters like 'single' or \"double\" quotes or `backticks`.",
		"",
		"Examples:",
		"Instead of CFLAGS=\"${CFLAGS:Q}\",",
		"     write CFLAGS=${CFLAGS:Q}.",
		"Instead of 's,@CFLAGS@,${CFLAGS:Q},',",
		"     write 's,@CFLAGS@,'${CFLAGS:Q}','.",
		"",
		seeGuide("Echoing a string exactly as-is", "echo-literal"))
}

func (ck *MkVarUseChecker) warnModifierQInQuotes(mod string) {
	mkline := ck.MkLine

	mkline.Warnf("Please move ${%s%s} outside of any quoting characters.",
		ck.use.varname, mod)
	mkline.Explain(
		"The :Q modifier only works reliably when it is used outside of any",
		"quoting characters like 'single' or \"double\" quotes or `backticks`.",
		"",
		"Examples:",
		"Instead of CFLAGS=\"${CFLAGS:Q}\",",
		"     write CFLAGS=${CFLAGS:Q}.",
		"Instead of 's,@CFLAGS@,${CFLAGS:Q},',",
		"     write 's,@CFLAGS@,'${CFLAGS:Q}','.",
		"",
		seeGuide("Echoing a string exactly as-is", "echo-literal"))
}

func (ck *MkVarUseChecker) warnRedundantModifierQ(mod string) {
	varname := ck.use.varname

	bad := "${" + varname + mod + "}"
	good := "${" + varname + strings.TrimSuffix(mod, ":Q") + "}"

	fix := ck.MkLine.Line.Autofix()
	fix.Notef("The :Q modifier isn't necessary for ${%s} here.", varname)
	fix.Explain(
		"Many variables in pkgsrc do not need the :Q modifier since they",
		"are not expected to contain whitespace or other special characters.",
		"Examples for these \"safe\" variables are:",
		"",
		"\t* filenames",
		"\t* directory names",
		"\t* user and group names",
		"\t* tool names and tool paths",
		"\t* variable names",
		"\t* package names (but not dependency patterns like pkg>=1.2)")
	fix.Replace(bad, good)
	fix.Apply()
}

func (ck *MkVarUseChecker) checkBuildDefs() {
	varname := ck.use.varname

	if !G.Pkgsrc.UserDefinedVars.IsDefined(varname) || G.Pkgsrc.IsBuildDef(varname) {
		return
	}
	if ck.MkLines.buildDefs[varname] {
		return
	}
	if !ck.MkLines.once.FirstTimeSlice("BUILD_DEFS", varname) {
		return
	}

	ck.MkLine.Warnf("The user-defined variable %s is used but not added to BUILD_DEFS.", varname)
	ck.MkLine.Explain(
		"When a pkgsrc package is built, many things can be configured by the",
		"pkgsrc user in the mk.conf file.",
		"All these configurations should be recorded in the binary package",
		"so the package can be reliably rebuilt.",
		"The BUILD_DEFS variable contains a list of all these",
		"user-settable variables, so please add your variable to it, too.")
}

func (ck *MkVarUseChecker) checkDeprecated() {
	varname := ck.use.varname
	instead := G.Pkgsrc.Deprecated[varname]
	if instead == "" {
		instead = G.Pkgsrc.Deprecated[varnameCanon(varname)]
	}
	if instead == "" {
		return
	}

	ck.MkLine.Warnf("Use of %q is deprecated. %s", varname, instead)
}