pkgtools/pkglint: update to 19.3.18

Changes since 19.3.17:

The SUBST check has been completely rewritten. It can handle several
SUBST classes at the same time now. This reduces the number of wrong
warnings.

8 files changed, 1240 insertions, 902 deletions

diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile
index d4e092b8e6a..9bbdcac99b6 100644
--- a/pkgtools/pkglint/Makefile
+++ b/pkgtools/pkglint/Makefile
@@ -1,7 +1,6 @@
-# $NetBSD: Makefile,v 1.618 2019/12/13 07:44:02 bsiegert Exp $
+# $NetBSD: Makefile,v 1.619 2019/12/14 18:04:15 rillig Exp $
 
-PKGNAME=	pkglint-19.3.17
-PKGREVISION=	1
+PKGNAME=	pkglint-19.3.18
 CATEGORIES=	pkgtools
 DISTNAME=	tools
 MASTER_SITES=	${MASTER_SITE_GITHUB:=golang/}
diff --git a/pkgtools/pkglint/files/mkcondchecker_test.go b/pkgtools/pkglint/files/mkcondchecker_test.go
index 6b55aaaa898..062897874f5 100644
--- a/pkgtools/pkglint/files/mkcondchecker_test.go
+++ b/pkgtools/pkglint/files/mkcondchecker_test.go
@@ -73,8 +73,7 @@ func (s *Suite) Test_MkCondChecker_Check(c *check.C) {
 	test(".if ${MACHINE_PLATFORM:MUnknownOS-*-*} || ${MACHINE_ARCH:Mx86}",
 		"WARN: filename.mk:4: "+
 			"The pattern \"UnknownOS\" cannot match any of "+
-			"{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+
-			"IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+
+			"{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+
 			"} for the operating system part of MACHINE_PLATFORM.",
 		"WARN: filename.mk:4: "+
 			"The pattern \"x86\" cannot match any of "+
diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go
index 49aa13296c0..1ee397e665f 100644
--- a/pkgtools/pkglint/files/substcontext.go
+++ b/pkgtools/pkglint/files/substcontext.go
@@ -3,30 +3,25 @@ package pkglint
 import "netbsd.org/pkglint/textproc"
 
 // SubstContext records the state of a block of variable assignments
-// that make up a SUBST class (see `mk/subst.mk`).
+// that make up a SUBST class.
+//
+// See mk/subst.mk.
 type SubstContext struct {
-	queuedIds []string
-	id        string
-	doneIds   map[string]bool
+	active *substBlock
 
-	foreignAllowed map[string]struct{}
-	foreign        []*MkLine
-
-	conds []*substCond
+	scopes []*substScope
 
 	once Once
 }
 
 func NewSubstContext() *SubstContext {
-	ctx := SubstContext{}
-	ctx.reset()
-	return &ctx
+	return &SubstContext{scopes: []*substScope{newSubstScope()}}
 }
 
 func (ctx *SubstContext) Process(mkline *MkLine) {
 	switch {
 	case mkline.IsEmpty():
-		ctx.finishClass(mkline)
+		ctx.emptyLine()
 	case mkline.IsVarassign():
 		ctx.varassign(mkline)
 	case mkline.IsDirective():
@@ -35,8 +30,20 @@ func (ctx *SubstContext) Process(mkline *MkLine) {
 }
 
 func (ctx *SubstContext) Finish(diag Diagnoser) {
-	ctx.finishClass(diag)
-	ctx.finishFile(diag)
+	// Prevent panics on unbalanced conditionals.
+	for len(ctx.scopes) > 1 {
+		ctx.leave(diag)
+	}
+
+	for _, scope := range ctx.scopes {
+		scope.finish(diag)
+	}
+}
+
+func (ctx *SubstContext) emptyLine() {
+	for _, scope := range ctx.scopes {
+		scope.emptyLine()
+	}
 }
 
 func (ctx *SubstContext) varassign(mkline *MkLine) {
@@ -47,8 +54,8 @@ func (ctx *SubstContext) varassign(mkline *MkLine) {
 	}
 
 	if ctx.isForeign(mkline.Varcanon()) {
-		if ctx.isActive() {
-			ctx.rememberForeign(mkline)
+		if ctx.isActive() && !ctx.block().seenEmpty {
+			ctx.block().rememberForeign(mkline)
 		}
 		return
 	}
@@ -59,115 +66,341 @@ func (ctx *SubstContext) varassign(mkline *MkLine) {
 		}
 	}
 
-	if hasPrefix(mkline.Varname(), "SUBST_") && !ctx.isActiveId(mkline.Varparam()) {
+	if hasPrefix(mkline.Varname(), "SUBST_") && mkline.Varparam() != ctx.activeId() {
 		if !ctx.varassignDifferentClass(mkline) {
 			return
 		}
 	}
 
+	block := ctx.block()
 	switch varcanon {
 	case "SUBST_STAGE.*":
-		ctx.varassignStage(mkline)
+		block.varassignStage(mkline)
 	case "SUBST_MESSAGE.*":
-		ctx.varassignMessages(mkline)
+		block.varassignMessages(mkline)
 	case "SUBST_FILES.*":
-		ctx.varassignFiles(mkline)
+		block.varassignFiles(mkline)
 	case "SUBST_SED.*":
-		ctx.varassignSed(mkline)
+		block.varassignSed(mkline)
 	case "SUBST_VARS.*":
-		ctx.varassignVars(mkline)
+		block.varassignVars(mkline)
 	case "SUBST_FILTER_CMD.*":
-		ctx.varassignFilterCmd(mkline)
+		block.varassignFilterCmd(mkline)
 	}
 }
 
 func (ctx *SubstContext) varassignClasses(mkline *MkLine) {
-	classes := mkline.ValueFields(mkline.WithoutMakeVariables(mkline.Value()))
-	if len(classes) == 0 {
+	ids := mkline.ValueFields(mkline.WithoutMakeVariables(mkline.Value()))
+	if len(ids) == 0 {
 		return
 	}
 
-	if len(classes) > 1 {
+	if len(ids) > 1 {
 		mkline.Notef("Please add only one class at a time to SUBST_CLASSES.")
 		mkline.Explain(
 			"This way, each substitution class forms a block in the package Makefile,",
 			"and to delete this block, it is not necessary to look anywhere else.")
 	}
-	for _, class := range classes {
-		ctx.queue(class)
-	}
 
-	id := classes[0]
-	if ctx.isActive() && !ctx.isActiveId(id) {
-		id := ctx.activeId() // since ctx.condEndif may reset it
+	ctx.prepareSubstClasses(mkline)
+	ctx.deactivate(mkline)
 
-		for ctx.isConditional() {
-			// This will be confusing for the outer SUBST block,
-			// but since that block is assumed to be finished,
-			// this doesn't matter.
-			ctx.condEndif(mkline)
-		}
-
-		complete := ctx.isComplete() // since ctx.finishClass will reset it
-		ctx.finishClass(mkline)
-		if !complete {
-			mkline.Warnf("Subst block %q should be finished before adding the next class to SUBST_CLASSES.", id)
+	for _, id := range ids {
+		if ctx.lookup(id) == nil {
+			ctx.scopes[len(ctx.scopes)-1].define(id)
+		} else if mkline.Varparam() == "" {
+			mkline.Errorf("Duplicate SUBST class %q.", id)
 		}
 	}
+}
 
-	ctx.setActiveId(id)
-
-	return
+func (ctx *SubstContext) prepareSubstClasses(mkline *MkLine) {
+	for _, scope := range ctx.scopes {
+		scope.prepareSubstClasses(mkline)
+	}
 }
 
 // varassignOutsideBlock handles variable assignments of SUBST variables that
 // appear without a directly corresponding SUBST block.
 func (ctx *SubstContext) varassignOutsideBlock(mkline *MkLine) (continueWithNewId bool) {
-	varparam := mkline.Varparam()
+	id := mkline.Varparam()
 
-	if ctx.isListCanon(mkline.Varcanon()) && ctx.isDone(varparam) {
+	if id != "" && ctx.isListCanon(mkline.Varcanon()) && ctx.isDone(id) {
 		if mkline.Op() != opAssignAppend {
 			mkline.Warnf("Late additions to a SUBST variable should use the += operator.")
 		}
 		return
 	}
-	if containsWord(mkline.Rationale(), varparam) {
-		return
+
+	return ctx.activate(mkline, ctx.lookup(id) == nil)
+}
+
+func (ctx *SubstContext) varassignDifferentClass(mkline *MkLine) (ok bool) {
+	varname := mkline.Varname()
+	unknown := ctx.lookup(mkline.Varparam()) == nil
+	if unknown && ctx.isActive() && !ctx.block().isComplete() {
+		mkline.Warnf("Variable %q does not match SUBST class %q.",
+			varname, ctx.activeId())
+		if !ctx.block().seenEmpty {
+			return false
+		}
+	}
+
+	return ctx.activate(mkline, unknown)
+}
+
+func (ctx *SubstContext) directive(mkline *MkLine) {
+	dir := mkline.Directive()
+	switch dir {
+	case "if":
+		ctx.enter()
+	case "elif":
+		ctx.nextBranch(mkline, false)
+	case "else":
+		ctx.nextBranch(mkline, true)
+	case "endif":
+		ctx.leave(mkline)
+	}
+}
+
+func (ctx *SubstContext) enter() {
+	for _, scope := range ctx.scopes {
+		scope.enter()
+	}
+	ctx.scopes = append(ctx.scopes, newSubstScope())
+}
+
+func (ctx *SubstContext) nextBranch(diag Diagnoser, isElse bool) {
+	if ctx.isActive() && !ctx.block().isConditional() {
+		ctx.deactivate(diag)
+	}
+
+	for _, scope := range ctx.scopes {
+		scope.nextBranch(diag, isElse)
+	}
+}
+
+func (ctx *SubstContext) leave(diag Diagnoser) {
+	ctx.deactivate(diag)
+
+	for _, scope := range ctx.scopes {
+		scope.leave(diag)
 	}
 
-	if ctx.start(varparam) {
+	if len(ctx.scopes) > 1 {
+		ctx.scopes = ctx.scopes[:len(ctx.scopes)-1]
+	}
+}
+
+func (ctx *SubstContext) activate(mkline *MkLine, deactivate bool) bool {
+	id := mkline.Varparam()
+	if id == "" {
+		mkline.Errorf("Invalid SUBST class %q in variable name.", id)
+		return false
+	}
+
+	if deactivate {
+		ctx.deactivate(mkline)
+	}
+
+	if block := ctx.lookup(id); block != nil {
+		ctx.active = block
 		return true
 	}
 
-	if ctx.once.FirstTime(varparam) {
+	if ctx.once.FirstTime(id) && !containsWord(mkline.Rationale(), id) {
 		mkline.Warnf("Before defining %s, the SUBST class "+
 			"should be declared using \"SUBST_CLASSES+= %s\".",
-			mkline.Varname(), varparam)
+			mkline.Varname(), id)
 	}
-	return
+	return false
 }
 
-func (ctx *SubstContext) varassignDifferentClass(mkline *MkLine) (ok bool) {
-	varname := mkline.Varname()
-	varparam := mkline.Varparam()
+func (ctx *SubstContext) deactivate(diag Diagnoser) {
+	if !ctx.isActive() {
+		return
+	}
 
-	if !ctx.isComplete() {
-		mkline.Warnf("Variable %q does not match SUBST class %q.", varname, ctx.activeId())
+	block := ctx.block()
+	if !block.isConditional() {
+		block.finish(diag)
+	}
+	ctx.active = nil
+}
+
+func (*SubstContext) isForeign(varcanon string) bool {
+	switch varcanon {
+	case
+		"SUBST_STAGE.*",
+		"SUBST_MESSAGE.*",
+		"SUBST_FILES.*",
+		"SUBST_SED.*",
+		"SUBST_VARS.*",
+		"SUBST_FILTER_CMD.*":
 		return false
 	}
+	return true
+}
+
+func (*SubstContext) isListCanon(varcanon string) bool {
+	switch varcanon {
+	case
+		"SUBST_FILES.*",
+		"SUBST_SED.*",
+		"SUBST_VARS.*":
+		return true
+	}
+	return false
+}
 
-	ctx.finishClass(mkline)
+func (ctx *SubstContext) block() *substBlock {
+	assertNotNil(ctx.active)
+	return ctx.active
+}
 
-	ctx.start(varparam)
-	return true
+func (ctx *SubstContext) lookup(id string) *substBlock {
+	for i := len(ctx.scopes) - 1; i >= 0; i-- {
+		if def := ctx.scopes[i].def(id); def != nil {
+			return def
+		}
+	}
+	return nil
+}
+
+func (ctx *SubstContext) isDone(id string) bool {
+	for _, scope := range ctx.scopes {
+		if scope.isDone(id) {
+			return true
+		}
+	}
+	return false
+}
+
+func (ctx *SubstContext) isActive() bool { return ctx.active != nil }
+
+func (ctx *SubstContext) activeId() string {
+	assert(ctx.isActive())
+	return ctx.active.id
+}
+
+type substScope struct {
+	defs []*substBlock
+}
+
+func newSubstScope() *substScope {
+	return &substScope{nil}
+}
+
+func (s *substScope) def(id string) *substBlock {
+	for _, def := range s.defs {
+		if def.id == id {
+			return def
+		}
+	}
+	return nil
+}
+
+func (s *substScope) define(id string) {
+	assert(s.def(id) == nil)
+	s.defs = append(s.defs, newSubstBlock(id))
+}
+
+func (s *substScope) isDone(id string) bool {
+	def := s.def(id)
+	return def != nil && def.done
 }
 
-func (ctx *SubstContext) varassignStage(mkline *MkLine) {
-	if ctx.isConditional() {
+func (s *substScope) emptyLine() {
+	for _, block := range s.defs {
+		block.seenEmpty = true
+	}
+}
+
+// finish brings all blocks that are defined in the current scope
+// to an end.
+func (s *substScope) finish(diag Diagnoser) {
+	for _, block := range s.defs {
+		block.finish(diag)
+	}
+}
+
+func (s *substScope) prepareSubstClasses(diag Diagnoser) {
+	for _, block := range s.defs {
+		if block.hasStarted() && !block.isComplete() {
+			diag.Warnf("Subst block %q should be finished "+
+				"before adding the next class to SUBST_CLASSES.",
+				block.id)
+		}
+	}
+}
+
+func (s *substScope) enter() {
+	for _, block := range s.defs {
+		block.enter()
+	}
+}
+
+func (s *substScope) nextBranch(diag Diagnoser, isElse bool) {
+	for _, block := range s.defs {
+		if block.isConditional() {
+			block.nextBranch(isElse)
+		} else {
+			block.finish(diag)
+		}
+	}
+}
+
+func (s *substScope) leave(diag Diagnoser) {
+	for _, block := range s.defs {
+		if block.isConditional() {
+			block.leave()
+		} else {
+			block.finish(diag)
+		}
+	}
+}
+
+type substBlock struct {
+	id string
+
+	// Records which of the SUBST variables have been seen either
+	// directly or in a conditional branch.
+	conds []*substCond
+
+	// In the paragraph of a SUBST block, there should be only variables
+	// that actually belong to the SUBST block.
+	//
+	// In addition, variables that are mentioned in SUBST_VARS may also
+	// be defined there because they closely relate to the SUBST block.
+	foreignAllowed map[string]struct{}
+	foreign        []*MkLine
+
+	// Whether there has been an empty line between the SUBST_CLASSES
+	// line and the current line.
+	//
+	// Before the empty line, variables that don't obviously belong to
+	// this SUBST block generate warnings since they may be typos,
+	// such as for a different SUBST block.
+	seenEmpty bool
+
+	// Whether the SUBST_CLASSES has already gone out of scope.
+	//
+	// XXX: When it is out of scope, it should also be unreachable
+	//  by any pkglint code. There's something inconsistent here.
+	done bool
+}
+
+func newSubstBlock(id string) *substBlock {
+	assert(id != "")
+	return &substBlock{id: id, conds: []*substCond{newSubstCond()}}
+}
+
+func (b *substBlock) varassignStage(mkline *MkLine) {
+	if b.isConditional() {
 		mkline.Warnf("%s should not be defined conditionally.", mkline.Varname())
 	}
 
-	ctx.dupString(mkline, ssStage)
+	b.dupString(mkline, ssStage)
 
 	value := mkline.Value()
 	if value == "pre-patch" || value == "post-patch" {
@@ -196,84 +429,55 @@ func (ctx *SubstContext) varassignStage(mkline *MkLine) {
 	}
 }
 
-func (ctx *SubstContext) varassignMessages(mkline *MkLine) {
+func (b *substBlock) varassignMessages(mkline *MkLine) {
 	varname := mkline.Varname()
 
-	if ctx.isConditional() {
+	if b.isConditional() {
 		mkline.Warnf("%s should not be defined conditionally.", varname)
 	}
 
-	ctx.dupString(mkline, ssMessage)
+	b.dupString(mkline, ssMessage)
 }
 
-func (ctx *SubstContext) varassignFiles(mkline *MkLine) {
-	ctx.dupList(mkline, ssFiles, ssNone)
+func (b *substBlock) varassignFiles(mkline *MkLine) {
+	b.dupList(mkline, ssFiles, ssNone)
 }
 
-func (ctx *SubstContext) varassignSed(mkline *MkLine) {
-	ctx.dupList(mkline, ssSed, ssNone)
-	ctx.seen().set(ssTransform)
+func (b *substBlock) varassignSed(mkline *MkLine) {
+	b.dupList(mkline, ssSed, ssNone)
+	b.addSeen(ssTransform)
 
-	ctx.suggestSubstVars(mkline)
+	b.suggestSubstVars(mkline)
 }
 
-func (ctx *SubstContext) varassignVars(mkline *MkLine) {
-	ctx.dupList(mkline, ssVars, ssVarsAutofix)
-	ctx.seen().set(ssTransform)
+func (b *substBlock) varassignVars(mkline *MkLine) {
+	b.dupList(mkline, ssVars, ssVarsAutofix)
+	b.addSeen(ssTransform)
 
 	for _, substVar := range mkline.Fields() {
-		ctx.allowVar(substVar)
+		b.allowVar(substVar)
 	}
 }
 
-func (ctx *SubstContext) varassignFilterCmd(mkline *MkLine) {
-	ctx.dupString(mkline, ssFilterCmd)
-	ctx.seen().set(ssTransform)
+func (b *substBlock) varassignFilterCmd(mkline *MkLine) {
+	b.dupString(mkline, ssFilterCmd)
+	b.addSeen(ssTransform)
 }
 
-func (ctx *SubstContext) dupList(mkline *MkLine, part substSeen, autofixPart substSeen) {
-	if ctx.seenInBranch(part) && mkline.Op() != opAssignAppend {
-		ctx.fixOperatorAppend(mkline, ctx.seenInBranch(autofixPart))
-	}
-	ctx.seen().set(part)
-}
-
-func (ctx *SubstContext) dupString(mkline *MkLine, part substSeen) {
-	if ctx.seenInBranch(part) {
-		mkline.Warnf("Duplicate definition of %q.", mkline.Varname())
-	}
-	ctx.seen().set(part)
-}
-
-func (ctx *SubstContext) fixOperatorAppend(mkline *MkLine, dueToAutofix bool) {
-	before := mkline.ValueAlign()
-	after := alignWith(mkline.Varname()+"+=", before)
-
-	fix := mkline.Autofix()
-	if dueToAutofix {
-		fix.Notef(SilentAutofixFormat)
-	} else {
-		fix.Warnf("All but the first assignment to %q should use the \"+=\" operator.",
-			mkline.Varname())
-	}
-	fix.Replace(before, after)
-	fix.Apply()
-}
-
-func (ctx *SubstContext) suggestSubstVars(mkline *MkLine) {
+func (b *substBlock) suggestSubstVars(mkline *MkLine) {
 
 	tokens, _ := splitIntoShellTokens(mkline.Line, mkline.Value())
 	for _, token := range tokens {
-		varname := ctx.extractVarname(mkline.UnquoteShell(token, false))
+		varname := b.extractVarname(mkline.UnquoteShell(token, false))
 		if varname == "" {
 			continue
 		}
 
-		id := ctx.activeId()
+		id := b.id
 		varop := sprintf("SUBST_VARS.%s%s%s",
 			id,
 			condStr(hasSuffix(id, "+"), " ", ""),
-			condStr(ctx.seenInBranch(ssVars), "+=", "="))
+			condStr(b.hasSeen(ssVars), "+=", "="))
 
 		fix := mkline.Autofix()
 		fix.Notef("The substitution command %q can be replaced with \"%s %s\".",
@@ -291,13 +495,43 @@ func (ctx *SubstContext) suggestSubstVars(mkline *MkLine) {
 		// assignment operators on them. It's probably not worth the
 		// effort, though.
 
-		ctx.seen().set(ssVars | ssVarsAutofix)
+		b.addSeen(ssVars)
+		b.addSeen(ssVarsAutofix)
+	}
+}
+
+func (b *substBlock) dupString(mkline *MkLine, part substSeen) {
+	if b.hasSeen(part) {
+		mkline.Warnf("Duplicate definition of %q.", mkline.Varname())
+	}
+	b.addSeen(part)
+}
+
+func (b *substBlock) dupList(mkline *MkLine, part substSeen, autofixPart substSeen) {
+	if b.hasSeenAnywhere(part) && mkline.Op() != opAssignAppend {
+		b.fixOperatorAppend(mkline, b.hasSeen(autofixPart))
+	}
+	b.addSeen(part)
+}
+
+func (b *substBlock) fixOperatorAppend(mkline *MkLine, dueToAutofix bool) {
+	before := mkline.ValueAlign()
+	after := alignWith(mkline.Varname()+"+=", before)
+
+	fix := mkline.Autofix()
+	if dueToAutofix {
+		fix.Notef(SilentAutofixFormat)
+	} else {
+		fix.Warnf("All but the first assignment to %q should use the \"+=\" operator.",
+			mkline.Varname())
 	}
+	fix.Replace(before, after)
+	fix.Apply()
 }
 
 // extractVarname extracts the variable name from a sed command of the form
 // s,@VARNAME@,${VARNAME}, and some related variants thereof.
-func (*SubstContext) extractVarname(token string) string {
+func (*substBlock) extractVarname(token string) string {
 	parser := NewMkLexer(token, nil)
 	lexer := parser.lexer
 	if !lexer.SkipByte('s') {
@@ -337,235 +571,111 @@ func (*SubstContext) extractVarname(token string) string {
 	return varname
 }
 
-func (*SubstContext) isForeign(varcanon string) bool {
-	switch varcanon {
-	case
-		"SUBST_STAGE.*",
-		"SUBST_MESSAGE.*",
-		"SUBST_FILES.*",
-		"SUBST_SED.*",
-		"SUBST_VARS.*",
-		"SUBST_FILTER_CMD.*":
-		return false
-	}
-	return true
+func (b *substBlock) isComplete() bool {
+	return b.allSeen().hasAll(ssStage | ssFiles | ssTransform)
 }
 
-func (*SubstContext) isListCanon(varcanon string) bool {
-	switch varcanon {
-	case
-		"SUBST_FILES.*",
-		"SUBST_SED.*",
-		"SUBST_VARS.*":
-		return true
+func (b *substBlock) hasSeen(part substSeen) bool {
+	for _, cond := range b.conds {
+		if cond.hasSeen(part) {
+			return true
+		}
 	}
 	return false
 }
 
-func (ctx *SubstContext) directive(mkline *MkLine) {
-	dir := mkline.Directive()
-	switch dir {
-	case "if":
-		ctx.condIf()
-
-	case "elif", "else":
-		ctx.condElse(mkline, dir)
-
-	case "endif":
-		ctx.condEndif(mkline)
+func (b *substBlock) hasSeenAnywhere(part substSeen) bool {
+	for _, cond := range b.conds {
+		if cond.hasSeenAnywhere(part) {
+			return true
+		}
 	}
+	return false
 }
 
-func (ctx *SubstContext) condIf() {
-	top := substCond{total: ssAll}
-	ctx.conds = append(ctx.conds, &top)
+func (b *substBlock) allSeen() substSeen {
+	all := ssNone
+	for _, cond := range b.conds {
+		all.addAll(cond.curr)
+	}
+	return all
 }
 
-func (ctx *SubstContext) condElse(mkline *MkLine, dir string) {
-	top := ctx.cond()
-	top.total.retain(top.curr)
-	if !ctx.isConditional() {
-		ctx.finishClass(mkline)
-	}
-	top.curr = ssNone
-	top.seenElse = dir == "else"
+func (b *substBlock) addSeen(part substSeen) {
+	b.conds[len(b.conds)-1].addSeen(part)
 }
 
-func (ctx *SubstContext) condEndif(diag Diagnoser) {
-	top := ctx.cond()
-	top.total.retain(top.curr)
-	if !ctx.isConditional() {
-		ctx.finishClass(diag)
-	}
-	if !top.seenElse {
-		top.total = ssNone
-	}
-	if len(ctx.conds) > 1 {
-		ctx.conds = ctx.conds[:len(ctx.conds)-1]
-	}
-	ctx.seen().union(top.total)
+func (b *substBlock) rememberForeign(mkline *MkLine) {
+	b.foreign = append(b.foreign, mkline)
 }
 
-func (ctx *SubstContext) finishClass(diag Diagnoser) {
-	if !ctx.isActive() {
-		return
-	}
+// isConditional returns whether the current line is at a deeper conditional
+// level than the corresponding SUBST_CLASSES line.
+func (b *substBlock) isConditional() bool { return len(b.conds) > 1 }
 
-	if ctx.seen().get(ssAll) {
-		ctx.checkBlockComplete(diag)
-		ctx.checkForeignVariables()
-	} else {
-		ctx.markAsNotDone()
+func (b *substBlock) allowVar(varname string) {
+	if b.foreignAllowed == nil {
+		b.foreignAllowed = map[string]struct{}{}
 	}
-
-	ctx.reset()
+	b.foreignAllowed[varname] = struct{}{}
 }
 
-func (ctx *SubstContext) checkBlockComplete(diag Diagnoser) {
-	id := ctx.activeId()
-	seen := ctx.seen()
-	if !seen.get(ssStage) {
-		diag.Warnf("Incomplete SUBST block: SUBST_STAGE.%s missing.", id)
-	}
-	if !seen.get(ssFiles) {
-		diag.Warnf("Incomplete SUBST block: SUBST_FILES.%s missing.", id)
-	}
-	if !seen.get(ssTransform) {
-		diag.Warnf("Incomplete SUBST block: SUBST_SED.%[1]s, SUBST_VARS.%[1]s or SUBST_FILTER_CMD.%[1]s missing.", id)
-	}
-}
+func (b *substBlock) enter() { b.conds = append(b.conds, newSubstCond()) }
 
-func (ctx *SubstContext) checkForeignVariables() {
-	ctx.forEachForeignVar(func(mkline *MkLine) {
-		mkline.Warnf("Foreign variable %q in SUBST block.", mkline.Varname())
-	})
+func (b *substBlock) nextBranch(isElse bool) {
+	cond := b.conds[len(b.conds)-1]
+	cond.leaveBranch()
+	cond.enterBranch(isElse)
 }
 
-func (ctx *SubstContext) finishFile(diag Diagnoser) {
-	for _, id := range ctx.queuedIds {
-		if id != "" && !ctx.isDone(id) {
-			ctx.warnUndefinedBlock(diag, id)
-		}
-	}
-}
+func (b *substBlock) leave() {
+	assert(b.isConditional())
 
-func (*SubstContext) warnUndefinedBlock(diag Diagnoser, id string) {
-	diag.Warnf("Missing SUBST block for %q.", id)
-	diag.Explain(
-		"After adding a SUBST class to SUBST_CLASSES,",
-		"the remaining SUBST variables should be defined in the same file.",
-		"",
-		"See mk/subst.mk for the comprehensive documentation.")
+	n := len(b.conds)
+	b.conds[n-2].leaveLevel(b.conds[n-1])
+	b.conds = b.conds[:n-1]
 }
 
-// In the paragraph of a SUBST block, there should be only variables
-// that actually belong to the SUBST block.
-//
-// In addition, variables that are mentioned in SUBST_VARS may also
-// be defined there because they closely relate to the SUBST block.
+func (b *substBlock) finish(diag Diagnoser) {
+	assert(len(b.conds) == 1)
 
-func (ctx *SubstContext) allowVar(varname string) {
-	if ctx.foreignAllowed == nil {
-		ctx.foreignAllowed = make(map[string]struct{})
+	if b.done {
+		return
 	}
-	ctx.foreignAllowed[varname] = struct{}{}
-}
+	b.done = true
 
-func (ctx *SubstContext) rememberForeign(mkline *MkLine) {
-	ctx.foreign = append(ctx.foreign, mkline)
-}
-
-// forEachForeignVar performs the given action for each variable that
-// is defined in the SUBST block and is not mentioned in SUBST_VARS.
-func (ctx *SubstContext) forEachForeignVar(action func(*MkLine)) {
-	for _, foreign := range ctx.foreign {
-		if _, ok := ctx.foreignAllowed[foreign.Varname()]; !ok {
-			action(foreign)
-		}
+	if !b.hasStarted() {
+		diag.Warnf("Missing SUBST block for %q.", b.id)
+		return
 	}
-}
-
-func (ctx *SubstContext) reset() {
-	ctx.id = ""
-	ctx.foreignAllowed = nil
-	ctx.foreign = nil
-	ctx.conds = []*substCond{{seenElse: true}}
-}
-
-func (ctx *SubstContext) isActive() bool { return ctx.id != "" }
-
-func (ctx *SubstContext) isActiveId(id string) bool { return ctx.id == id }
-
-func (ctx *SubstContext) activeId() string {
-	assert(ctx.isActive())
-	return ctx.id
-}
-
-func (ctx *SubstContext) setActiveId(id string) {
-	ctx.id = id
-	ctx.cond().top = true
-	ctx.markAsDone(id)
-}
 
-func (ctx *SubstContext) queue(id string) {
-	ctx.queuedIds = append(ctx.queuedIds, id)
-}
-
-func (ctx *SubstContext) start(id string) bool {
-	for i, queuedId := range ctx.queuedIds {
-		if queuedId == id {
-			ctx.queuedIds[i] = ""
-			ctx.setActiveId(id)
-			return true
-		}
+	if !b.hasSeen(ssStage) {
+		diag.Warnf("Incomplete SUBST block: SUBST_STAGE.%s missing.", b.id)
 	}
-	return false
-}
-
-func (ctx *SubstContext) markAsDone(id string) {
-	if ctx.doneIds == nil {
-		ctx.doneIds = map[string]bool{}
+	if !b.hasSeen(ssFiles) {
+		diag.Warnf("Incomplete SUBST block: SUBST_FILES.%s missing.", b.id)
+	}
+	if !b.hasSeen(ssTransform) {
+		diag.Warnf(
+			"Incomplete SUBST block: SUBST_SED.%[1]s, "+
+				"SUBST_VARS.%[1]s or SUBST_FILTER_CMD.%[1]s missing.",
+			b.id)
 	}
-	ctx.doneIds[id] = true
-}
-
-func (ctx *SubstContext) markAsNotDone() {
-	ctx.doneIds[ctx.id] = false
-}
-
-func (ctx *SubstContext) isDone(varparam string) bool {
-	return ctx.doneIds[varparam]
-}
-
-func (ctx *SubstContext) isComplete() bool {
-	return ctx.seen().hasAll(ssStage | ssFiles | ssTransform)
-}
-
-// isConditional returns whether the current line is at a deeper conditional
-// level than the assignment where the corresponding class ID is added to
-// SUBST_CLASSES.
-//
-// TODO: Adjust the implementation to this description.
-func (ctx *SubstContext) isConditional() bool {
-	return !ctx.cond().top
-}
 
-// cond returns information about the parts of the SUBST block that
-// have already been seen in the current leaf branch of the conditionals.
-func (ctx *SubstContext) seen() *substSeen {
-	return &ctx.cond().curr
+	b.checkForeignVariables()
 }
 
-// cond returns information about the current branch of conditionals.
-func (ctx *SubstContext) cond() *substCond {
-	return ctx.conds[len(ctx.conds)-1]
+func (b *substBlock) checkForeignVariables() {
+	for _, mkline := range b.foreign {
+		if _, ok := b.foreignAllowed[mkline.Varname()]; !ok {
+			mkline.Warnf("Foreign variable %q in SUBST block.", mkline.Varname())
+		}
+	}
 }
 
-// Returns true if the given flag from substSeen has been seen
-// somewhere in the conditional path of the current line.
-func (ctx *SubstContext) seenInBranch(part substSeen) bool {
-	for _, cond := range ctx.conds {
-		if cond.curr.get(part) {
+func (b *substBlock) hasStarted() bool {
+	for _, cond := range b.conds {
+		if cond.currAny != ssNone {
 			return true
 		}
 	}
@@ -573,23 +683,16 @@ func (ctx *SubstContext) seenInBranch(part substSeen) bool {
 }
 
 type substCond struct {
-	// Tells whether a SUBST block has started at this conditional level.
-	// All variable assignments that belong to this class must happen at
-	// this conditional level or below it.
-	//
-	// TODO: For Test_SubstContext_Directive__conditional_complete,
-	//  this needs to be changed to the set of classes that have been
-	//  added to SUBST_CLASSES at this level.
-	top bool
-
 	// Collects the parts of the SUBST block that have been defined in all
 	// branches that have been parsed completely.
-	total substSeen
+	total    substSeen
+	totalAny substSeen
 
 	// Collects the parts of the SUBST block that are defined in the current
 	// branch of the conditional. At the end of the branch, they are merged
 	// into the total.
-	curr substSeen
+	curr    substSeen
+	currAny substSeen
 
 	// Marks whether the current conditional statement has
 	// an .else branch. If it doesn't, this means that all variables
@@ -597,6 +700,40 @@ type substCond struct {
 	seenElse bool
 }
 
+func newSubstCond() *substCond { return &substCond{total: ssAll} }
+
+func (c *substCond) enterBranch(isElse bool) {
+	c.curr = ssNone
+	c.currAny = ssNone
+	c.seenElse = isElse
+}
+
+func (c *substCond) leaveBranch() {
+	c.total.retainAll(c.curr)
+	c.totalAny.addAll(c.currAny)
+	c.curr = ssNone
+	c.currAny = ssNone
+}
+
+func (c *substCond) leaveLevel(child *substCond) {
+	child.leaveBranch()
+	if child.seenElse {
+		c.curr.addAll(child.total)
+	}
+	c.currAny.addAll(child.totalAny)
+}
+
+func (c *substCond) hasSeen(part substSeen) bool { return c.curr.has(part) }
+
+func (c *substCond) hasSeenAnywhere(part substSeen) bool {
+	return c.currAny.has(part)
+}
+
+func (c *substCond) addSeen(part substSeen) {
+	c.curr.set(part)
+	c.currAny.set(part)
+}
+
 // substSeen contains all variables that depend on a particular SUBST
 // class ID. These variables can be set in conditional branches, and
 // pkglint keeps track whether they are set in all branches or only
@@ -617,8 +754,16 @@ const (
 	ssNone substSeen = 0
 )
 
-func (s *substSeen) set(part substSeen)          { *s |= part }
-func (s *substSeen) get(part substSeen) bool     { return *s&part != 0 }
-func (s *substSeen) hasAll(other substSeen) bool { return *s&other == other }
-func (s *substSeen) union(other substSeen)       { *s |= other }
-func (s *substSeen) retain(other substSeen)      { *s &= other }
+func (s *substSeen) set(part substSeen) {
+	assert(part&(part-1) == 0)
+	*s |= part
+}
+
+func (s *substSeen) has(part substSeen) bool {
+	assert(part&(part-1) == 0)
+	return *s&part != 0
+}
+
+func (s substSeen) hasAll(other substSeen) bool { return s&other == other }
+func (s *substSeen) addAll(other substSeen)     { *s |= other }
+func (s *substSeen) retainAll(other substSeen)  { *s &= other }
diff --git a/pkgtools/pkglint/files/substcontext_test.go b/pkgtools/pkglint/files/substcontext_test.go
index f8bd4b67e99..8cae3f3c451 100644
--- a/pkgtools/pkglint/files/substcontext_test.go
+++ b/pkgtools/pkglint/files/substcontext_test.go
@@ -5,6 +5,7 @@ import "gopkg.in/check.v1"
 func (t *Tester) NewSubstAutofixTest(lines ...string) func(bool) {
 	return func(autofix bool) {
 		mklines := t.NewMkLines("filename.mk", lines...)
+		mklines.collectRationale()
 		ctx := NewSubstContext()
 
 		mklines.ForEach(ctx.Process)
@@ -18,111 +19,13 @@ func (t *Tester) RunSubst(lines ...string) {
 	assert(lines[len(lines)-1] != "")
 
 	mklines := t.NewMkLines("filename.mk", lines...)
+	mklines.collectRationale()
 	ctx := NewSubstContext()
 
 	mklines.ForEach(ctx.Process)
 	ctx.Finish(mklines.EOFLine())
 }
 
-func (s *Suite) Test_SubstContext__OPSYSVARS(c *check.C) {
-	t := s.Init(c)
-
-	ctx := NewSubstContext()
-
-	// SUBST_CLASSES is added to OPSYSVARS in mk/bsd.pkg.mk.
-	ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES.SunOS+=prefix"))
-	ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_CLASSES.NetBSD+=prefix"))
-	ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_FILES.prefix=Makefile"))
-	ctx.varassign(t.NewMkLine("filename.mk", 14, "SUBST_SED.prefix=s,@PREFIX@,${PREFIX},g"))
-	ctx.varassign(t.NewMkLine("filename.mk", 15, "SUBST_STAGE.prefix=post-configure"))
-
-	t.CheckEquals(ctx.isComplete(), true)
-
-	ctx.Finish(t.NewMkLine("filename.mk", 15, ""))
-
-	t.CheckOutputLines(
-		"NOTE: filename.mk:14: The substitution command \"s,@PREFIX@,${PREFIX},g\" " +
-			"can be replaced with \"SUBST_VARS.prefix= PREFIX\".")
-}
-
-func (s *Suite) Test_SubstContext__no_class(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"UNRELATED=anything",
-		"SUBST_FILES.repl+=Makefile.in",
-		"SUBST_SED.repl+=-e s,from,to,g")
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:2: Before defining SUBST_FILES.repl, " +
-			"the SUBST class should be declared using \"SUBST_CLASSES+= repl\".")
-}
-
-func (s *Suite) Test_SubstContext__multiple_classes_in_one_line(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"SUBST_CLASSES+=         one two",
-		"SUBST_STAGE.one=        post-configure",
-		"SUBST_FILES.one=        one.txt",
-		"SUBST_SED.one=          s,one,1,g",
-		"SUBST_STAGE.two=        post-configure",
-		"SUBST_FILES.two=        two.txt")
-
-	t.CheckOutputLines(
-		"NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.",
-		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.")
-}
-
-func (s *Suite) Test_SubstContext__multiple_classes_in_one_line_multiple_blocks(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"SUBST_CLASSES+=         one two",
-		"SUBST_STAGE.one=        post-configure",
-		"SUBST_FILES.one=        one.txt",
-		"SUBST_SED.one=          s,one,1,g",
-		"",
-		"SUBST_STAGE.two=        post-configure",
-		"SUBST_FILES.two=        two.txt",
-		"",
-		"SUBST_STAGE.three=      post-configure",
-		"",
-		"SUBST_VARS.four=        PREFIX",
-		"",
-		"SUBST_VARS.three=       PREFIX")
-
-	t.CheckOutputLines(
-		"NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.",
-		"WARN: filename.mk:8: Incomplete SUBST block: "+
-			"SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.",
-		"WARN: filename.mk:9: Before defining SUBST_STAGE.three, "+
-			"the SUBST class should be declared using \"SUBST_CLASSES+= three\".",
-		"WARN: filename.mk:11: Before defining SUBST_VARS.four, "+
-			"the SUBST class should be declared using \"SUBST_CLASSES+= four\".")
-}
-
-func (s *Suite) Test_SubstContext__multiple_classes_in_one_block(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"SUBST_CLASSES+=         one",
-		"SUBST_STAGE.one=        post-configure",
-		"SUBST_STAGE.one=        post-configure",
-		"SUBST_FILES.one=        one.txt",
-		"SUBST_CLASSES+=         two", // The block "one" is not finished yet.
-		"SUBST_SED.one=          s,one,1,g",
-		"SUBST_STAGE.two=        post-configure",
-		"SUBST_FILES.two=        two.txt",
-		"SUBST_SED.two=          s,two,2,g")
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:3: Duplicate definition of \"SUBST_STAGE.one\".",
-		"WARN: filename.mk:5: Incomplete SUBST block: SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.",
-		"WARN: filename.mk:5: Subst block \"one\" should be finished before adding the next class to SUBST_CLASSES.",
-		"WARN: filename.mk:6: Variable \"SUBST_SED.one\" does not match SUBST class \"two\".")
-}
-
 // This is a strange example that probably won't occur in practice.
 //
 // Continuing a SUBST class in one of the branches and starting
@@ -147,26 +50,7 @@ func (s *Suite) Test_SubstContext__partially_continued_class_in_conditional(c *c
 	t.CheckOutputEmpty()
 }
 
-func (s *Suite) Test_SubstContext__files_missing(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"SUBST_CLASSES+=         one",
-		"SUBST_STAGE.one=        pre-configure",
-		"SUBST_CLASSES+=         two",
-		"SUBST_STAGE.two=        pre-configure",
-		"SUBST_FILES.two=        two.txt",
-		"SUBST_SED.two=          s,two,2,g")
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:3: Incomplete SUBST block: SUBST_FILES.one missing.",
-		"WARN: filename.mk:3: Incomplete SUBST block: "+
-			"SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.",
-		"WARN: filename.mk:3: Subst block \"one\" should be finished "+
-			"before adding the next class to SUBST_CLASSES.")
-}
-
-func (s *Suite) Test_SubstContext__directives(c *check.C) {
+func (s *Suite) Test_SubstContext__conditionals(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
@@ -193,88 +77,75 @@ func (s *Suite) Test_SubstContext__directives(c *check.C) {
 			"to \"SUBST_SED.os\" should use the \"+=\" operator.")
 }
 
-func (s *Suite) Test_SubstContext__adjacent(c *check.C) {
+func (s *Suite) Test_SubstContext_varassign__no_class(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=\t1",
-		"SUBST_STAGE.1=\tpre-configure",
-		"SUBST_FILES.1=\tfile1",
-		"SUBST_SED.1=\t-e s,subst1,repl1,",
-		"SUBST_CLASSES+=\t2",
-		"SUBST_SED.1+=\t-e s,subst1b,repl1b,", // Misplaced
-		"SUBST_STAGE.2=\tpre-configure",
-		"SUBST_FILES.2=\tfile2",
-		"SUBST_SED.2=\t-e s,subst2,repl2,")
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:6: Variable \"SUBST_SED.1\" does not match SUBST class \"2\".")
-}
-
-func (s *Suite) Test_SubstContext__do_patch(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"SUBST_CLASSES+=\tos",
-		"SUBST_STAGE.os=\tdo-patch",
-		"SUBST_FILES.os=\tguess-os.h",
-		"SUBST_SED.os=\t-e s,@OPSYS@,Darwin,")
-
-	// No warning, since there is nothing to fix automatically.
-	// This case doesn't occur in practice anyway.
-	t.CheckOutputEmpty()
-}
-
-// Variables mentioned in SUBST_VARS are not considered "foreign"
-// in the block and may be mixed with the other SUBST variables.
-func (s *Suite) Test_SubstContext__SUBST_VARS_defined_in_block(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"SUBST_CLASSES+=\tos",
-		"SUBST_STAGE.os=\tpre-configure",
-		"SUBST_FILES.os=\tguess-os.h",
-		"SUBST_VARS.os=\tTODAY1",
-		"TODAY1!=\tdate",
-		"TODAY2!=\tdate")
+		"UNRELATED=anything",
+		"SUBST_FILES.repl+=Makefile.in",
+		"SUBST_SED.repl+=-e s,from,to,g")
 
 	t.CheckOutputLines(
-		"WARN: filename.mk:6: Foreign variable \"TODAY2\" in SUBST block.")
+		"WARN: filename.mk:2: Before defining SUBST_FILES.repl, " +
+			"the SUBST class should be declared using \"SUBST_CLASSES+= repl\".")
 }
 
-// Variables mentioned in SUBST_VARS may appear in the same paragraph,
-// or alternatively anywhere else in the file.
-func (s *Suite) Test_SubstContext__SUBST_VARS_in_next_paragraph(c *check.C) {
+func (s *Suite) Test_SubstContext_varassign__multiple_classes_in_one_line_multiple_blocks(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=\tos",
-		"SUBST_STAGE.os=\tpre-configure",
-		"SUBST_FILES.os=\tguess-os.h",
-		"SUBST_VARS.os=\tTODAY1",
+		"SUBST_CLASSES+=         one two",
+		"SUBST_STAGE.one=        post-configure",
+		"SUBST_FILES.one=        one.txt",
+		"SUBST_SED.one=          s,one,1,g",
 		"",
-		"TODAY1!=\tdate",
-		"TODAY2!=\tdate")
+		"SUBST_STAGE.two=        post-configure",
+		"SUBST_FILES.two=        two.txt",
+		"",
+		"SUBST_STAGE.three=      post-configure",
+		"",
+		"SUBST_VARS.four=        PREFIX",
+		"",
+		"SUBST_VARS.three=       PREFIX")
 
-	t.CheckOutputEmpty()
+	t.CheckOutputLines(
+		"NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.",
+		"WARN: filename.mk:9: Variable \"SUBST_STAGE.three\" "+
+			"does not match SUBST class \"two\".",
+		"WARN: filename.mk:9: Incomplete SUBST block: "+
+			"SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.",
+		"WARN: filename.mk:9: Before defining SUBST_STAGE.three, "+
+			"the SUBST class should be declared using \"SUBST_CLASSES+= three\".",
+		"WARN: filename.mk:11: Before defining SUBST_VARS.four, "+
+			"the SUBST class should be declared using \"SUBST_CLASSES+= four\".")
 }
 
-func (s *Suite) Test_SubstContext__multiple_SUBST_VARS(c *check.C) {
+func (s *Suite) Test_SubstContext_varassign__multiple_classes_in_one_block(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=\tos",
-		"SUBST_STAGE.os=\tpre-configure",
-		"SUBST_FILES.os=\tguess-os.h",
-		"SUBST_VARS.os=\tPREFIX VARBASE")
+		"SUBST_CLASSES+=         one",
+		"SUBST_STAGE.one=        post-configure",
+		"SUBST_STAGE.one=        post-configure",
+		"SUBST_FILES.one=        one.txt",
+		"SUBST_CLASSES+=         two", // The block "one" is not finished yet.
+		"SUBST_SED.one=          s,one,1,g",
+		"SUBST_STAGE.two=        post-configure",
+		"SUBST_FILES.two=        two.txt",
+		"SUBST_SED.two=          s,two,2,g")
 
-	t.CheckOutputEmpty()
+	t.CheckOutputLines(
+		"WARN: filename.mk:3: Duplicate definition of \"SUBST_STAGE.one\".",
+		"WARN: filename.mk:5: Subst block \"one\" should be finished "+
+			"before adding the next class to SUBST_CLASSES.",
+		"WARN: filename.mk:5: Incomplete SUBST block: SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.",
+		"WARN: filename.mk:6: Late additions to a SUBST variable should use the += operator.")
 }
 
-// As of May 2019, pkglint does not check the order of the variables in
+// As of December 2019, pkglint does not check the order of the variables in
 // a SUBST block. Enforcing this order, or at least suggesting it, would
 // make pkgsrc packages more uniform, which is a good idea, but not urgent.
-func (s *Suite) Test_SubstContext__unusual_variable_order(c *check.C) {
+func (s *Suite) Test_SubstContext_varassign__unusual_order(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
@@ -287,41 +158,7 @@ func (s *Suite) Test_SubstContext__unusual_variable_order(c *check.C) {
 	t.CheckOutputEmpty()
 }
 
-func (s *Suite) Test_SubstContext__completely_conditional_then(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		".if ${OPSYS} == Linux",
-		"SUBST_CLASSES+=\tid",
-		"SUBST_STAGE.id=\tpre-configure",
-		"SUBST_SED.id=\t-e sahara",
-		".else",
-		".endif")
-
-	// The block already ends at the .else, not at the end of the file,
-	// since that is the scope where the SUBST id is defined.
-	t.CheckOutputLines(
-		"WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.id missing.")
-}
-
-func (s *Suite) Test_SubstContext__completely_conditional_else(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		".if ${OPSYS} == Linux",
-		".else",
-		"SUBST_CLASSES+=\tid",
-		"SUBST_STAGE.id=\tpre-configure",
-		"SUBST_SED.id=\t-e sahara",
-		".endif")
-
-	// The block already ends at the .endif, not at the end of the file,
-	// since that is the scope where the SUBST id is defined.
-	t.CheckOutputLines(
-		"WARN: filename.mk:6: Incomplete SUBST block: SUBST_FILES.id missing.")
-}
-
-func (s *Suite) Test_SubstContext__SUBST_CLASSES_in_separate_paragraph(c *check.C) {
+func (s *Suite) Test_SubstContext_varassign__blocks_in_separate_paragraphs(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
@@ -344,7 +181,7 @@ func (s *Suite) Test_SubstContext__SUBST_CLASSES_in_separate_paragraph(c *check.
 		"WARN: filename.mk:EOF: Missing SUBST block for \"4\".")
 }
 
-func (s *Suite) Test_SubstContext__wrong_class(c *check.C) {
+func (s *Suite) Test_SubstContext_varassign__typo_in_id(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
@@ -358,33 +195,9 @@ func (s *Suite) Test_SubstContext__wrong_class(c *check.C) {
 
 	t.CheckOutputLines(
 		"NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.",
-		"WARN: filename.mk:2: Variable \"SUBST_STAGE.x\" does not match SUBST class \"1\".",
-		"WARN: filename.mk:3: Variable \"SUBST_FILES.x\" does not match SUBST class \"1\".",
-		"WARN: filename.mk:4: Variable \"SUBST_VARS.x\" does not match SUBST class \"1\".",
-		// XXX: This line could change to 2, since that is already in the queue.
-		"WARN: filename.mk:5: Variable \"SUBST_STAGE.2\" does not match SUBST class \"1\".",
-		"WARN: filename.mk:6: Variable \"SUBST_FILES.2\" does not match SUBST class \"1\".",
-		"WARN: filename.mk:7: Variable \"SUBST_VARS.2\" does not match SUBST class \"1\".",
-		"WARN: filename.mk:EOF: Missing SUBST block for \"1\".",
-		"WARN: filename.mk:EOF: Missing SUBST block for \"2\".")
-}
-
-func (s *Suite) Test_SubstContext_varassign__late_addition(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		"SUBST_CLASSES+=\tid",
-		"SUBST_STAGE.id=\tpost-configure",
-		"SUBST_FILES.id=\tfiles",
-		"SUBST_VARS.id=\tPREFIX",
-		"",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_VARS.id=\tOPSYS",
-		".endif")
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:7: Late additions to a SUBST variable " +
-			"should use the += operator.")
+		"WARN: filename.mk:2: Before defining SUBST_STAGE.x, "+
+			"the SUBST class should be declared using \"SUBST_CLASSES+= x\".",
+		"WARN: filename.mk:EOF: Missing SUBST block for \"1\".")
 }
 
 func (s *Suite) Test_SubstContext_varassign__late_addition_to_unknown_class(c *check.C) {
@@ -403,6 +216,93 @@ func (s *Suite) Test_SubstContext_varassign__late_addition_to_unknown_class(c *c
 			"the SUBST class should be declared using \"SUBST_CLASSES+= id\".")
 }
 
+func (s *Suite) Test_SubstContext_varassign__rationale(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"# Adjust setup.py",
+		"SUBST_CLASSES+=         setup",
+		"SUBST_STAGE.setup=      post-configure",
+		"SUBST_FILES.setup=      setup.py",
+		"SUBST_VARS.setup=       VAR")
+
+	// The rationale in line 1 is supposed to suppress warnings,
+	// not add new ones.
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_SubstContext_varassign__interleaved(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+= 1 2 3",
+		"SUBST_STAGE.1=  post-configure",
+		"SUBST_STAGE.2=  post-configure",
+		"SUBST_STAGE.3=  post-configure",
+		"SUBST_FILES.1=  setup.py",
+		"SUBST_FILES.2=  setup.py",
+		"SUBST_FILES.3=  setup.py",
+		"SUBST_VARS.1=   VAR",
+		"SUBST_VARS.2=   VAR",
+		"SUBST_VARS.3=   VAR")
+
+	// The above does not follow the common pattern of defining
+	// each block on its own.
+	// It technically works but is not easy to read for humans.
+	t.CheckOutputLines(
+		"NOTE: filename.mk:1: " +
+			"Please add only one class at a time to SUBST_CLASSES.")
+}
+
+func (s *Suite) Test_SubstContext_varassignClasses__OPSYSVARS(c *check.C) {
+	t := s.Init(c)
+
+	ctx := NewSubstContext()
+
+	// SUBST_CLASSES is added to OPSYSVARS in mk/bsd.pkg.mk.
+	ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES.SunOS+=prefix"))
+	ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_CLASSES.NetBSD+=prefix"))
+	ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_FILES.prefix=Makefile"))
+	ctx.varassign(t.NewMkLine("filename.mk", 14, "SUBST_SED.prefix=s,@PREFIX@,${PREFIX},g"))
+	ctx.varassign(t.NewMkLine("filename.mk", 15, "SUBST_STAGE.prefix=post-configure"))
+
+	t.CheckEquals(ctx.block().isComplete(), true)
+
+	ctx.Finish(t.NewMkLine("filename.mk", 15, ""))
+
+	t.CheckOutputLines(
+		"NOTE: filename.mk:14: The substitution command \"s,@PREFIX@,${PREFIX},g\" " +
+			"can be replaced with \"SUBST_VARS.prefix= PREFIX\".")
+}
+
+func (s *Suite) Test_SubstContext_varassignClasses__duplicate_id(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+= id",
+		"SUBST_CLASSES+= id")
+
+	t.CheckOutputLines(
+		"ERROR: filename.mk:2: Duplicate SUBST class \"id\".",
+		"WARN: filename.mk:EOF: Missing SUBST block for \"id\".")
+}
+
+func (s *Suite) Test_SubstContext_varassignClasses__multiple_classes_in_one_line(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         one two",
+		"SUBST_STAGE.one=        post-configure",
+		"SUBST_FILES.one=        one.txt",
+		"SUBST_SED.one=          s,one,1,g",
+		"SUBST_STAGE.two=        post-configure",
+		"SUBST_FILES.two=        two.txt")
+
+	t.CheckOutputLines(
+		"NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.")
+}
+
 func (s *Suite) Test_SubstContext_varassignClasses__none(c *check.C) {
 	t := s.Init(c)
 
@@ -428,6 +328,44 @@ func (s *Suite) Test_SubstContext_varassignClasses__indirect(c *check.C) {
 		"WARN: filename.mk:2: VAR is used but not defined.")
 }
 
+func (s *Suite) Test_SubstContext_varassignOutsideBlock__assign(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=\t1",
+		"SUBST_STAGE.1=\tpre-configure",
+		"SUBST_FILES.1=\tfile1",
+		"SUBST_SED.1=\t-e s,subst1,repl1,",
+		"SUBST_CLASSES+=\t2",
+		"SUBST_SED.1=\t-e s,subst1b,repl1b,", // Misplaced
+		"SUBST_STAGE.2=\tpre-configure",
+		"SUBST_FILES.2=\tfile2",
+		"SUBST_SED.2=\t-e s,subst2,repl2,")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:6: Late additions to a SUBST variable " +
+			"should use the += operator.")
+}
+
+func (s *Suite) Test_SubstContext_varassignOutsideBlock__append(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=\t1",
+		"SUBST_STAGE.1=\tpre-configure",
+		"SUBST_FILES.1=\tfile1",
+		"SUBST_SED.1=\t-e s,subst1,repl1,",
+		"SUBST_CLASSES+=\t2",
+		"SUBST_SED.1+=\t-e s,subst1b,repl1b,", // Misplaced
+		"SUBST_STAGE.2=\tpre-configure",
+		"SUBST_FILES.2=\tfile2",
+		"SUBST_SED.2=\t-e s,subst2,repl2,")
+
+	// The SUBST_SED.1 line is misplaced. It uses the += operator,
+	// which makes it unclear whether this is a typo or not.
+	t.CheckOutputEmpty()
+}
+
 // The rationale for the stray SUBST variables has to be specific.
 //
 // For example, in the following snippet from mail/dkim-milter/options.mk
@@ -472,7 +410,420 @@ func (s *Suite) Test_SubstContext_varassignOutsideBlock__rationale(c *check.C) {
 			"the SUBST class should be declared using \"SUBST_CLASSES+= libs\".")
 }
 
-func (s *Suite) Test_SubstContext_varassignStage__pre_patch(c *check.C) {
+// Unbalanced conditionals must not lead to a panic.
+func (s *Suite) Test_SubstContext_directive__before_SUBST_CLASSES(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		".if 0",
+		".endif",
+		"SUBST_CLASSES+=\tos",
+		".elif 0")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:4: Missing SUBST block for \"os\".")
+}
+
+func (s *Suite) Test_SubstContext_directive__conditional_blocks_complete(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		".if ${OPSYS} == NetBSD",
+		"SUBST_CLASSES+= nb",
+		"SUBST_STAGE.nb= post-configure",
+		"SUBST_FILES.nb= guess-netbsd.h",
+		"SUBST_VARS.nb=  HAVE_NETBSD",
+		".else",
+		"SUBST_CLASSES+= os",
+		"SUBST_STAGE.os= post-configure",
+		"SUBST_FILES.os= guess-netbsd.h",
+		"SUBST_VARS.os=  HAVE_OTHER",
+		".endif")
+
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_SubstContext_directive__conditional_blocks_incomplete(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		".if ${OPSYS} == NetBSD",
+		"SUBST_CLASSES+= nb",
+		"SUBST_STAGE.nb= post-configure",
+		"SUBST_VARS.nb=  HAVE_NETBSD",
+		".else",
+		"SUBST_CLASSES+= os",
+		"SUBST_STAGE.os= post-configure",
+		"SUBST_FILES.os= guess-netbsd.h",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.nb missing.",
+		"WARN: filename.mk:6: Subst block \"nb\" should be finished "+
+			"before adding the next class to SUBST_CLASSES.",
+		"WARN: filename.mk:9: Incomplete SUBST block: "+
+			"SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.")
+}
+
+func (s *Suite) Test_SubstContext_directive__conditional_complete(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+= id",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_STAGE.id=\t\tpost-configure",
+		"SUBST_MESSAGE.id=\tpost-configure",
+		"SUBST_FILES.id=\t\tguess-netbsd.h",
+		"SUBST_SED.id=\t\t-e s,from,to,",
+		"SUBST_VARS.id=\t\tHAVE_OTHER",
+		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
+		".else",
+		"SUBST_STAGE.id=\t\tpost-configure",
+		"SUBST_MESSAGE.id=\tpost-configure",
+		"SUBST_FILES.id=\t\tguess-netbsd.h",
+		"SUBST_SED.id=\t\t-e s,from,to,",
+		"SUBST_VARS.id=\t\tHAVE_OTHER",
+		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:3: SUBST_STAGE.id should not be defined conditionally.",
+		"WARN: filename.mk:4: SUBST_MESSAGE.id should not be defined conditionally.",
+		"WARN: filename.mk:10: SUBST_STAGE.id should not be defined conditionally.",
+		"WARN: filename.mk:11: SUBST_MESSAGE.id should not be defined conditionally.")
+}
+
+func (s *Suite) Test_SubstContext_directive__conditionally_overwritten_filter(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+= id",
+		"SUBST_STAGE.id=\t\tpost-configure",
+		"SUBST_MESSAGE.id=\tpost-configure",
+		"SUBST_FILES.id=\t\tguess-netbsd.h",
+		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:7: Duplicate definition of \"SUBST_FILTER_CMD.id\".")
+}
+
+// Hopefully nobody will ever trigger this case in real pkgsrc.
+// It's plain confusing to a casual reader to nest a complete
+// SUBST block into another SUBST block.
+func (s *Suite) Test_SubstContext_directive__conditionally_nested_block(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         outer",
+		"SUBST_STAGE.outer=      post-configure",
+		"SUBST_FILES.outer=      outer.txt",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_CLASSES+=         inner",
+		"SUBST_STAGE.inner=      post-configure",
+		"SUBST_FILES.inner=      inner.txt",
+		"SUBST_VARS.inner=       INNER",
+		".endif",
+		"SUBST_VARS.outer=       OUTER")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:5: Subst block \"outer\" should be finished " +
+			"before adding the next class to SUBST_CLASSES.")
+}
+
+// It's completely valid to have several SUBST blocks in a single paragraph.
+// As soon as a SUBST_CLASSES line appears, pkglint assumes that all previous
+// SUBST blocks are finished. That's exactly the case here.
+func (s *Suite) Test_SubstContext_directive__conditionally_following_block(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         outer",
+		"SUBST_STAGE.outer=      post-configure",
+		"SUBST_FILES.outer=      outer.txt",
+		"SUBST_VARS.outer=       OUTER",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_CLASSES+=         middle",
+		"SUBST_STAGE.middle=     post-configure",
+		"SUBST_FILES.middle=     inner.txt",
+		"SUBST_VARS.middle=      INNER",
+		".  if ${MACHINE_ARCH} == amd64",
+		"SUBST_CLASSES+=         inner",
+		"SUBST_STAGE.inner=      post-configure",
+		"SUBST_FILES.inner=      inner.txt",
+		"SUBST_VARS.inner=       INNER",
+		".  endif",
+		".endif")
+
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_SubstContext_directive__two_blocks_in_condition(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		".if ${OPSYS} == NetBSD",
+		"SUBST_CLASSES+= a",
+		"SUBST_STAGE.a=  post-configure",
+		"SUBST_FILES.a=  outer.txt",
+		"SUBST_VARS.a=   OUTER",
+		"SUBST_CLASSES+= b",
+		"SUBST_STAGE.b=  post-configure",
+		"SUBST_FILES.b=  inner.txt",
+		"SUBST_VARS.b=   INNER",
+		".endif")
+
+	// Up to 2019-12-12, pkglint wrongly warned in filename.mk:6:
+	//  Subst block "a" should be finished before adding
+	//  the next class to SUBST_CLASSES.
+	// The warning was wrong since block "a" has all required fields set.
+	// The warning was caused by an inconsistent check whether the current
+	// block had any conditional variables.
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_SubstContext_directive__nested_conditional_incomplete_block(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         outer",
+		"SUBST_STAGE.outer=      post-configure",
+		"SUBST_FILES.outer=      outer.txt",
+		"SUBST_VARS.outer=       OUTER",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_CLASSES+=         inner1",
+		"SUBST_STAGE.inner1=     post-configure",
+		"SUBST_VARS.inner1=      INNER",
+		"SUBST_CLASSES+=         inner2",
+		"SUBST_STAGE.inner2=     post-configure",
+		"SUBST_FILES.inner2=     inner.txt",
+		"SUBST_VARS.inner2=      INNER",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:9: Subst block \"inner1\" should be finished "+
+			"before adding the next class to SUBST_CLASSES.",
+		"WARN: filename.mk:9: Incomplete SUBST block: SUBST_FILES.inner1 missing.")
+}
+
+func (s *Suite) Test_SubstContext_leave__details_in_then_branch(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         os",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_VARS.os=          OPSYS",
+		"SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
+		"SUBST_STAGE.os=         post-configure",
+		"SUBST_MESSAGE.os=       Guessing operating system",
+		"SUBST_FILES.os=         guess-os.h",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:5: SUBST_STAGE.os should not be defined conditionally.",
+		"WARN: filename.mk:6: SUBST_MESSAGE.os should not be defined conditionally.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_STAGE.os missing.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: "+
+			"SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.")
+}
+
+func (s *Suite) Test_SubstContext_leave__details_in_else_branch(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         os",
+		".if ${OPSYS} == NetBSD",
+		".else",
+		"SUBST_VARS.os=          OPSYS",
+		"SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
+		"SUBST_STAGE.os=         post-configure",
+		"SUBST_MESSAGE.os=       Guessing operating system",
+		"SUBST_FILES.os=         guess-os.h",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:6: SUBST_STAGE.os should not be defined conditionally.",
+		"WARN: filename.mk:7: SUBST_MESSAGE.os should not be defined conditionally.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_STAGE.os missing.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: "+
+			"SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.")
+}
+
+func (s *Suite) Test_SubstContext_leave__empty_conditional_at_end(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         os",
+		"SUBST_VARS.os=          OPSYS",
+		"SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
+		"SUBST_STAGE.os=         post-configure",
+		"SUBST_MESSAGE.os=       Guessing operating system",
+		"SUBST_FILES.os=         guess-os.h",
+		".if ${OPSYS} == NetBSD",
+		".else",
+		".endif")
+
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_SubstContext_leave__missing_transformation_in_one_branch(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         os",
+		"SUBST_STAGE.os=         post-configure",
+		"SUBST_MESSAGE.os=       Guessing operating system",
+		"SUBST_FILES.os=         guess-os.h",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_FILES.os=         -e s,@OpSYS@,NetBSD,", // A simple typo, this should be SUBST_SED.
+		".elif ${OPSYS} == Darwin",
+		"SUBST_SED.os=           -e s,@OPSYS@,Darwin1,",
+		"SUBST_SED.os=           -e s,@OPSYS@,Darwin2,",
+		".else",
+		"SUBST_VARS.os=           OPSYS",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:6: All but the first assignment "+
+			"to \"SUBST_FILES.os\" should use the \"+=\" operator.",
+		"WARN: filename.mk:9: All but the first assignment "+
+			"to \"SUBST_SED.os\" should use the \"+=\" operator.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.os, "+
+			"SUBST_VARS.os or SUBST_FILTER_CMD.os missing.")
+}
+
+func (s *Suite) Test_SubstContext_leave__nested_conditionals(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=         os",
+		"SUBST_STAGE.os=         post-configure",
+		"SUBST_MESSAGE.os=       Guessing operating system",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_FILES.os=         guess-netbsd.h",
+		".  if ${ARCH} == i386",
+		"SUBST_FILTER_CMD.os=    ${SED} -e s,@OPSYS,NetBSD-i386,",
+		".  elif ${ARCH} == x86_64",
+		"SUBST_VARS.os=          OPSYS",
+		".  else",
+		"SUBST_SED.os=           -e s,@OPSYS,NetBSD-unknown",
+		".  endif",
+		".else",
+		// This branch omits SUBST_FILES.
+		"SUBST_SED.os=           -e s,@OPSYS@,unknown,",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.")
+}
+
+// With every .if directive, a new scope is created, to properly
+// keep track of the conditional level at which the SUBST classes
+// are declared.
+//
+// The scopes are even created when there is no SUBST variable
+// anywhere close. The conditionals must be tracked for determining
+// the end of the scope for the SUBST_CLASSES IDs.
+func (s *Suite) Test_substScope__conditionals(c *check.C) {
+	t := s.Init(c)
+
+	ctx := NewSubstContext()
+
+	line := func(text string) {
+		mkline := t.NewMkLine("filename.mk", 123, text)
+		ctx.Process(mkline)
+	}
+	verifyScopes := func(n int) {
+		t.CheckEquals(len(ctx.scopes), n)
+	}
+
+	verifyScopes(1)
+
+	line(".if 1")
+	verifyScopes(2)
+
+	line(".  if 1")
+	verifyScopes(3)
+
+	line(".    if 1")
+	verifyScopes(4)
+
+	line(".    elif 1")
+	verifyScopes(4)
+
+	line(".    else")
+	verifyScopes(4)
+
+	line(".    endif")
+	verifyScopes(3)
+
+	line(".  endif")
+	verifyScopes(2)
+
+	line(".endif")
+	verifyScopes(1)
+
+	// An unbalanced .endif must not lead to a panic.
+	line(".endif")
+	verifyScopes(1)
+
+	ctx.Finish(NewLineEOF("filename.mk"))
+}
+
+func (s *Suite) Test_substScope_prepareSubstClasses(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+= 1",
+		"SUBST_STAGE.1=  post-configure",
+		".if 0")
+
+	// There's no need to warn about unbalanced conditionals
+	// since that is already done by MkLines.Check.
+	t.CheckOutputLines(
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.1 missing.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: "+
+			"SUBST_SED.1, SUBST_VARS.1 or SUBST_FILTER_CMD.1 missing.")
+}
+
+func (s *Suite) Test_substScope_prepareSubstClasses__nested(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+= 1",
+		"SUBST_STAGE.1=  post-configure",
+		".if 0",
+		".if 0",
+		"SUBST_CLASSES+= 2")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:5: Subst block \"1\" should be finished "+
+			"before adding the next class to SUBST_CLASSES.",
+		"WARN: filename.mk:EOF: Missing SUBST block for \"2\".",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.1 missing.",
+		"WARN: filename.mk:EOF: Incomplete SUBST block: "+
+			"SUBST_SED.1, SUBST_VARS.1 or SUBST_FILTER_CMD.1 missing.")
+}
+
+func (s *Suite) Test_substBlock_varassignStage__do_patch(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=\tos",
+		"SUBST_STAGE.os=\tdo-patch",
+		"SUBST_FILES.os=\tguess-os.h",
+		"SUBST_SED.os=\t-e s,@OPSYS@,Darwin,")
+
+	// No warning, since there is nothing to fix automatically.
+	// This case doesn't occur in practice anyway.
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_substBlock_varassignStage__pre_patch(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -495,7 +846,7 @@ func (s *Suite) Test_SubstContext_varassignStage__pre_patch(c *check.C) {
 		"SUBST_SED.os=   -e s,@OPSYS@,Darwin,")
 }
 
-func (s *Suite) Test_SubstContext_varassignStage__post_patch(c *check.C) {
+func (s *Suite) Test_substBlock_varassignStage__post_patch(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -518,12 +869,31 @@ func (s *Suite) Test_SubstContext_varassignStage__post_patch(c *check.C) {
 		"SUBST_SED.os=   -e s,@OPSYS@,Darwin,")
 }
 
+func (s *Suite) Test_substBlock_varassignStage__empty_class(c *check.C) {
+	t := s.Init(c)
+
+	t.Chdir(".")
+
+	t.RunSubst(
+		"SUBST_CLASSES+= id",
+		"",
+		"SUBST_STAGE.=   post-patch",
+		"SUBST_STAGE.id= post-configure",
+		"SUBST_FILES.id= files",
+		"SUBST_VARS.id=  VAR",
+		"SUBST_VARS.=    VAR")
+
+	t.CheckOutputLines(
+		"ERROR: filename.mk:3: Invalid SUBST class \"\" in variable name.",
+		"ERROR: filename.mk:7: Invalid SUBST class \"\" in variable name.")
+}
+
 // As of December 2019, pkglint does not use token positions internally.
 // Instead it only does simple string replacement when autofixing things.
 // To avoid damaging anything, replacements are only done if they are
 // unambiguous. This is not the case here, since line 4 contains the
 // string "pre-patch" twice.
-func (s *Suite) Test_SubstContext_varassignStage__ambiguous_replacement(c *check.C) {
+func (s *Suite) Test_substBlock_varassignStage__ambiguous_replacement(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -541,7 +911,7 @@ func (s *Suite) Test_SubstContext_varassignStage__ambiguous_replacement(c *check
 	t.CheckEquals(t.File("filename.mk").IsFile(), false)
 }
 
-func (s *Suite) Test_SubstContext_varassignStage__with_NO_CONFIGURE(c *check.C) {
+func (s *Suite) Test_substBlock_varassignStage__with_NO_CONFIGURE(c *check.C) {
 	t := s.Init(c)
 
 	pkg := t.SetUpPackage("category/package",
@@ -572,7 +942,7 @@ func (s *Suite) Test_SubstContext_varassignStage__with_NO_CONFIGURE(c *check.C)
 			"when NO_CONFIGURE is set (in line 35).")
 }
 
-func (s *Suite) Test_SubstContext_varassignStage__without_NO_CONFIGURE(c *check.C) {
+func (s *Suite) Test_substBlock_varassignStage__without_NO_CONFIGURE(c *check.C) {
 	t := s.Init(c)
 
 	pkg := t.SetUpPackage("category/package",
@@ -589,7 +959,7 @@ func (s *Suite) Test_SubstContext_varassignStage__without_NO_CONFIGURE(c *check.
 
 // Before 2019-12-12, pkglint wrongly warned about variables that were
 // not obviously SUBST variables, even if they were used later in SUBST_VARS.
-func (s *Suite) Test_SubstContext_varassignVars__var_before_SUBST_VARS(c *check.C) {
+func (s *Suite) Test_substBlock_varassignVars__var_before_SUBST_VARS(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
@@ -612,23 +982,19 @@ func (s *Suite) Test_SubstContext_varassignVars__var_before_SUBST_VARS(c *check.
 		"WARN: filename.mk:4: Foreign variable \"FOREIGN\" in SUBST block.")
 }
 
-func (s *Suite) Test_SubstContext_dupList__conditional_before_unconditional(c *check.C) {
+func (s *Suite) Test_substBlock_varassignVars(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+= os",
-		"SUBST_STAGE.os= post-configure",
-		".if 1",
-		"SUBST_FILES.os= conditional",
-		".endif",
-		"SUBST_FILES.os= unconditional",
-		"SUBST_VARS.os=  OPSYS")
+		"SUBST_CLASSES+=\tos",
+		"SUBST_STAGE.os=\tpre-configure",
+		"SUBST_FILES.os=\tguess-os.h",
+		"SUBST_VARS.os=\tPREFIX VARBASE")
 
-	// TODO: Warn that the conditional line is overwritten.
 	t.CheckOutputEmpty()
 }
 
-func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) {
+func (s *Suite) Test_substBlock_suggestSubstVars(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -752,7 +1118,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) {
 
 // If the SUBST_CLASS identifier ends with a plus, the generated code must
 // use the correct assignment operator and be nicely formatted.
-func (s *Suite) Test_SubstContext_suggestSubstVars__plus(c *check.C) {
+func (s *Suite) Test_substBlock_suggestSubstVars__plus(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -786,7 +1152,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__plus(c *check.C) {
 // The last of the SUBST_SED variables is 15 characters wide. When SUBST_SED
 // is replaced with SUBST_VARS, this becomes 16 characters and therefore
 // requires the whole paragraph to be indented by one more tab.
-func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_realign_paragraph(c *check.C) {
+func (s *Suite) Test_substBlock_suggestSubstVars__autofix_realign_paragraph(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -818,7 +1184,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_realign_paragraph(c
 		"SUBST_VARS.pfx+=        PREFIX")
 }
 
-func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_sed(c *check.C) {
+func (s *Suite) Test_substBlock_suggestSubstVars__autofix_plus_sed(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -849,7 +1215,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_sed(c *check.C)
 		"SUBST_SED.pfx+=         -e s,@PREFIX@,other,g")
 }
 
-func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_vars(c *check.C) {
+func (s *Suite) Test_substBlock_suggestSubstVars__autofix_plus_vars(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -881,7 +1247,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_vars(c *check.C
 		"SUBST_VARS.id+= PKGMANDIR")
 }
 
-func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_indentation(c *check.C) {
+func (s *Suite) Test_substBlock_suggestSubstVars__autofix_indentation(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -911,7 +1277,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_indentation(c *check
 		"SUBST_VARS.fix-paths=           PREFIX")
 }
 
-func (s *Suite) Test_SubstContext_suggestSubstVars__conditional(c *check.C) {
+func (s *Suite) Test_substBlock_suggestSubstVars__conditional(c *check.C) {
 	t := s.Init(c)
 
 	t.Chdir(".")
@@ -947,11 +1313,46 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__conditional(c *check.C) {
 		".endif")
 }
 
-func (s *Suite) Test_SubstContext_extractVarname(c *check.C) {
+func (s *Suite) Test_substBlock_dupList__late_addition(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+=\tid",
+		"SUBST_STAGE.id=\tpost-configure",
+		"SUBST_FILES.id=\tfiles",
+		"SUBST_VARS.id=\tPREFIX",
+		"",
+		".if ${OPSYS} == NetBSD",
+		"SUBST_VARS.id=\tOPSYS",
+		".endif")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:7: All but the first assignment " +
+			"to \"SUBST_VARS.id\" should use the \"+=\" operator.")
+}
+
+func (s *Suite) Test_substBlock_dupList__conditional_before_unconditional(c *check.C) {
+	t := s.Init(c)
+
+	t.RunSubst(
+		"SUBST_CLASSES+= os",
+		"SUBST_STAGE.os= post-configure",
+		".if 1",
+		"SUBST_FILES.os= conditional",
+		".endif",
+		"SUBST_FILES.os= unconditional",
+		"SUBST_VARS.os=  OPSYS")
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:6: All but the first assignment " +
+			"to \"SUBST_FILES.os\" should use the \"+=\" operator.")
+}
+
+func (s *Suite) Test_substBlock_extractVarname(c *check.C) {
 	t := s.Init(c)
 
 	test := func(input, expected string) {
-		t.CheckEquals((*SubstContext).extractVarname(nil, input), expected)
+		t.CheckEquals((*substBlock).extractVarname(nil, input), expected)
 	}
 
 	// A simple variable name.
@@ -994,361 +1395,158 @@ func (s *Suite) Test_SubstContext_extractVarname(c *check.C) {
 	test("s,@VAR@,${VAR}suffix,", "")
 }
 
-func (s *Suite) Test_SubstContext_directive__before_SUBST_CLASSES(c *check.C) {
+func (s *Suite) Test_substBlock_isComplete__incomplete(c *check.C) {
 	t := s.Init(c)
 
-	t.RunSubst(
-		".if 0",
-		".endif",
-		"SUBST_CLASSES+=\tos",
-		".elif 0") // Just for branch coverage.
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:EOF: Missing SUBST block for \"os\".")
-}
-
-func (s *Suite) Test_SubstContext_directive__conditional_blocks_complete(c *check.C) {
-	t := s.Init(c)
-
-	t.RunSubst(
-		".if ${OPSYS} == NetBSD",
-		"SUBST_CLASSES+= nb",
-		"SUBST_STAGE.nb= post-configure",
-		"SUBST_FILES.nb= guess-netbsd.h",
-		"SUBST_VARS.nb=  HAVE_NETBSD",
-		".else",
-		"SUBST_CLASSES+= os",
-		"SUBST_STAGE.os= post-configure",
-		"SUBST_FILES.os= guess-netbsd.h",
-		"SUBST_VARS.os=  HAVE_OTHER",
-		".endif")
+	ctx := NewSubstContext()
 
-	t.CheckOutputEmpty()
-}
+	ctx.varassign(t.NewMkLine("filename.mk", 10, "PKGNAME=pkgname-1.0"))
 
-func (s *Suite) Test_SubstContext_directive__conditional_blocks_incomplete(c *check.C) {
-	t := s.Init(c)
+	t.CheckEquals(ctx.isActive(), false)
 
-	t.RunSubst(
-		".if ${OPSYS} == NetBSD",
-		"SUBST_CLASSES+= nb",
-		"SUBST_STAGE.nb= post-configure",
-		"SUBST_VARS.nb=  HAVE_NETBSD",
-		".else",
-		"SUBST_CLASSES+= os",
-		"SUBST_STAGE.os= post-configure",
-		"SUBST_FILES.os= guess-netbsd.h",
-		".endif")
+	ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES+=interp"))
 
-	t.CheckOutputLines(
-		"WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.nb missing.",
-		"WARN: filename.mk:9: Incomplete SUBST block: "+
-			"SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.")
-}
+	t.CheckEquals(ctx.isActive(), false)
 
-func (s *Suite) Test_SubstContext_directive__conditional_complete(c *check.C) {
-	t := s.Init(c)
+	ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_FILES.interp=Makefile"))
 
-	t.RunSubst(
-		"SUBST_CLASSES+= id",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_STAGE.id=\t\tpost-configure",
-		"SUBST_MESSAGE.id=\tpost-configure",
-		"SUBST_FILES.id=\t\tguess-netbsd.h",
-		"SUBST_SED.id=\t\t-e s,from,to,",
-		"SUBST_VARS.id=\t\tHAVE_OTHER",
-		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
-		".else",
-		"SUBST_STAGE.id=\t\tpost-configure",
-		"SUBST_MESSAGE.id=\tpost-configure",
-		"SUBST_FILES.id=\t\tguess-netbsd.h",
-		"SUBST_SED.id=\t\t-e s,from,to,",
-		"SUBST_VARS.id=\t\tHAVE_OTHER",
-		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
-		".endif")
+	t.CheckEquals(ctx.activeId(), "interp")
+	t.CheckEquals(ctx.block().isComplete(), false)
 
-	t.CheckOutputLines(
-		"WARN: filename.mk:3: SUBST_STAGE.id should not be defined conditionally.",
-		"WARN: filename.mk:4: SUBST_MESSAGE.id should not be defined conditionally.",
-		"WARN: filename.mk:10: SUBST_STAGE.id should not be defined conditionally.",
-		"WARN: filename.mk:11: SUBST_MESSAGE.id should not be defined conditionally.")
-}
+	ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_SED.interp=s,@PREFIX@,${PREFIX},g"))
 
-func (s *Suite) Test_SubstContext_directive__conditionally_overwritten_filter(c *check.C) {
-	t := s.Init(c)
+	t.CheckEquals(ctx.block().isComplete(), false)
 
-	t.RunSubst(
-		"SUBST_CLASSES+= id",
-		"SUBST_STAGE.id=\t\tpost-configure",
-		"SUBST_MESSAGE.id=\tpost-configure",
-		"SUBST_FILES.id=\t\tguess-netbsd.h",
-		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_FILTER_CMD.id=\tHAVE_OTHER",
-		".endif")
+	ctx.Finish(t.NewMkLine("filename.mk", 14, ""))
 
 	t.CheckOutputLines(
-		"WARN: filename.mk:7: Duplicate definition of \"SUBST_FILTER_CMD.id\".")
+		"NOTE: filename.mk:13: The substitution command \"s,@PREFIX@,${PREFIX},g\" "+
+			"can be replaced with \"SUBST_VARS.interp= PREFIX\".",
+		"WARN: filename.mk:14: Incomplete SUBST block: SUBST_STAGE.interp missing.")
 }
 
-// Hopefully nobody will ever trigger this case in real pkgsrc.
-// It's plain confusing to a casual reader to nest a complete
-// SUBST block into another SUBST block.
-// That's why pkglint doesn't cover this case correctly.
-func (s *Suite) Test_SubstContext_directive__conditionally_nested_block(c *check.C) {
+func (s *Suite) Test_substBlock_isComplete__complete(c *check.C) {
 	t := s.Init(c)
 
-	t.RunSubst(
-		"SUBST_CLASSES+=         outer",
-		"SUBST_STAGE.outer=      post-configure",
-		"SUBST_FILES.outer=      outer.txt",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_CLASSES+=         inner",
-		"SUBST_STAGE.inner=      post-configure",
-		"SUBST_FILES.inner=      inner.txt",
-		"SUBST_VARS.inner=       INNER",
-		".endif",
-		"SUBST_VARS.outer=       OUTER")
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:5: Incomplete SUBST block: "+
-			"SUBST_SED.outer, SUBST_VARS.outer or SUBST_FILTER_CMD.outer missing.",
-		"WARN: filename.mk:5: Subst block \"outer\" should be finished "+
-			"before adding the next class to SUBST_CLASSES.",
-		"WARN: filename.mk:10: "+
-			"Late additions to a SUBST variable should use the += operator.")
-}
+	ctx := NewSubstContext()
 
-// It's completely valid to have several SUBST blocks in a single paragraph.
-// As soon as a SUBST_CLASSES line appears, pkglint assumes that all previous
-// SUBST blocks are finished. That's exactly the case here.
-func (s *Suite) Test_SubstContext_directive__conditionally_following_block(c *check.C) {
-	t := s.Init(c)
+	ctx.varassign(t.NewMkLine("filename.mk", 10, "PKGNAME=pkgname-1.0"))
+	ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES+=p"))
+	ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_FILES.p=Makefile"))
+	ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_SED.p=s,@PREFIX@,${PREFIX},g"))
 
-	t.RunSubst(
-		"SUBST_CLASSES+=         outer",
-		"SUBST_STAGE.outer=      post-configure",
-		"SUBST_FILES.outer=      outer.txt",
-		"SUBST_VARS.outer=       OUTER",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_CLASSES+=         middle",
-		"SUBST_STAGE.middle=     post-configure",
-		"SUBST_FILES.middle=     inner.txt",
-		"SUBST_VARS.middle=      INNER",
-		".  if ${MACHINE_ARCH} == amd64",
-		"SUBST_CLASSES+=         inner",
-		"SUBST_STAGE.inner=      post-configure",
-		"SUBST_FILES.inner=      inner.txt",
-		"SUBST_VARS.inner=       INNER",
-		".  endif",
-		".endif")
+	t.CheckEquals(ctx.block().isComplete(), false)
 
-	t.CheckOutputEmpty()
-}
+	ctx.varassign(t.NewMkLine("filename.mk", 14, "SUBST_STAGE.p=post-configure"))
 
-func (s *Suite) Test_SubstContext_directive__two_blocks_in_condition(c *check.C) {
-	t := s.Init(c)
+	t.CheckEquals(ctx.block().isComplete(), true)
 
-	t.RunSubst(
-		".if ${OPSYS} == NetBSD",
-		"SUBST_CLASSES+= a",
-		"SUBST_STAGE.a=  post-configure",
-		"SUBST_FILES.a=  outer.txt",
-		"SUBST_VARS.a=   OUTER",
-		"SUBST_CLASSES+= b",
-		"SUBST_STAGE.b=  post-configure",
-		"SUBST_FILES.b=  inner.txt",
-		"SUBST_VARS.b=   INNER",
-		".endif")
+	ctx.Finish(t.NewMkLine("filename.mk", 15, ""))
 
-	// Up to 2019-12-12, pkglint wrongly warned in filename.mk:6:
-	//  Subst block "a" should be finished before adding
-	//  the next class to SUBST_CLASSES.
-	// The warning was wrong since block "a" has all required fields set.
-	// The warning was caused by an inconsistent check whether the current
-	// block had any conditional variables.
-	t.CheckOutputEmpty()
+	t.CheckOutputLines(
+		"NOTE: filename.mk:13: The substitution command \"s,@PREFIX@,${PREFIX},g\" " +
+			"can be replaced with \"SUBST_VARS.p= PREFIX\".")
 }
 
-func (s *Suite) Test_SubstContext_directive__nested_conditional_incomplete_block(c *check.C) {
+func (s *Suite) Test_substBlock_finish__conditional_inside_then(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=         outer",
-		"SUBST_STAGE.outer=      post-configure",
-		"SUBST_FILES.outer=      outer.txt",
-		"SUBST_VARS.outer=       OUTER",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_CLASSES+=         inner1",
-		"SUBST_STAGE.inner1=     post-configure",
-		"SUBST_VARS.inner1=      INNER",
-		"SUBST_CLASSES+=         inner2",
-		"SUBST_STAGE.inner2=     post-configure",
-		"SUBST_FILES.inner2=     inner.txt",
-		"SUBST_VARS.inner2=      INNER",
+		".if ${OPSYS} == Linux",
+		"SUBST_CLASSES+=\tid",
+		"SUBST_STAGE.id=\tpre-configure",
+		"SUBST_SED.id=\t-e sahara",
+		".else",
 		".endif")
 
+	// The block already ends at the .else, not at the end of the file,
+	// since that is the scope where the SUBST id is defined.
 	t.CheckOutputLines(
-		"WARN: filename.mk:9: Incomplete SUBST block: SUBST_FILES.inner1 missing.",
-		"WARN: filename.mk:9: Subst block \"inner1\" should be finished "+
-			"before adding the next class to SUBST_CLASSES.")
+		"WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.id missing.")
 }
 
-func (s *Suite) Test_SubstContext_finishClass__details_in_then_branch(c *check.C) {
+func (s *Suite) Test_substBlock_finish__conditional_inside_else(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=         os",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_VARS.os=          OPSYS",
-		"SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
-		"SUBST_STAGE.os=         post-configure",
-		"SUBST_MESSAGE.os=       Guessing operating system",
-		"SUBST_FILES.os=         guess-os.h",
+		".if ${OPSYS} == Linux",
+		".else",
+		"SUBST_CLASSES+=\tid",
+		"SUBST_STAGE.id=\tpre-configure",
+		"SUBST_SED.id=\t-e sahara",
 		".endif")
 
+	// The block already ends at the .endif, not at the end of the file,
+	// since that is the scope where the SUBST id is defined.
 	t.CheckOutputLines(
-		"WARN: filename.mk:5: SUBST_STAGE.os should not be defined conditionally.",
-		"WARN: filename.mk:6: SUBST_MESSAGE.os should not be defined conditionally.",
-		"WARN: filename.mk:EOF: Missing SUBST block for \"os\".")
+		"WARN: filename.mk:6: Incomplete SUBST block: SUBST_FILES.id missing.")
 }
 
-func (s *Suite) Test_SubstContext_finishClass__details_in_else_branch(c *check.C) {
+func (s *Suite) Test_substBlock_finish__files_missing(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=         os",
-		".if ${OPSYS} == NetBSD",
-		".else",
-		"SUBST_VARS.os=          OPSYS",
-		"SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
-		"SUBST_STAGE.os=         post-configure",
-		"SUBST_MESSAGE.os=       Guessing operating system",
-		"SUBST_FILES.os=         guess-os.h",
-		".endif")
+		"SUBST_CLASSES+=         one",
+		"SUBST_STAGE.one=        pre-configure",
+		"SUBST_CLASSES+=         two",
+		"SUBST_STAGE.two=        pre-configure",
+		"SUBST_FILES.two=        two.txt",
+		"SUBST_SED.two=          s,two,2,g")
 
 	t.CheckOutputLines(
-		"WARN: filename.mk:6: SUBST_STAGE.os should not be defined conditionally.",
-		"WARN: filename.mk:7: SUBST_MESSAGE.os should not be defined conditionally.",
-		"WARN: filename.mk:EOF: Missing SUBST block for \"os\".")
+		"WARN: filename.mk:3: Subst block \"one\" should be finished "+
+			"before adding the next class to SUBST_CLASSES.",
+		"WARN: filename.mk:3: Incomplete SUBST block: SUBST_FILES.one missing.",
+		"WARN: filename.mk:3: Incomplete SUBST block: "+
+			"SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.")
 }
 
-func (s *Suite) Test_SubstContext_finishClass__empty_conditional_at_end(c *check.C) {
+// Variables mentioned in SUBST_VARS may appear in the same paragraph,
+// or alternatively anywhere else in the file.
+func (s *Suite) Test_substBlock_checkForeignVariables__in_next_paragraph(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=         os",
-		"SUBST_VARS.os=          OPSYS",
-		"SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
-		"SUBST_STAGE.os=         post-configure",
-		"SUBST_MESSAGE.os=       Guessing operating system",
-		"SUBST_FILES.os=         guess-os.h",
-		".if ${OPSYS} == NetBSD",
-		".else",
-		".endif")
+		"SUBST_CLASSES+=\tos",
+		"SUBST_STAGE.os=\tpre-configure",
+		"SUBST_FILES.os=\tguess-os.h",
+		"SUBST_VARS.os=\tTODAY1",
+		"",
+		"TODAY1!=\tdate",
+		"TODAY2!=\tdate")
 
 	t.CheckOutputEmpty()
 }
 
-func (s *Suite) Test_SubstContext_finishClass__missing_transformation_in_one_branch(c *check.C) {
+func (s *Suite) Test_substBlock_checkForeignVariables__mixed_separate(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=         os",
-		"SUBST_STAGE.os=         post-configure",
-		"SUBST_MESSAGE.os=       Guessing operating system",
-		"SUBST_FILES.os=         guess-os.h",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_FILES.os=         -e s,@OpSYS@,NetBSD,", // A simple typo, this should be SUBST_SED.
-		".elif ${OPSYS} == Darwin",
-		"SUBST_SED.os=           -e s,@OPSYS@,Darwin1,",
-		"SUBST_SED.os=           -e s,@OPSYS@,Darwin2,",
-		".else",
-		"SUBST_VARS.os=           OPSYS",
-		".endif")
+		"SUBST_CLASSES+= 1",
+		"SUBST_STAGE.1=  post-configure",
+		"SUBST_FILES.1=  files",
+		"",
+		"SUBST_VARS.1=   VAR",
+		"USE_TOOLS+=     gmake")
 
-	t.CheckOutputLines(
-		"WARN: filename.mk:6: All but the first assignment "+
-			"to \"SUBST_FILES.os\" should use the \"+=\" operator.",
-		"WARN: filename.mk:9: All but the first assignment "+
-			"to \"SUBST_SED.os\" should use the \"+=\" operator.",
-		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.os, "+
-			"SUBST_VARS.os or SUBST_FILTER_CMD.os missing.")
+	// The USE_TOOLS is not in the SUBST block anymore since there is
+	// an empty line between SUBST_CLASSES and SUBST_VARS.
+	t.CheckOutputEmpty()
 }
 
-func (s *Suite) Test_SubstContext_finishClass__nested_conditionals(c *check.C) {
+// Variables mentioned in SUBST_VARS are not considered "foreign"
+// in the block and may be mixed with the other SUBST variables.
+func (s *Suite) Test_substBlock_checkForeignVariables__in_block(c *check.C) {
 	t := s.Init(c)
 
 	t.RunSubst(
-		"SUBST_CLASSES+=         os",
-		"SUBST_STAGE.os=         post-configure",
-		"SUBST_MESSAGE.os=       Guessing operating system",
-		".if ${OPSYS} == NetBSD",
-		"SUBST_FILES.os=         guess-netbsd.h",
-		".  if ${ARCH} == i386",
-		"SUBST_FILTER_CMD.os=    ${SED} -e s,@OPSYS,NetBSD-i386,",
-		".  elif ${ARCH} == x86_64",
-		"SUBST_VARS.os=          OPSYS",
-		".  else",
-		"SUBST_SED.os=           -e s,@OPSYS,NetBSD-unknown",
-		".  endif",
-		".else",
-		// This branch omits SUBST_FILES.
-		"SUBST_SED.os=           -e s,@OPSYS@,unknown,",
-		".endif")
-
-	t.CheckOutputLines(
-		"WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.")
-}
-
-func (s *Suite) Test_SubstContext_isComplete__incomplete(c *check.C) {
-	t := s.Init(c)
-
-	ctx := NewSubstContext()
-
-	ctx.varassign(t.NewMkLine("filename.mk", 10, "PKGNAME=pkgname-1.0"))
-
-	t.CheckEquals(ctx.id, "")
-
-	ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES+=interp"))
-
-	t.CheckEquals(ctx.id, "interp")
-
-	ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_FILES.interp=Makefile"))
-
-	t.CheckEquals(ctx.isComplete(), false)
-
-	ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_SED.interp=s,@PREFIX@,${PREFIX},g"))
-
-	t.CheckEquals(ctx.isComplete(), false)
-
-	ctx.Finish(t.NewMkLine("filename.mk", 14, ""))
-
-	t.CheckOutputLines(
-		"NOTE: filename.mk:13: The substitution command \"s,@PREFIX@,${PREFIX},g\" "+
-			"can be replaced with \"SUBST_VARS.interp= PREFIX\".",
-		"WARN: filename.mk:14: Incomplete SUBST block: SUBST_STAGE.interp missing.")
-}
-
-func (s *Suite) Test_SubstContext_isComplete__complete(c *check.C) {
-	t := s.Init(c)
-
-	ctx := NewSubstContext()
-
-	ctx.varassign(t.NewMkLine("filename.mk", 10, "PKGNAME=pkgname-1.0"))
-	ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES+=p"))
-	ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_FILES.p=Makefile"))
-	ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_SED.p=s,@PREFIX@,${PREFIX},g"))
-
-	t.CheckEquals(ctx.isComplete(), false)
-
-	ctx.varassign(t.NewMkLine("filename.mk", 14, "SUBST_STAGE.p=post-configure"))
-
-	t.CheckEquals(ctx.isComplete(), true)
-
-	ctx.Finish(t.NewMkLine("filename.mk", 15, ""))
+		"SUBST_CLASSES+=\tos",
+		"SUBST_STAGE.os=\tpre-configure",
+		"SUBST_FILES.os=\tguess-os.h",
+		"SUBST_VARS.os=\tTODAY1",
+		"TODAY1!=\tdate",
+		"TODAY2!=\tdate")
 
 	t.CheckOutputLines(
-		"NOTE: filename.mk:13: The substitution command \"s,@PREFIX@,${PREFIX},g\" " +
-			"can be replaced with \"SUBST_VARS.p= PREFIX\".")
+		"WARN: filename.mk:6: Foreign variable \"TODAY2\" in SUBST block.")
 }
diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go
index 568f46c7a86..df471079ffd 100644
--- a/pkgtools/pkglint/files/vardefs.go
+++ b/pkgtools/pkglint/files/vardefs.go
@@ -518,7 +518,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) {
 		"openjdk8 oracle-jdk8 openjdk7 sun-jdk7 jdk16 jdk15 kaffe",
 		"_PKG_JVMS.*")
 
-	platforms := reg.enumFromFiles(src,
+	operatingSystems := reg.enumFromFiles(src,
 		"mk/platform",
 		`(.*)\.mk$`, "$1",
 		"Cygwin DragonFly FreeBSD Linux NetBSD SunOS")
@@ -1362,7 +1362,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) {
 	reg.pkglistrat("ONLY_FOR_COMPILER", compilers)
 	reg.pkglistrat("ONLY_FOR_PLATFORM", BtMachinePlatformPattern)
 	reg.pkgrat("ONLY_FOR_UNPRIVILEGED", BtYesNo)
-	reg.sysload("OPSYS", platforms, DefinedIfInScope|NonemptyIfDefined)
+	reg.sysload("OPSYS", operatingSystems, DefinedIfInScope|NonemptyIfDefined)
 	reg.pkglistbl3("OPSYSVARS", BtVariableName)
 	reg.pkg("OSVERSION_SPECIFIC", BtYes)
 	reg.sysload("OS_VARIANT", BtIdentifierDirect, DefinedIfInScope)
diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go
index 2bd90c889d6..f489251d4ef 100644
--- a/pkgtools/pkglint/files/vartype.go
+++ b/pkgtools/pkglint/files/vartype.go
@@ -467,7 +467,6 @@ var (
 	BtYesNo                  = &BasicType{"YesNo", (*VartypeCheck).YesNo}
 	BtYesNoIndirectly        = &BasicType{"YesNoIndirectly", (*VartypeCheck).YesNoIndirectly}
 
-	BtMachineOpsys            = enumFromValues(machineOpsysValues)
 	BtMachineArch             = enumFromValues(machineArchValues)
 	BtMachineGnuArch          = enumFromValues(machineGnuArchValues)
 	BtEmulOpsys               = enumFromValues(emulOpsysValues)
@@ -490,10 +489,6 @@ func init() {
 // TODO: Move these values to VarTypeRegistry.Init and read them from the
 //  pkgsrc infrastructure files, as far as possible.
 const (
-	machineOpsysValues = "" + // See mk/platform
-		"AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD " +
-		"HPUX Haiku IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare"
-
 	// See mk/emulator/emulator-vars.mk.
 	emulOpsysValues = "" +
 		"bitrig bsdos cygwin darwin dragonfly freebsd " +
diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go
index 399c82253f5..d2a4f407472 100644
--- a/pkgtools/pkglint/files/vartypecheck.go
+++ b/pkgtools/pkglint/files/vartypecheck.go
@@ -799,8 +799,14 @@ func (cv *VartypeCheck) MachinePlatformPattern() {
 	}
 
 	if m, opsysPattern, versionPattern, archPattern := match3(pattern, reTriple); m {
+		// This is cheated, but the dynamically loaded enums are not
+		// provided in a type registry where they could be looked up
+		// by a name. The following line therefore assumes that OPSYS
+		// is an operating system name, which sounds like a safe bet.
+		btOpsys := G.Pkgsrc.vartypes.types["OPSYS"].basicType
+
 		opsysCv := cv.WithVarnameValueMatch("the operating system part of "+cv.Varname, opsysPattern)
-		BtMachineOpsys.checker(opsysCv)
+		btOpsys.checker(opsysCv)
 
 		versionCv := cv.WithVarnameValueMatch("the version part of "+cv.Varname, versionPattern)
 		versionCv.Version()
diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go
index da05d89357b..d80ecdd6153 100644
--- a/pkgtools/pkglint/files/vartypecheck_test.go
+++ b/pkgtools/pkglint/files/vartypecheck_test.go
@@ -1176,8 +1176,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatform(c *check.C) {
 	vt.Output(
 		"WARN: filename.mk:1: \"linux-i386\" is not a valid platform pattern.",
 		"WARN: filename.mk:2: The pattern \"nextbsd\" cannot match any of "+
-			"{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+
-			"IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+
+			"{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+
 			"} for the operating system part of MACHINE_PLATFORM.",
 		"WARN: filename.mk:2: The pattern \"8087\" cannot match any of "+
 			"{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+
@@ -1191,8 +1190,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatform(c *check.C) {
 			"rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+
 			"} for the hardware architecture part of MACHINE_PLATFORM.",
 		"WARN: filename.mk:3: The pattern \"netbsd\" cannot match any of "+
-			"{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+
-			"IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+
+			"{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+
 			"} for the operating system part of MACHINE_PLATFORM.",
 		"WARN: filename.mk:3: The pattern \"l*\" cannot match any of "+
 			"{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+
@@ -1227,8 +1225,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatformPattern(c *check.C) {
 	vt.Output(
 		"WARN: filename.mk:1: \"linux-i386\" is not a valid platform pattern.",
 		"WARN: filename.mk:2: The pattern \"nextbsd\" cannot match any of "+
-			"{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+
-			"IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+
+			"{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+
 			"} for the operating system part of ONLY_FOR_PLATFORM.",
 		"WARN: filename.mk:2: The pattern \"8087\" cannot match any of "+
 			"{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+
@@ -1242,8 +1239,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatformPattern(c *check.C) {
 			"rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+
 			"} for the hardware architecture part of ONLY_FOR_PLATFORM.",
 		"WARN: filename.mk:3: The pattern \"netbsd\" cannot match any of "+
-			"{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+
-			"IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+
+			"{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+
 			"} for the operating system part of ONLY_FOR_PLATFORM.",
 		"WARN: filename.mk:3: The pattern \"l*\" cannot match any of "+
 			"{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+