Received: by mail.netbsd.org (Postfix, from userid 605) id E176484D43; Mon, 16 Dec 2019 17:06:10 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mail.netbsd.org (Postfix) with ESMTP id 6758C84D41 for ; Mon, 16 Dec 2019 17:06:10 +0000 (UTC) X-Virus-Scanned: amavisd-new at netbsd.org Received: from mail.netbsd.org ([IPv6:::1]) by localhost (mail.netbsd.org [IPv6:::1]) (amavisd-new, port 10025) with ESMTP id EpKRR6CxFvCI for ; Mon, 16 Dec 2019 17:06:05 +0000 (UTC) Received: from cvs.NetBSD.org (ivanova.netbsd.org [199.233.217.197]) by mail.netbsd.org (Postfix) with ESMTP id A1B5A84CFC for ; Mon, 16 Dec 2019 17:06:05 +0000 (UTC) Received: by cvs.NetBSD.org (Postfix, from userid 500) id 9584EFA97; Mon, 16 Dec 2019 17:06:05 +0000 (UTC) Content-Transfer-Encoding: 7bit Content-Type: multipart/mixed; boundary="_----------=_157651596543580" MIME-Version: 1.0 Date: Mon, 16 Dec 2019 17:06:05 +0000 From: "Roland Illig" Subject: CVS commit: pkgsrc/pkgtools/pkglint To: pkgsrc-changes@NetBSD.org Reply-To: rillig@netbsd.org X-Mailer: log_accum Message-Id: <20191216170605.9584EFA97@cvs.NetBSD.org> Sender: pkgsrc-changes-owner@NetBSD.org List-Id: pkgsrc-changes.NetBSD.org Precedence: bulk List-Unsubscribe: This is a multi-part message in MIME format. --_----------=_157651596543580 Content-Disposition: inline Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII" Module Name: pkgsrc Committed By: rillig Date: Mon Dec 16 17:06:05 UTC 2019 Modified Files: pkgsrc/pkgtools/pkglint: Makefile pkgsrc/pkgtools/pkglint/files: autofix.go autofix_test.go check_test.go logging.go mklinechecker.go mklinechecker_test.go mktypes.go mktypes_test.go package.go substcontext.go substcontext_test.go util.go util_test.go varalignblock.go varalignblock_test.go vartypecheck.go vartypecheck_test.go Log Message: pkgtools/pkglint: update to 19.3.19 Changes since 19.3.18: Small improvements to edge cases in SUBST blocks. Small improvements to edge cases in variable assignment and alignment of the continuation backslashes. The --source option shows the changes from autofix, even when the --show-autofix option is not given. This is a welcome side-effect of making the autofix logging simpler and more predictable. To generate a diff of this commit: cvs rdiff -u -r1.619 -r1.620 pkgsrc/pkgtools/pkglint/Makefile cvs rdiff -u -r1.34 -r1.35 pkgsrc/pkgtools/pkglint/files/autofix.go \ pkgsrc/pkgtools/pkglint/files/substcontext.go cvs rdiff -u -r1.35 -r1.36 pkgsrc/pkgtools/pkglint/files/autofix_test.go \ pkgsrc/pkgtools/pkglint/files/logging.go cvs rdiff -u -r1.60 -r1.61 pkgsrc/pkgtools/pkglint/files/check_test.go cvs rdiff -u -r1.58 -r1.59 pkgsrc/pkgtools/pkglint/files/mklinechecker.go cvs rdiff -u -r1.53 -r1.54 \ pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go cvs rdiff -u -r1.23 -r1.24 pkgsrc/pkgtools/pkglint/files/mktypes.go cvs rdiff -u -r1.21 -r1.22 pkgsrc/pkgtools/pkglint/files/mktypes_test.go cvs rdiff -u -r1.74 -r1.75 pkgsrc/pkgtools/pkglint/files/package.go \ pkgsrc/pkgtools/pkglint/files/vartypecheck.go cvs rdiff -u -r1.33 -r1.34 pkgsrc/pkgtools/pkglint/files/substcontext_test.go cvs rdiff -u -r1.66 -r1.67 pkgsrc/pkgtools/pkglint/files/util.go cvs rdiff -u -r1.43 -r1.44 pkgsrc/pkgtools/pkglint/files/util_test.go cvs rdiff -u -r1.12 -r1.13 pkgsrc/pkgtools/pkglint/files/varalignblock.go cvs rdiff -u -r1.8 -r1.9 pkgsrc/pkgtools/pkglint/files/varalignblock_test.go cvs rdiff -u -r1.67 -r1.68 pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. --_----------=_157651596543580 Content-Disposition: inline Content-Length: 92722 Content-Transfer-Encoding: binary Content-Type: text/x-diff; charset=us-ascii Modified files: Index: pkgsrc/pkgtools/pkglint/Makefile diff -u pkgsrc/pkgtools/pkglint/Makefile:1.619 pkgsrc/pkgtools/pkglint/Makefile:1.620 --- pkgsrc/pkgtools/pkglint/Makefile:1.619 Sat Dec 14 18:04:15 2019 +++ pkgsrc/pkgtools/pkglint/Makefile Mon Dec 16 17:06:05 2019 @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.619 2019/12/14 18:04:15 rillig Exp $ +# $NetBSD: Makefile,v 1.620 2019/12/16 17:06:05 rillig Exp $ -PKGNAME= pkglint-19.3.18 +PKGNAME= pkglint-19.3.19 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} Index: pkgsrc/pkgtools/pkglint/files/autofix.go diff -u pkgsrc/pkgtools/pkglint/files/autofix.go:1.34 pkgsrc/pkgtools/pkglint/files/autofix.go:1.35 --- pkgsrc/pkgtools/pkglint/files/autofix.go:1.34 Sun Dec 8 00:06:38 2019 +++ pkgsrc/pkgtools/pkglint/files/autofix.go Mon Dec 16 17:06:05 2019 @@ -21,8 +21,7 @@ type Autofix struct { line *Line linesBefore []string // Newly inserted lines, including \n linesAfter []string // Newly inserted lines, including \n - // Whether an actual fix has been applied (or, without --show-autofix, - // whether a fix is applicable) + // Whether an actual fix has been applied to the text of the raw lines modified bool autofixShortTerm @@ -30,11 +29,18 @@ type Autofix struct { // autofixShortTerm is the part of the Autofix that is reset after each call to Apply. type autofixShortTerm struct { - actions []autofixAction // Human-readable description of the actual autofix actions - level *LogLevel // - diagFormat string // Is logged only if it couldn't be fixed automatically - diagArgs []interface{} // - explanation []string // Is printed together with the diagnostic + // Human-readable description of the actual autofix actions. + // There can be more than one action in cases where a follow-up + // fix is necessary. + actions []autofixAction + + // The diagnostic to be logged. + // It is subject to the --only command line option. + // In --autofix mode it is suppressed if there were no actual actions. + level *LogLevel + diagFormat string + diagArgs []interface{} + explanation []string } type autofixAction struct { @@ -49,11 +55,11 @@ type autofixAction struct { // to log a diagnostic by other means. const SilentAutofixFormat = "SilentAutofixFormat" -// AutofixFormat is a special value that is used for logging +// autofixFormat is a special value that is used for logging // diagnostics like "Replacing \"old\" with \"new\".". // // Since these are not really diagnostics, duplicates are not suppressed. -const AutofixFormat = "AutofixFormat" +const autofixFormat = "AutofixFormat" func NewAutofix(line *Line) *Autofix { return &Autofix{line: line} @@ -77,20 +83,22 @@ func (fix *Autofix) Notef(format string, // Explain remembers the explanation for logging it later when Apply is called. func (fix *Autofix) Explain(explanation ...string) { // Since a silent fix doesn't have a diagnostic, its explanation would - // not provide any clue as to what diagnostic it belongs. That would - // be confusing, therefore this case is not allowed. + // not provide any clue as to what diagnostic it belongs. + // That would be confusing, therefore this case is not allowed. assert(fix.diagFormat != SilentAutofixFormat) fix.explanation = explanation } // Replace replaces "from" with "to", a single time. +// If the text is not found exactly once, nothing is replaced at all. func (fix *Autofix) Replace(from string, to string) { fix.ReplaceAfter("", from, to) } // ReplaceAfter replaces the text "prefix+from" with "prefix+to", a single time. // In the diagnostic, only the replacement of "from" with "to" is mentioned. +// If the text is not found exactly once, nothing is replaced at all. func (fix *Autofix) ReplaceAfter(prefix, from string, to string) { fix.assertRealLine() if fix.skip() { @@ -121,6 +129,9 @@ func (fix *Autofix) ReplaceAfter(prefix, // TODO: Do this properly by parsing the whole line again, // and ideally everything that depends on the parsed line. // This probably requires a generic notification mechanism. + // + // FIXME: Only actually update fix.line.Text if the replacement + // has been done exactly once; see ReplaceAt. fix.line.Text = strings.Replace(fix.line.Text, prefixFrom, prefixTo, 1) } fix.Describef(rawLine.Lineno, "Replacing %q with %q.", from, to) @@ -130,38 +141,38 @@ func (fix *Autofix) ReplaceAfter(prefix, } // ReplaceAt replaces the text "from" with "to", a single time. -// But only if the text at the given position is indeed "from". -func (fix *Autofix) ReplaceAt(rawIndex int, textIndex int, from string, to string) (modified bool, replaced string) { +// If the text at the given position does not match, ReplaceAt panics. +func (fix *Autofix) ReplaceAt(rawIndex int, textIndex int, from string, to string) { assert(from != to) fix.assertRealLine() + // XXX: This should only affect the diagnostics, but not the modifications + // to the text of the affected line, since that text will be used in + // further checks. if fix.skip() { - return false, "" + return } rawLine := fix.line.raw[rawIndex] - if textIndex >= len(rawLine.textnl) || !hasPrefix(rawLine.textnl[textIndex:], from) { - return false, "" - } + assert(textIndex < len(rawLine.textnl)) + assert(hasPrefix(rawLine.textnl[textIndex:], from)) - replaced = rawLine.textnl[:textIndex] + to + rawLine.textnl[textIndex+len(from):] + replaced := rawLine.textnl[:textIndex] + to + rawLine.textnl[textIndex+len(from):] - if G.Logger.IsAutofix() { - rawLine.textnl = replaced + rawLine.textnl = replaced - // Fix the parsed text as well. - // This is only approximate and won't work in some edge cases - // that involve escaped comments or replacements across line breaks. - // - // TODO: Do this properly by parsing the whole line again, - // and ideally everything that depends on the parsed line. - // This probably requires a generic notification mechanism. - if strings.Count(fix.line.Text, from) == 1 { - fix.line.Text = strings.Replace(fix.line.Text, from, to, 1) - } + // Fix the parsed text as well. + // This is only approximate and won't work in some edge cases + // that involve escaped comments or replacements across line breaks. + // + // TODO: Do this properly by parsing the whole line again, + // and ideally everything that depends on the parsed line. + // This probably requires a generic notification mechanism. + if strings.Count(fix.line.Text, from) == 1 { + fix.line.Text = strings.Replace(fix.line.Text, from, to, 1) } + fix.Describef(rawLine.Lineno, "Replacing %q with %q.", from, to) - return true, replaced } // ReplaceRegex replaces the first howOften or all occurrences (if negative) @@ -206,6 +217,9 @@ func (fix *Autofix) ReplaceRegex(from re // TODO: Do this properly by parsing the whole line again, // and ideally everything that depends on the parsed line. // This probably requires a generic notification mechanism. + // + // FIXME: Only actually update fix.line.Text if the replacement + // has been done exactly once. done = 0 fix.line.Text = replaceAllFunc( fix.line.Text, @@ -270,19 +284,18 @@ func (fix *Autofix) Delete() { // The fixer function must check whether it can actually fix something, // and if so, call Describef to describe the actual fix. // -// If showAutofix and autofix are both false, the fix must only be -// described by calling Describef. No observable modification must be done, -// not even in memory. -// -// If showAutofix is true but autofix is false, the fix should be done in -// memory as far as possible. For example, changing the text of Line.raw -// is appropriate, but changing files in the file system is not. +// If autofix is false, the the fix should be applied, as far as only +// in-memory data structures are effected, and these are not written +// back to disk. No externally observable modification must be done. +// For example, changing the text of Line.raw is appropriate, +// but changing files in the file system is not. // // Only if autofix is true, fixes other than modifying the current Line // should be done persistently, such as changes to the file system. // -// In any case, changes to the current Line will be written back to disk -// by SaveAutofixChanges, after fixing all the lines in the file at once. +// If pkglint is run in --autofix mode, all changes to the lines of a +// file will be collected in memory and are written back to disk by +// SaveAutofixChanges, once at the end. func (fix *Autofix) Custom(fixer func(showAutofix, autofix bool)) { // Contrary to the fixes that modify the line text, this one // can be run even on dummy lines (like those standing for a @@ -295,21 +308,29 @@ func (fix *Autofix) Custom(fixer func(sh fixer(G.Logger.Opts.ShowAutofix, G.Logger.Opts.Autofix) } -// Describef is used while Autofix.Custom is called to remember a description -// of the actual fix for logging it later when Apply is called. +// Describef can be called from within an Autofix.Custom call to remember a +// description of the actual fix for logging it later when Apply is called. // Describef may be called multiple times before calling Apply. func (fix *Autofix) Describef(lineno int, format string, args ...interface{}) { fix.actions = append(fix.actions, autofixAction{sprintf(format, args...), lineno}) } -// Apply does the actual work. -// Depending on the pkglint mode, it either: +// Apply does the actual work that has been prepared by previous calls to +// Errorf, Warnf, Notef, Describef, Replace, Delete and so on. // -// * logs the associated message (default) but does not record the fixes in the line -// -// * logs what would be fixed (--show-autofix) and records the fixes in the line -// -// * records the fixes in the line (--autofix), ready for SaveAutofixChanges +// In default mode, the diagnostic is logged even when nothing has actually +// been fixed. This frees the calling code from distinguishing the cases where +// a fix can or cannot be applied automatically. +// +// In --show-autofix mode, only those diagnostics are logged that actually fix +// something. This is done to hide possibly distracting, unrelated diagnostics. +// +// In --autofix mode, only the actual changes are logged, but not the +// corresponding diagnostics. To get both, specify --show-autofix as well. +// +// Apply does the modifications only in memory. To actually save them to disk, +// SaveAutofixChanges needs to be called. For example, this is done by +// MkLines.Check. func (fix *Autofix) Apply() { line := fix.line @@ -356,7 +377,7 @@ func (fix *Autofix) Apply() { if action.lineno != 0 { lineno = strconv.Itoa(action.lineno) } - G.Logger.Logf(AutofixLogLevel, line.Filename, lineno, AutofixFormat, action.description) + G.Logger.Logf(AutofixLogLevel, line.Filename, lineno, autofixFormat, action.description) } G.Logger.showSource(line) } @@ -416,18 +437,21 @@ func (fix *Autofix) affectedLinenos() st func (fix *Autofix) skip() bool { assert(fix.diagFormat != "") // The diagnostic must be given before the action. - // This check is necessary for the --only command line option. return !G.Logger.shallBeLogged(fix.diagFormat) } func (fix *Autofix) assertRealLine() { - assert(fix.line.firstLine >= 1) // Cannot autofix this line since it is not a real line. + // Some Line objects do not correspond to real lines of a file. + // These cannot be fixed since they are neither part of Lines nor of MkLines. + assert(fix.line.firstLine >= 1) } // SaveAutofixChanges writes the given lines back into their files, // applying the autofix changes. // The lines may come from different files. // Only files that actually have changed lines are saved. +// +// This only happens in --autofix mode. func SaveAutofixChanges(lines *Lines) (autofixed bool) { if trace.Tracing { defer trace.Call0()() @@ -450,6 +474,14 @@ func SaveAutofixChanges(lines *Lines) (a if G.Testing { abs := G.Abs(lines.Filename) absTmp := G.Abs(NewCurrPathSlash(os.TempDir())) + + // This assertion prevents the pkglint tests from overwriting files + // on disk. This can easily happen if a test creates Lines or MkLines + // using a relative path. + // + // By default, these paths are relative to the current working + // directory. To let them refer to the temporary directory used by + // the tests, call t.Chdir("."). assertf(abs.HasPrefixPath(absTmp), "%q must be inside %q", abs, absTmp) } Index: pkgsrc/pkgtools/pkglint/files/substcontext.go diff -u pkgsrc/pkgtools/pkglint/files/substcontext.go:1.34 pkgsrc/pkgtools/pkglint/files/substcontext.go:1.35 --- pkgsrc/pkgtools/pkglint/files/substcontext.go:1.34 Sat Dec 14 18:04:15 2019 +++ pkgsrc/pkgtools/pkglint/files/substcontext.go Mon Dec 16 17:06:05 2019 @@ -66,27 +66,14 @@ func (ctx *SubstContext) varassign(mklin } } - if hasPrefix(mkline.Varname(), "SUBST_") && mkline.Varparam() != ctx.activeId() { + if mkline.Varparam() != ctx.activeId() { if !ctx.varassignDifferentClass(mkline) { return } } block := ctx.block() - switch varcanon { - case "SUBST_STAGE.*": - block.varassignStage(mkline) - case "SUBST_MESSAGE.*": - block.varassignMessages(mkline) - case "SUBST_FILES.*": - block.varassignFiles(mkline) - case "SUBST_SED.*": - block.varassignSed(mkline) - case "SUBST_VARS.*": - block.varassignVars(mkline) - case "SUBST_FILTER_CMD.*": - block.varassignFilterCmd(mkline) - } + block.varassign(mkline) } func (ctx *SubstContext) varassignClasses(mkline *MkLine) { @@ -138,7 +125,7 @@ func (ctx *SubstContext) varassignOutsid func (ctx *SubstContext) varassignDifferentClass(mkline *MkLine) (ok bool) { varname := mkline.Varname() unknown := ctx.lookup(mkline.Varparam()) == nil - if unknown && ctx.isActive() && !ctx.block().isComplete() { + if unknown && !ctx.block().isComplete() { mkline.Warnf("Variable %q does not match SUBST class %q.", varname, ctx.activeId()) if !ctx.block().seenEmpty { @@ -319,8 +306,22 @@ func (s *substScope) emptyLine() { // finish brings all blocks that are defined in the current scope // to an end. func (s *substScope) finish(diag Diagnoser) { + foreignOk := map[string]bool{} + for _, def := range s.defs { + for allowed := range def.foreignAllowed { + foreignOk[allowed] = true + } + } + for _, block := range s.defs { block.finish(diag) + + for _, mkline := range block.foreign { + if !foreignOk[mkline.Varname()] { + mkline.Warnf("Foreign variable %q in SUBST block.", + mkline.Varname()) + } + } } } @@ -395,6 +396,23 @@ func newSubstBlock(id string) *substBloc return &substBlock{id: id, conds: []*substCond{newSubstCond()}} } +func (b *substBlock) varassign(mkline *MkLine) { + switch mkline.Varcanon() { + case "SUBST_STAGE.*": + b.varassignStage(mkline) + case "SUBST_MESSAGE.*": + b.varassignMessages(mkline) + case "SUBST_FILES.*": + b.varassignFiles(mkline) + case "SUBST_SED.*": + b.varassignSed(mkline) + case "SUBST_VARS.*": + b.varassignVars(mkline) + default: + b.varassignFilterCmd(mkline) + } +} + func (b *substBlock) varassignStage(mkline *MkLine) { if b.isConditional() { mkline.Warnf("%s should not be defined conditionally.", mkline.Varname()) @@ -661,16 +679,6 @@ func (b *substBlock) finish(diag Diagnos "SUBST_VARS.%[1]s or SUBST_FILTER_CMD.%[1]s missing.", b.id) } - - b.checkForeignVariables() -} - -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()) - } - } } func (b *substBlock) hasStarted() bool { Index: pkgsrc/pkgtools/pkglint/files/autofix_test.go diff -u pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.35 pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.36 --- pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.35 Sun Dec 8 22:03:38 2019 +++ pkgsrc/pkgtools/pkglint/files/autofix_test.go Mon Dec 16 17:06:05 2019 @@ -555,21 +555,43 @@ func (s *Suite) Test_Autofix_ReplaceAfte func (s *Suite) Test_Autofix_ReplaceAt(c *check.C) { t := s.Init(c) + mainPart := func(texts []string, rawIndex int, column int, from, to string) { + mklines := t.NewMkLines("filename.mk", texts...) + assert(len(mklines.mklines) == 1) + mkline := mklines.mklines[0] + + fix := mkline.Autofix() + fix.Notef("Should be appended instead of assigned.") + fix.ReplaceAt(rawIndex, column, from, to) + fix.Apply() + } + lines := func(lines ...string) []string { return lines } test := func(texts []string, rawIndex int, column int, from, to string, diagnostics ...string) { + doTest := func(bool) { + mainPart(texts, rawIndex, column, from, to) + } - mainPart := func(autofix bool) { - mklines := t.NewMkLines("filename.mk", texts...) - assert(len(mklines.mklines) == 1) - mkline := mklines.mklines[0] - - fix := mkline.Autofix() - fix.Notef("Should be appended instead of assigned.") - fix.ReplaceAt(rawIndex, column, from, to) - fix.Apply() + t.ExpectDiagnosticsAutofix(doTest, diagnostics...) + } + + testAssert := func(texts []string, rawIndex int, column int, from, to string) { + doTest := func(bool) { + t.ExpectAssert( + func() { mainPart(texts, rawIndex, column, from, to) }) } - t.ExpectDiagnosticsAutofix(mainPart, diagnostics...) + t.ExpectDiagnosticsAutofix(doTest, nil...) + } + + testPanicMatches := func(texts []string, rawIndex int, column int, from, to, panicMessage string) { + doTest := func(bool) { + t.ExpectPanicMatches( + func() { mainPart(texts, rawIndex, column, from, to) }, + panicMessage) + } + + t.ExpectDiagnosticsAutofix(doTest, nil...) } test( @@ -582,35 +604,38 @@ func (s *Suite) Test_Autofix_ReplaceAt(c "AUTOFIX: filename.mk:1: Replacing \"=\" with \"+=\".") // If the text at the precisely given position does not match, - // the note is still printed, but nothing is replaced automatically. - test( + // it is a programming mistake, therefore pkglint panics. + testAssert( lines( "VAR=value1 \\", "\tvalue2"), - 0, 3, "?", "+=", - - "NOTE: filename.mk:1--2: Should be appended instead of assigned.") + 0, 3, "?", "+=") // Getting the line number wrong is a strange programming error, and // there does not need to be any code checking for this in the main code. - t.ExpectPanicMatches( - func() { test(lines("VAR=value"), 10, 3, "from", "to", nil...) }, + testPanicMatches( + lines( + "VAR=value"), + 10, 3, "from", "to", `runtime error: index out of range.*`) // It is a programming error to replace a string with itself, since that // would produce confusing diagnostics. - t.ExpectAssert( - func() { test(lines("VAR=value"), 0, 4, "value", "value", nil...) }) + testAssert( + lines( + "VAR=value"), + 0, 4, "value", "value") // Getting the column number wrong may happen when a previous replacement - // has made the string shorter than before, therefore no panic in this case. - test( + // has made the string shorter than before. + // This is a programming mistake, therefore panic. + // All fixes that work on the raw lines are supposed to work exactly and + // know what they are doing. + testAssert( lines( "VAR=value1 \\", "\tvalue2"), - 0, 20, "?", "+=", - - "NOTE: filename.mk:1--2: Should be appended instead of assigned.") + 0, 20, "?", "+=") } func (s *Suite) Test_Autofix_ReplaceRegex__show_autofix(c *check.C) { Index: pkgsrc/pkgtools/pkglint/files/logging.go diff -u pkgsrc/pkgtools/pkglint/files/logging.go:1.35 pkgsrc/pkgtools/pkglint/files/logging.go:1.36 --- pkgsrc/pkgtools/pkglint/files/logging.go:1.35 Mon Dec 9 20:38:16 2019 +++ pkgsrc/pkgtools/pkglint/files/logging.go Mon Dec 16 17:06:05 2019 @@ -268,7 +268,7 @@ func (l *Logger) Logf(level *LogLevel, f } } - if G.Testing && format != AutofixFormat { + if G.Testing && format != autofixFormat { if textproc.Alpha.Contains(format[0]) { assertf( textproc.Upper.Contains(format[0]), @@ -284,7 +284,7 @@ func (l *Logger) Logf(level *LogLevel, f if !filename.IsEmpty() { filename = filename.CleanPath() } - if G.Opts.Profiling && format != AutofixFormat && level != Fatal { + if G.Opts.Profiling && format != autofixFormat && level != Fatal { l.histo.Add(format, 1) } Index: pkgsrc/pkgtools/pkglint/files/check_test.go diff -u pkgsrc/pkgtools/pkglint/files/check_test.go:1.60 pkgsrc/pkgtools/pkglint/files/check_test.go:1.61 --- pkgsrc/pkgtools/pkglint/files/check_test.go:1.60 Fri Dec 13 01:39:23 2019 +++ pkgsrc/pkgtools/pkglint/files/check_test.go Mon Dec 16 17:06:05 2019 @@ -61,8 +61,6 @@ func (s *Suite) SetUpTest(c *check.C) { G.Testing = true trace.Out = &t.stdout - // XXX: Maybe the tests can run a bit faster when they don't - // create a temporary directory each. G.Pkgsrc = NewPkgsrc(t.File(".")) t.c = c @@ -1327,7 +1325,8 @@ func (t *Tester) CheckDotColumns(lines . width := tabWidth(prefix) num, err := strconv.Atoi(line[m[2]:m[3]]) assertNil(err, "") - t.CheckEqualsf(num, width, "lines[%d]", index) + t.CheckEqualsf(num, width, + "The dots in lines[%d] are wrong.", index) } } } Index: pkgsrc/pkgtools/pkglint/files/mklinechecker.go diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.58 pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.59 --- pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.58 Sun Dec 8 22:03:38 2019 +++ pkgsrc/pkgtools/pkglint/files/mklinechecker.go Mon Dec 16 17:06:05 2019 @@ -64,14 +64,14 @@ func (ck MkLineChecker) checkTextVarUse( tokens, _ := NewMkLexer(text, nil).MkTokens() for i, token := range tokens { - // TODO: flatten - if token.Varuse != nil { - spaceLeft := i-1 < 0 || matches(tokens[i-1].Text, `[\t ]$`) - spaceRight := i+1 >= len(tokens) || matches(tokens[i+1].Text, `^[\t ]`) - isWordPart := !(spaceLeft && spaceRight) - vuc := VarUseContext{vartype, time, VucQuotPlain, isWordPart} - NewMkVarUseChecker(token.Varuse, ck.MkLines, ck.MkLine).Check(&vuc) + if token.Varuse == nil { + continue } + spaceLeft := i-1 < 0 || matches(tokens[i-1].Text, `[\t ]$`) + spaceRight := i+1 >= len(tokens) || matches(tokens[i+1].Text, `^[\t ]`) + isWordPart := !(spaceLeft && spaceRight) + vuc := VarUseContext{vartype, time, VucQuotPlain, isWordPart} + NewMkVarUseChecker(token.Varuse, ck.MkLines, ck.MkLine).Check(&vuc) } } Index: pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.53 pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.54 --- pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.53 Sun Dec 8 22:03:38 2019 +++ pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go Mon Dec 16 17:06:05 2019 @@ -310,23 +310,24 @@ func (s *Suite) Test_MkLineChecker_check func (s *Suite) Test_MkLineChecker_checkShellCommand__indentation(c *check.C) { t := s.Init(c) - mklines := t.SetUpFileMkLines("filename.mk", - MkCvsID, - "", - "do-install:", - "\t\techo 'unnecessarily indented'", - "\t\tfor var in 1 2 3; do \\", - "\t\t\techo \"$$var\"; \\", - "\t echo \"spaces\"; \\", - "\t\tdone", - "", - "\t\t\t\t\t# comment, not a shell command") + doTest := func(bool) { + mklines := t.SetUpFileMkLines("filename.mk", + MkCvsID, + "", + "do-install:", + "\t\techo 'unnecessarily indented'", + "\t\tfor var in 1 2 3; do \\", + "\t\t\techo \"$$var\"; \\", + "\t echo \"spaces\"; \\", + "\t\tdone", + "", + "\t\t\t\t\t# comment, not a shell command") - mklines.Check() - t.SetUpCommandLine("-Wall", "--autofix") - mklines.Check() + mklines.Check() + } - t.CheckOutputLines( + t.ExpectDiagnosticsAutofix( + doTest, "NOTE: ~/filename.mk:4: Shell programs should be indented with a single tab.", "WARN: ~/filename.mk:4: Unknown shell command \"echo\".", "NOTE: ~/filename.mk:5--8: Shell programs should be indented with a single tab.", Index: pkgsrc/pkgtools/pkglint/files/mktypes.go diff -u pkgsrc/pkgtools/pkglint/files/mktypes.go:1.23 pkgsrc/pkgtools/pkglint/files/mktypes.go:1.24 --- pkgsrc/pkgtools/pkglint/files/mktypes.go:1.23 Fri Dec 13 01:39:23 2019 +++ pkgsrc/pkgtools/pkglint/files/mktypes.go Mon Dec 16 17:06:05 2019 @@ -61,8 +61,9 @@ func (m MkVarUseModifier) MatchSubst() ( // MkVarUseModifier{"S,name,file,g"}.Subst("distname-1.0") => "distfile-1.0" func (m MkVarUseModifier) Subst(str string) (string, bool) { // XXX: The call to MatchSubst is usually redundant because MatchSubst - // is typically called directly before calling Subst. - ok, regex, from, to, options := m.MatchSubst() + // is typically called directly before calling Subst. + // This comes from a time when there was no boolean return value. + ok, isRegex, from, to, options := m.MatchSubst() if !ok { return "", false } @@ -77,14 +78,14 @@ func (m MkVarUseModifier) Subst(str stri from = from[:len(from)-1] } - if regex && matches(from, `^[\w-]+$`) && matches(to, `^[^&$\\]*$`) { + if isRegex && matches(from, `^[\w-]+$`) && matches(to, `^[^&$\\]*$`) { // The "from" pattern is so simple that it doesn't matter whether // the modifier is :S or :C, therefore treat it like the simpler :S. - regex = false + isRegex = false } - if regex { - // TODO: Maybe implement regular expression substitutions later. + if isRegex { + // XXX: Maybe implement regular expression substitutions later. return "", false } Index: pkgsrc/pkgtools/pkglint/files/mktypes_test.go diff -u pkgsrc/pkgtools/pkglint/files/mktypes_test.go:1.21 pkgsrc/pkgtools/pkglint/files/mktypes_test.go:1.22 --- pkgsrc/pkgtools/pkglint/files/mktypes_test.go:1.21 Fri Dec 13 01:39:23 2019 +++ pkgsrc/pkgtools/pkglint/files/mktypes_test.go Mon Dec 16 17:06:05 2019 @@ -87,72 +87,67 @@ func (s *Suite) Test_MkVarUseModifier_Ma t.CheckEquals(options, "1") } -// As of 2019-03-24, pkglint doesn't know how to handle complicated -// :C modifiers. -func (s *Suite) Test_MkVarUseModifier_Subst__regexp(c *check.C) { +func (s *Suite) Test_MkVarUseModifier_Subst(c *check.C) { t := s.Init(c) - mod := MkVarUseModifier{"C,.*,,"} + test := func(mod, str, result string, ok bool) { + m := MkVarUseModifier{mod} - empty, ok := mod.Subst("anything") + actualResult, actualOk := m.Subst(str) - t.CheckEquals(ok, false) - t.CheckEquals(empty, "") -} - -// When given a modifier that is not actually a :S or :C, Subst -// doesn't do anything. -func (s *Suite) Test_MkVarUseModifier_Subst__invalid_argument(c *check.C) { - t := s.Init(c) - - mod := MkVarUseModifier{"Mpattern"} - - empty, ok := mod.Subst("anything") - - t.CheckEquals(ok, false) - t.CheckEquals(empty, "") -} - -func (s *Suite) Test_MkVarUseModifier_Subst__no_tracing(c *check.C) { - t := s.Init(c) - - mod := MkVarUseModifier{"S,from,to,"} - t.DisableTracing() + t.CheckDeepEquals( + []interface{}{actualResult, actualOk}, + []interface{}{result, ok}) + } - result, ok := mod.Subst("from a to b") + test("???", "anything", "", false) - t.CheckEquals(ok, true) - t.CheckEquals(result, "to a to b") -} + test("S,from,to,", "from", "to", true) -// Since the replacement text is not a simple string, the :C modifier -// cannot be treated like the :S modifier. The variable could contain -// one of the special characters that would need to be escaped in the -// replacement text. -func (s *Suite) Test_MkVarUseModifier_Subst__C_with_complex_replacement(c *check.C) { - t := s.Init(c) + test("C,from,to,", "from", "to", true) - mod := MkVarUseModifier{"C,from,${VAR},"} + test("C,syntax error", "anything", "", false) - result, ok := mod.Subst("from a to b") + // The substitution modifier does not match, therefore + // the value is returned unmodified, but successful. + test("C,no_match,replacement,", "value", "value", true) - t.CheckEquals(ok, false) - t.CheckEquals(result, "") -} + // As of December 2019, pkglint doesn't know how to handle + // complicated :C modifiers. + test("C,.*,,", "anything", "", false) -func (s *Suite) Test_MkVarUseModifier_Subst__S_dollar_at(c *check.C) { - t := s.Init(c) + // When given a modifier that is not actually a :S or :C, Subst + // doesn't do anything. + test("Mpattern", "anything", "", false) - mod := MkVarUseModifier{"S/$@/replaced/"} + test("S,from,to,", "from a to b", "to a to b", true) - result, ok := mod.Subst("The target") + // Since the replacement text is not a simple string, the :C modifier + // cannot be treated like the :S modifier. The variable could contain + // one of the special characters that would need to be escaped in the + // replacement text. + test("C,from,${VAR},", "from a to b", "", false) // As of December 2019, nothing is substituted. If pkglint should ever - // handle variables in the modifier, this test would been to provide a + // handle variables in the modifier, this test would need to provide a // context in which to resolve the variables. If that happens, the // .TARGET variable needs to be set to "target". - t.CheckEquals(ok, true) - t.CheckEquals(result, "The target") + test("S/$@/replaced/", "The target", "The target", true) + test("S,${PREFIX},/prefix,", "${PREFIX}/dir", "", false) + + // Just for code coverage. + t.DisableTracing() + test("S,long,long long,g", "A long story", "A long long story", true) + t.EnableTracing() + + // And now again with full tracing, to investigate cases where + // pkglint produces results that are not easily understandable. + t.EnableTracingToLog() + test("S,long,long long,g", "A long story", "A long long story", true) + t.EnableTracing() + t.CheckOutputLines( + "TRACE: Subst: \"A long story\" " + + "\"S,long,long long,g\" => \"A long long story\"") } func (s *Suite) Test_MkVarUseModifier_EvalSubst(c *check.C) { Index: pkgsrc/pkgtools/pkglint/files/package.go diff -u pkgsrc/pkgtools/pkglint/files/package.go:1.74 pkgsrc/pkgtools/pkglint/files/package.go:1.75 --- pkgsrc/pkgtools/pkglint/files/package.go:1.74 Sun Dec 8 22:03:38 2019 +++ pkgsrc/pkgtools/pkglint/files/package.go Mon Dec 16 17:06:05 2019 @@ -403,7 +403,7 @@ func (pkg *Package) resolveIncludedFile( if mkline.Basename != "buildlink3.mk" { if includedFile.HasSuffixPath("buildlink3.mk") { curr := mkline.File(includedFile) - if G.Pkg != nil && !curr.ContainsText("$") && !curr.IsFile() { + if G.Pkg != nil && !curr.IsFile() { curr = G.Pkg.File(PackagePath(includedFile)) } packagePath := pkg.Rel(curr) Index: pkgsrc/pkgtools/pkglint/files/vartypecheck.go diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.74 pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.75 --- pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.74 Sat Dec 14 18:04:16 2019 +++ pkgsrc/pkgtools/pkglint/files/vartypecheck.go Mon Dec 16 17:06:05 2019 @@ -689,14 +689,7 @@ func (cv *VartypeCheck) IdentifierDirect return } - switch { - case matches(cv.ValueNoVar, `^[+\-.\w]+$`): - // Fine. - - case cv.Value != "" && cv.ValueNoVar == "": - // Don't warn here. - - default: + if !matches(cv.ValueNoVar, `^[+\-.\w]+$`) { cv.Warnf("Invalid identifier %q.", cv.Value) } } Index: pkgsrc/pkgtools/pkglint/files/substcontext_test.go diff -u pkgsrc/pkgtools/pkglint/files/substcontext_test.go:1.33 pkgsrc/pkgtools/pkglint/files/substcontext_test.go:1.34 --- pkgsrc/pkgtools/pkglint/files/substcontext_test.go:1.33 Sat Dec 14 18:04:15 2019 +++ pkgsrc/pkgtools/pkglint/files/substcontext_test.go Mon Dec 16 17:06:05 2019 @@ -410,6 +410,54 @@ func (s *Suite) Test_SubstContext_varass "the SUBST class should be declared using \"SUBST_CLASSES+= libs\".") } +func (s *Suite) Test_SubstContext_varassignDifferentClass__same_paragraph(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= 1", + "SUBST_STAGE.1= pre-configure", + "SUBST_FILES.1= files", + "SUBST_VARS.x= VAR", + "SUBST_VARS.x= VAR") + + // There is a switch of the SUBST class in the middle of the paragraph. + // This is often a typo, therefore pkglint still expects the SUBST class + // 1 to be continued in line 4. + // + // If there were an empty line before line 4, pkglint would have + // interpreted that as an intention to start a new block in the next + // paragraph. + t.CheckOutputLines( + "WARN: filename.mk:4: Variable \"SUBST_VARS.x\" "+ + "does not match SUBST class \"1\".", + "WARN: filename.mk:5: Variable \"SUBST_VARS.x\" "+ + "does not match SUBST class \"1\".", + "WARN: filename.mk:EOF: Incomplete SUBST block: "+ + "SUBST_SED.1, SUBST_VARS.1 or SUBST_FILTER_CMD.1 missing.") +} + +func (s *Suite) Test_SubstContext_varassignDifferentClass__next_paragraph(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= 1", + "SUBST_STAGE.1= pre-configure", + "SUBST_FILES.1= files", + "", + "SUBST_VARS.x= VAR", + "SUBST_VARS.x= VAR") + + // There is a switch of the SUBST class at the end of the paragraph. + // Pkglint sees that as an intention to start a new SUBST block. + t.CheckOutputLines( + "WARN: filename.mk:5: Variable \"SUBST_VARS.x\" "+ + "does not match SUBST class \"1\".", + "WARN: filename.mk:5: Incomplete SUBST block: "+ + "SUBST_SED.1, SUBST_VARS.1 or SUBST_FILTER_CMD.1 missing.", + "WARN: filename.mk:5: Before defining SUBST_VARS.x, "+ + "the SUBST class should be declared using \"SUBST_CLASSES+= x\".") +} + // Unbalanced conditionals must not lead to a panic. func (s *Suite) Test_SubstContext_directive__before_SUBST_CLASSES(c *check.C) { t := s.Init(c) @@ -721,6 +769,56 @@ func (s *Suite) Test_SubstContext_leave_ "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.") } +func (s *Suite) Test_SubstContext_activeId__SUBST_CLASSES_in_separate_paragraph(c *check.C) { + t := s.Init(c) + + ctx := NewSubstContext() + + checkNoActiveId := func() { + t.CheckEquals(ctx.isActive(), false) + } + checkActiveId := func(id string) { + t.CheckEquals(ctx.activeId(), id) + } + lineno := 1 + line := func(text string) { + ctx.Process(t.NewMkLine("filename.mk", lineno, text)) + lineno++ + } + + line("SUBST_CLASSES+= 1 2 3 4") + checkNoActiveId() + + line("") + checkNoActiveId() + + line("SUBST_STAGE.1= post-configure") + checkActiveId("1") + + line("SUBST_FILES.1= files") + line("SUBST_VARS.1= VAR1") + checkActiveId("1") + + line("") + checkActiveId("1") + + line("SUBST_STAGE.2= post-configure") + checkActiveId("2") + + line("SUBST_FILES.2= files") + line("SUBST_VARS.2= VAR1") + line("") + line("SUBST_STAGE.3= post-configure") + line("SUBST_FILES.3= files") + line("SUBST_VARS.3= VAR1") + + ctx.Finish(NewLineEOF("filename.mk")) + + t.CheckOutputLines( + "NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.", + "WARN: filename.mk:EOF: Missing SUBST block for \"4\".") +} + // With every .if directive, a new scope is created, to properly // keep track of the conditional level at which the SUBST classes // are declared. @@ -774,6 +872,85 @@ func (s *Suite) Test_substScope__conditi ctx.Finish(NewLineEOF("filename.mk")) } +func (s *Suite) Test_substScope_define__assertion(c *check.C) { + t := s.Init(c) + + scope := newSubstScope() + scope.define("id") + + t.ExpectAssert( + func() { scope.define("id") }) +} + +// Variables mentioned in SUBST_VARS may appear in the same paragraph, +// or alternatively anywhere else in the file. +func (s *Suite) Test_substScope_finish__foreign_in_next_paragraph(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") + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_substScope_finish__foreign_mixed_separate(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= 1", + "SUBST_STAGE.1= post-configure", + "SUBST_FILES.1= files", + "", + "SUBST_VARS.1= VAR", + "USE_TOOLS+= gmake") + + // The USE_TOOLS is not in the SUBST block anymore since there is + // an empty line between SUBST_CLASSES and SUBST_VARS. + 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_substScope_finish__foreign_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") + + t.CheckOutputLines( + "WARN: filename.mk:6: Foreign variable \"TODAY2\" in SUBST block.") +} + +func (s *Suite) Test_substScope_finish__foreign_two_blocks_one_paragraph(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= 1 2", + "SUBST_STAGE.1= pre-configure", + "VAR2= value2", + "SUBST_FILES.1= files", + "SUBST_VARS.1= VAR1", + "SUBST_STAGE.2= pre-configure", + "SUBST_FILES.2= files", + "VAR1= value1", + "SUBST_VARS.2= VAR2") + + t.CheckOutputLines( + "NOTE: filename.mk:1: " + + "Please add only one class at a time to SUBST_CLASSES.") +} + func (s *Suite) Test_substScope_prepareSubstClasses(c *check.C) { t := s.Init(c) @@ -809,6 +986,113 @@ func (s *Suite) Test_substScope_prepareS "SUBST_SED.1, SUBST_VARS.1 or SUBST_FILTER_CMD.1 missing.") } +func (s *Suite) Test_substBlock__enter_leave_and_finish(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, "") + b := newSubstBlock("id") + + t.CheckEquals(len(b.conds), 1) + + b.enter() + + t.CheckEquals(len(b.conds), 2) + + b.leave() + + t.CheckEquals(len(b.conds), 1) + + b.finish(mkline) + + t.CheckOutputLines( + "WARN: filename.mk:123: Missing SUBST block for \"id\".") +} + +// In a conditional without an else branch, none of the variable +// definitions from the then branch are seen in the outer scope. +func (s *Suite) Test_substBlock__enter_and_leave_without_else(c *check.C) { + t := s.Init(c) + + b := newSubstBlock("id") + + b.enter() // .if + b.addSeen(ssSed) // SUBST_SED + b.leave() // .endif + + t.CheckEquals(b.allSeen(), ssNone) + t.CheckEquals(b.done, false) +} + +func (s *Suite) Test_substBlock__enter_and_leave_with_else(c *check.C) { + t := s.Init(c) + + b := newSubstBlock("id") + + b.enter() // .if + b.addSeen(ssVars) // SUBST_VARS + b.addSeen(ssTransform) // SUBST_VARS + b.nextBranch(true) // .else + b.addSeen(ssSed) // SUBST_SED + b.addSeen(ssTransform) // SUBST_SED + b.leave() // .endif + + t.CheckEquals(b.hasSeen(ssTransform), true) + t.CheckEquals(b.done, false) +} + +func (s *Suite) Test_substBlock__enter_and_leave_with_elif(c *check.C) { + t := s.Init(c) + + b := newSubstBlock("id") + + b.enter() // .if + b.addSeen(ssFiles) // SUBST_FILES + b.addSeen(ssVars) // SUBST_VARS + b.nextBranch(false) // .elif + b.addSeen(ssFiles) // SUBST_FILES + b.addSeen(ssSed) // SUBST_SED + b.nextBranch(true) // .else + b.addSeen(ssFiles) // SUBST_FILES + b.addSeen(ssSed) // SUBST_SED + b.leave() // .endif + + t.CheckEquals(b.hasSeen(ssFiles), true) + t.CheckEquals(b.done, false) +} + +func (s *Suite) Test_newSubstBlock(c *check.C) { + t := s.Init(c) + + b := newSubstBlock("id") + + t.CheckEquals(b.id, "id") + t.CheckEquals(len(b.conds), 1) + t.CheckEquals(b.done, false) + t.CheckEquals(b.allSeen(), ssNone) +} + +func (s *Suite) Test_newSubstBlock__assertion(c *check.C) { + t := s.Init(c) + + t.ExpectAssert( + func() { newSubstBlock("") }) +} + +func (s *Suite) Test_substBlock_varassign__typo_in_subst_variable(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+=\tos", + "SUBST_STAGE.os=\tdo-patch", + "SUBST_FILES.os=\tguess-os.h", + "SUBST_DED.os=\t-e s,@OPSYS@,Darwin,") + + t.CheckOutputLines( + "WARN: filename.mk:EOF: Incomplete SUBST block: "+ + "SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.", + "WARN: filename.mk:4: Foreign variable \"SUBST_DED.os\" in SUBST block.") +} + func (s *Suite) Test_substBlock_varassignStage__do_patch(c *check.C) { t := s.Init(c) @@ -1501,52 +1785,32 @@ func (s *Suite) Test_substBlock_finish__ "SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.") } -// 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) { +func (s *Suite) Test_substBlock_finish__assertion(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") + b := newSubstBlock("id") + b.enter() - t.CheckOutputEmpty() + t.ExpectAssert( + func() { b.finish(nil) }) } -func (s *Suite) Test_substBlock_checkForeignVariables__mixed_separate(c *check.C) { +func (s *Suite) Test_substSeen_set__assertion(c *check.C) { t := s.Init(c) - t.RunSubst( - "SUBST_CLASSES+= 1", - "SUBST_STAGE.1= post-configure", - "SUBST_FILES.1= files", - "", - "SUBST_VARS.1= VAR", - "USE_TOOLS+= gmake") - - // The USE_TOOLS is not in the SUBST block anymore since there is - // an empty line between SUBST_CLASSES and SUBST_VARS. - t.CheckOutputEmpty() + t.ExpectAssert( + func() { + seen := ssAll + seen.set(ssAll) + }) } -// 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) { +func (s *Suite) Test_substSeen_has__assertion(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") - - t.CheckOutputLines( - "WARN: filename.mk:6: Foreign variable \"TODAY2\" in SUBST block.") + t.ExpectAssert( + func() { + seen := ssAll + seen.has(ssAll) + }) } Index: pkgsrc/pkgtools/pkglint/files/util.go diff -u pkgsrc/pkgtools/pkglint/files/util.go:1.66 pkgsrc/pkgtools/pkglint/files/util.go:1.67 --- pkgsrc/pkgtools/pkglint/files/util.go:1.66 Fri Dec 13 01:39:23 2019 +++ pkgsrc/pkgtools/pkglint/files/util.go Mon Dec 16 17:06:05 2019 @@ -574,6 +574,11 @@ func (o *Once) check(key uint64) bool { // // TODO: Merge this code with Var, which defines essentially the // same features. +// +// See also substScope, which already analyzes the possible variable values +// based on the conditional code paths. +// +// See also RedundantScope. type Scope struct { firstDef map[string]*MkLine // TODO: Can this be removed? lastDef map[string]*MkLine @@ -1381,3 +1386,23 @@ func (b *LazyStringBuilder) String() str } return b.expected[:b.len] } + +type interval struct { + min int + max int +} + +func newInterval() *interval { + return &interval{int(^uint(0) >> 1), ^int(^uint(0) >> 1)} +} + +func (i *interval) add(x int) { + if x < i.min { + i.min = x + } + if x > i.max { + i.max = x + } +} + +func (i *interval) isEmpty() bool { return i.min > i.max } Index: pkgsrc/pkgtools/pkglint/files/util_test.go diff -u pkgsrc/pkgtools/pkglint/files/util_test.go:1.43 pkgsrc/pkgtools/pkglint/files/util_test.go:1.44 --- pkgsrc/pkgtools/pkglint/files/util_test.go:1.43 Fri Dec 13 01:39:23 2019 +++ pkgsrc/pkgtools/pkglint/files/util_test.go Mon Dec 16 17:06:05 2019 @@ -1150,3 +1150,26 @@ func keys(m interface{}) []string { sort.Strings(keys) return keys } + +func (s *Suite) Test_interval(c *check.C) { + t := s.Init(c) + + i := newInterval() + + t.CheckEquals(i.min > i.max, true) + + i.add(3) + + t.CheckEquals(i.min, 3) + t.CheckEquals(i.max, 3) + + i.add(7) + + t.CheckEquals(i.min, 3) + t.CheckEquals(i.max, 7) + + i.add(-5) + + t.CheckEquals(i.min, -5) + t.CheckEquals(i.max, 7) +} Index: pkgsrc/pkgtools/pkglint/files/varalignblock.go diff -u pkgsrc/pkgtools/pkglint/files/varalignblock.go:1.12 pkgsrc/pkgtools/pkglint/files/varalignblock.go:1.13 --- pkgsrc/pkgtools/pkglint/files/varalignblock.go:1.12 Fri Dec 13 01:39:23 2019 +++ pkgsrc/pkgtools/pkglint/files/varalignblock.go Mon Dec 16 17:06:05 2019 @@ -183,28 +183,18 @@ func (va *VaralignBlock) processVarassig return } - follow := false var infos []*varalignLine for i, raw := range mkline.raw { parts := NewVaralignSplitter().split(strings.TrimSuffix(raw.textnl, "\n"), i == 0) - info := varalignLine{mkline, i, follow, false, parts} - - if i == 0 && info.isEmptyContinuation() { - follow = true - info.multiEmpty = true - } - + info := varalignLine{mkline, i, false, false, parts} infos = append(infos, &info) } va.mkinfos = append(va.mkinfos, &varalignMkLine{infos}) } func (va *VaralignBlock) Finish() { - mkinfos := va.mkinfos - skip := va.skip - *va = VaralignBlock{} // overwrites infos and skip - - if len(mkinfos) == 0 || skip { + if len(va.mkinfos) == 0 || va.skip { + *va = VaralignBlock{} return } @@ -212,32 +202,39 @@ func (va *VaralignBlock) Finish() { defer trace.Call()() } - newWidth := va.optimalWidth(mkinfos) - va.adjustLong(newWidth, mkinfos) + newWidth := va.optimalWidth() + va.adjustLong(newWidth) + + for _, mkinfo := range va.mkinfos { + mkinfo.realign(newWidth) + } - for _, mkinfo := range mkinfos { + *va = VaralignBlock{} +} - // When the indentation of the initial line of a multiline is - // changed, all its follow-up lines are shifted by the same - // amount and in the same direction. Typical examples are - // SUBST_SED, shell programs and AWK programs like in - // GENERATE_PLIST. - indentDiffSet := false - - // The amount by which the follow-up lines are shifted. - // Positive values mean shifting to the right, negative values - // mean shifting to the left. - indentDiff := 0 +func (l *varalignMkLine) realign(newWidth int) { + // When the indentation of the initial line of a multiline is + // changed, all its follow-up lines are shifted by the same + // amount and in the same direction. Typical examples are + // SUBST_SED, shell programs and AWK programs like in + // GENERATE_PLIST. + indentDiffSet := false - _, rightMargin := mkinfo.rightMargin() - for _, info := range mkinfo.infos { + // The amount by which the follow-up lines are shifted. + // Positive values mean shifting to the right, negative values + // mean shifting to the left. + indentDiff := 0 - // TODO: move below va.realign - info.alignContinuation(newWidth, rightMargin) + _, rightMargin := l.rightMargin() + isMultiEmpty := l.isMultiEmpty() + for _, info := range l.infos { - if newWidth > 0 || info.rawIndex > 0 { - va.realign(info, newWidth, &indentDiffSet, &indentDiff) - } + if newWidth > 0 || info.rawIndex > 0 { + info.realignDetails(newWidth, &indentDiffSet, &indentDiff, isMultiEmpty) + } + + if !info.fixedSpaceBeforeContinuation { + info.alignContinuation(newWidth, rightMargin) } } } @@ -248,23 +245,41 @@ func (va *VaralignBlock) Finish() { // There may be a single line sticking out from the others (called outlier). // This is to prevent a single SITES.* variable from forcing the rest of the // paragraph to be indented too far to the right. -func (*VaralignBlock) optimalWidth(mkinfos []*varalignMkLine) int { +func (va *VaralignBlock) optimalWidth() int { + + minVarnameOpWidth, outlier := va.varnameOpWidths() + minTotalWidth, maxTotalWidth := va.spaceWidths(outlier) + va.traceWidths(minTotalWidth, maxTotalWidth, minVarnameOpWidth, outlier) + + if minTotalWidth > minVarnameOpWidth && minTotalWidth == maxTotalWidth && minTotalWidth%8 == 0 { + // The whole paragraph is already indented to the same width. + return minTotalWidth + } + + if minVarnameOpWidth == 0 { + // Only continuation lines in this paragraph. + return 0 + } + + return minVarnameOpWidth&-8 + 8 +} +func (va *VaralignBlock) varnameOpWidths() (int, int) { var widths bag - for _, mkinfo := range mkinfos { - for _, info := range mkinfo.infos { - if !info.multiEmpty && info.rawIndex == 0 { - widths.Add(info.fixer, info.varnameOpWidth()) - } + for _, mkinfo := range va.mkinfos { + if !mkinfo.isMultiEmpty() { + info := mkinfo.infos[0] + widths.add(info.fixer, info.spaceBeforeValueColumn()) } } + if widths.len() == 0 { + return 0, 0 + } + widths.sortDesc() longest := widths.opt(0) - var longestLine *MkLine - if len(widths.slice) > 0 { - longestLine = widths.key(0).(*MkLine) - } + longestLine := widths.key(0).(*MkLine) secondLongest := widths.opt(1) haveOutlier := secondLongest != 0 && @@ -274,22 +289,24 @@ func (*VaralignBlock) optimalWidth(mkinf // Minimum required width of varnameOp, without the trailing whitespace. minVarnameOpWidth := condInt(haveOutlier, secondLongest, longest) outlier := condInt(haveOutlier, longest, 0) + return minVarnameOpWidth, outlier +} +func (va *VaralignBlock) spaceWidths(outlier int) (min, max int) { // Widths of the current indentation (including whitespace) - var spaceWidths bag - for _, mkinfo := range mkinfos { - for _, info := range mkinfo.infos { - if info.multiEmpty || info.rawIndex > 0 || outlier > 0 && info.varnameOpWidth() == outlier { - continue - } - spaceWidths.Add(nil, info.varnameOpSpaceWidth()) + spaceWidths := newInterval() + for _, mkinfo := range va.mkinfos { + info := mkinfo.infos[0] + if mkinfo.isMultiEmpty() || outlier > 0 && info.spaceBeforeValueColumn() == outlier { + continue } + spaceWidths.add(info.valueColumn()) } - spaceWidths.sortDesc() - minTotalWidth := spaceWidths.min() - maxTotalWidth := spaceWidths.max() + return spaceWidths.min, spaceWidths.max +} +func (va *VaralignBlock) traceWidths(minTotalWidth int, maxTotalWidth int, minVarnameOpWidth int, outlier int) { if trace.Tracing { trace.Stepf("Indentation including whitespace is between %d and %d.", minTotalWidth, maxTotalWidth) @@ -298,27 +315,16 @@ func (*VaralignBlock) optimalWidth(mkinf trace.Stepf("The outlier is at indentation %d.", outlier) } } - - if minTotalWidth > minVarnameOpWidth && minTotalWidth == maxTotalWidth && minTotalWidth%8 == 0 { - // The whole paragraph is already indented to the same width. - return minTotalWidth - } - - if minVarnameOpWidth == 0 { - // Only continuation lines in this paragraph. - return 0 - } - - return minVarnameOpWidth&-8 + 8 } // adjustLong allows any follow-up line to start either in column 8 or at // least in column newWidth. But only if there is at least one continuation // line that starts in column 8 and needs the full width up to column 72. -func (va *VaralignBlock) adjustLong(newWidth int, mkinfos []*varalignMkLine) { +func (va *VaralignBlock) adjustLong(newWidth int) { anyLong := false - for _, mkinfo := range mkinfos { + for _, mkinfo := range va.mkinfos { infos := mkinfo.infos + isMultiEmpty := mkinfo.isMultiEmpty() for i, info := range infos { if info.rawIndex == 0 { anyLong = false @@ -326,27 +332,27 @@ func (va *VaralignBlock) adjustLong(newW if follow.rawIndex == 0 { break } - if !follow.multiEmpty && follow.spaceBeforeValue == "\t" && follow.varnameOpSpaceWidth() < newWidth && follow.widthAlignedAt(newWidth) > 72 { + if !isMultiEmpty && follow.spaceBeforeValue == "\t" && follow.valueColumn() < newWidth && follow.widthAlignedAt(newWidth) > 72 { anyLong = true break } } } - info.long = anyLong && info.varnameOpSpaceWidth() == 8 + info.long = anyLong && info.valueColumn() == 8 } } } -func (va *VaralignBlock) realign(info *varalignLine, newWidth int, indentDiffSet *bool, indentDiff *int) { - if info.multiEmpty { +func (info *varalignLine) realignDetails(newWidth int, indentDiffSet *bool, indentDiff *int, isMultiEmpty bool) { + if isMultiEmpty { if info.rawIndex == 0 { info.alignValueMultiEmptyInitial(newWidth) } else { - va.realignMultiEmptyFollow(info, newWidth, indentDiffSet, indentDiff) + info.realignMultiEmptyFollow(newWidth, indentDiffSet, indentDiff) } } else if info.rawIndex == 0 && info.isContinuation() { - va.realignMultiInitial(info, newWidth, indentDiffSet, indentDiff) + info.realignMultiInitial(newWidth, indentDiffSet, indentDiff) } else if info.rawIndex > 0 { assert(*indentDiffSet) info.alignValueMultiFollow(newWidth, *indentDiff) @@ -355,7 +361,7 @@ func (va *VaralignBlock) realign(info *v } } -func (va *VaralignBlock) realignMultiEmptyFollow(info *varalignLine, newWidth int, indentDiffSet *bool, indentDiff *int) { +func (info *varalignLine) realignMultiEmptyFollow(newWidth int, indentDiffSet *bool, indentDiff *int) { oldSpace := info.spaceBeforeValue oldWidth := tabWidth(oldSpace) @@ -370,9 +376,9 @@ func (va *VaralignBlock) realignMultiEmp info.alignValueMultiEmptyFollow(imax(oldWidth+*indentDiff, 8)) } -func (va *VaralignBlock) realignMultiInitial(info *varalignLine, newWidth int, indentDiffSet *bool, indentDiff *int) { +func (info *varalignLine) realignMultiInitial(newWidth int, indentDiffSet *bool, indentDiff *int) { *indentDiffSet = true - *indentDiff = newWidth - info.varnameOpSpaceWidth() + *indentDiff = newWidth - info.valueColumn() info.alignValueMultiInitial(newWidth) } @@ -476,6 +482,20 @@ type varalignMkLine struct { infos []*varalignLine } +// Is true for multilines that don't have a value in their first +// physical line. +// +// The follow-up lines of these lines may be indented with as few +// as a single tab. Example: +// VAR= \ +// value1 \ +// value2 +// In all other lines, the indentation must be at least the indentation +// of the first value found. +func (l *varalignMkLine) isMultiEmpty() bool { + return l.infos[0].isEmptyContinuation() +} + // rightMargin calculates the column in which the continuation backslashes // should be placed. // In addition it returns whether the right margin is already in its @@ -524,21 +544,11 @@ type varalignLine struct { fixer Autofixer rawIndex int - // Is true for multilines that don't have a value in their first - // physical line. - // - // The follow-up lines of these lines may be indented with as few - // as a single tab. Example: - // VAR= \ - // value1 \ - // value2 - // In all other lines, the indentation must be at least the indentation - // of the first value found. - multiEmpty bool - // Whether the line is so long that it may use a single tab as indentation. long bool + fixedSpaceBeforeContinuation bool + varalignParts } @@ -590,7 +600,7 @@ func (info *varalignLine) alignValueMult // Indent the outlier and any other lines that stick out // with a space instead of a tab to keep the line short. newSpace := " " - if info.varnameOpSpaceWidth() <= newWidth { + if info.valueColumn() <= newWidth { newSpace = alignmentAfter(leadingComment+varnameOp, newWidth) } @@ -603,7 +613,7 @@ func (info *varalignLine) alignValueMult } hasSpace := strings.IndexByte(oldSpace, ' ') != -1 - oldColumn := info.varnameOpSpaceWidth() + oldColumn := info.valueColumn() column := tabWidthSlice(leadingComment, varnameOp, newSpace) fix := info.fixer.Autofix() @@ -616,6 +626,7 @@ func (info *varalignLine) alignValueMult } fix.ReplaceAt(info.rawIndex, info.spaceBeforeValueIndex(), oldSpace, newSpace) fix.Apply() + info.spaceBeforeValue = newSpace } func (info *varalignLine) alignValueMultiInitial(column int) { @@ -628,7 +639,7 @@ func (info *varalignLine) alignValueMult return } - oldWidth := info.varnameOpSpaceWidth() + oldWidth := info.valueColumn() width := tabWidthSlice(leadingComment, varnameOp, newSpace) fix := info.fixer.Autofix() @@ -641,6 +652,7 @@ func (info *varalignLine) alignValueMult } fix.ReplaceAt(info.rawIndex, info.spaceBeforeValueIndex(), oldSpace, newSpace) fix.Apply() + info.spaceBeforeValue = newSpace } func (info *varalignLine) alignValueMultiEmptyFollow(column int) { @@ -650,44 +662,37 @@ func (info *varalignLine) alignValueMult return } - // Below a continuation marker, there may be a completely empty line. - // This is confusing to the human readers, but technically allowed. - if info.varalignParts.isEmpty() { - return - } - - fix := info.fixer.Autofix() - fix.Notef("This continuation line should be indented with %q.", newSpace) - fix.ReplaceAt(info.rawIndex, info.spaceBeforeValueIndex(), oldSpace, newSpace) - fix.Apply() - - // TODO: Merge with alignValueMultiFollow + info.alignFollow(newSpace) } func (info *varalignLine) alignValueMultiFollow(column, indentDiff int) { oldSpace := info.spaceBeforeValue newWidth := imax(column, tabWidth(oldSpace)+indentDiff) newSpace := indent(newWidth) - if newSpace == oldSpace || info.long || info.isTooLongFor(newWidth) { + if newSpace == oldSpace { + return + } + + info.alignFollow(newSpace) +} + +func (info *varalignLine) alignFollow(newSpace string) { + // Below a continuation marker, there may be a completely empty line. + // This is confusing to the human readers, but technically allowed. + if info.varalignParts.isEmpty() { return } + continuationColumn := 0 + if info.spaceBeforeContinuation() != " " { + continuationColumn = imin(72, info.continuationColumn()) + } + fix := info.fixer.Autofix() fix.Notef("This continuation line should be indented with %q.", newSpace) - modified, replaced := fix.ReplaceAt(info.rawIndex, info.spaceBeforeValueIndex(), oldSpace, newSpace) - assert(modified) - if info.continuation != "" && info.continuationColumn() == 72 { - orig := strings.TrimSuffix(replaced, "\n") - base := rtrimHspace(strings.TrimSuffix(orig, "\\")) - spaceIndex := len(base) - oldSuffix := orig[spaceIndex:] - newSuffix := " \\" - if tabWidth(base) < 72 { - newSuffix = alignmentAfter(base, 72) + "\\" - } - if newSuffix != oldSuffix { - fix.ReplaceAt(info.rawIndex, spaceIndex, oldSuffix, newSuffix) - } + info.replaceSpaceBeforeValue(fix, newSpace) + if info.isContinuation() { + info.replaceSpaceBeforeContinuationSilently(fix, continuationColumn) } fix.Apply() } @@ -698,11 +703,14 @@ func (info *varalignLine) alignContinuat } oldSpace := info.spaceBeforeContinuation() - if oldSpace == " " || oldSpace == "\t" { + if oldSpace == " " { return } column := info.continuationColumn() + if column <= 72 && oldSpace == "\t" { + return + } if column == 72 || column == rightMarginColumn || column <= valueColumn { return } @@ -716,16 +724,47 @@ func (info *varalignLine) alignContinuat } else if info.uptoValueWidth() >= rightMarginColumn { fix.Notef("The continuation backslash should be preceded by a single space or tab.") } else { - newSpace = alignmentAfter(info.uptoValue(), rightMarginColumn) + newSpace = alignmentToWidths(info.uptoValueWidth(), rightMarginColumn) fix.Notef( "The continuation backslash should be in column %d, not %d.", rightMarginColumn+1, column+1) } index := info.continuationIndex() - len(oldSpace) fix.ReplaceAt(info.rawIndex, index, oldSpace, newSpace) + info.setSpaceBeforeContinuation(newSpace) fix.Apply() } +func (info *varalignLine) replaceSpaceBeforeValue(fix *Autofix, newSpace string) { + index := info.spaceBeforeValueIndex() + fix.ReplaceAt(info.rawIndex, index, info.spaceBeforeValue, newSpace) + info.spaceBeforeValue = newSpace +} + +func (info *varalignLine) replaceSpaceBeforeContinuationSilently(fix *Autofix, column int) { + if info.value == "" { + return + } + + oldSpace := info.spaceBeforeContinuation() + if oldSpace == " " { + return + } + newSpaceColumn := info.uptoValueWidth() + newSpace := alignmentToWidths(newSpaceColumn, column) + if newSpace == "" { + newSpace = " " + } + if oldSpace == newSpace { + return + } + + index := info.spaceBeforeContinuationIndex() + fix.ReplaceAt(info.rawIndex, index, oldSpace+"\\", newSpace+"\\") + info.varalignParts.setSpaceBeforeContinuation(newSpace) + info.fixedSpaceBeforeContinuation = true +} + func (*varalignLine) explainWrongColumn(fix *Autofix) { fix.Explain( "Normally, all variable values in a block should start at the same column.", @@ -760,6 +799,58 @@ func (p *varalignParts) String() string p.continuation } +func (p *varalignParts) leadingCommentIndex() int { + return 0 +} + +func (p *varalignParts) varnameOpIndex() int { + return p.leadingCommentIndex() + len(p.leadingComment) +} + +// spaceBeforeValueIndex returns the string index at which the space before the value starts. +// It's the same as the end of the assignment operator. Example: +// #VAR= value +// The index is 5. +func (p *varalignParts) spaceBeforeValueIndex() int { + return p.varnameOpIndex() + len(p.varnameOp) +} + +func (p *varalignParts) valueIndex() int { + return p.spaceBeforeValueIndex() + len(p.spaceBeforeValue) +} + +func (p *varalignParts) spaceAfterValueIndex() int { + return p.valueIndex() + len(p.value) +} + +func (p *varalignParts) continuationIndex() int { + return p.spaceAfterValueIndex() + len(p.spaceAfterValue) +} + +func (p *varalignParts) leadingCommentColumn() int { + return 0 +} + +func (p *varalignParts) varnameOpColumn() int { + return tabWidthAppend(p.leadingCommentColumn(), p.leadingComment) +} + +func (p *varalignParts) spaceBeforeValueColumn() int { + return tabWidthAppend(p.varnameOpColumn(), p.varnameOp) +} + +func (p *varalignParts) valueColumn() int { + return tabWidthAppend(p.spaceBeforeValueColumn(), p.spaceBeforeValue) +} + +func (p *varalignParts) spaceAfterValueColumn() int { + return tabWidthAppend(p.valueColumn(), p.value) +} + +func (p *varalignParts) continuationColumn() int { + return tabWidthAppend(p.spaceAfterValueColumn(), p.spaceAfterValue) +} + // continuation returns whether this line ends with a backslash. func (p *varalignParts) isContinuation() bool { return p.continuation != "" @@ -773,22 +864,6 @@ func (p *varalignParts) isEmpty() bool { return p.value == "" && !p.isContinuation() } -func (p *varalignParts) varnameOpWidth() int { - return tabWidthSlice(p.leadingComment, p.varnameOp) -} - -func (p *varalignParts) varnameOpSpaceWidth() int { - return tabWidthSlice(p.leadingComment, p.varnameOp, p.spaceBeforeValue) -} - -// spaceBeforeValueIndex returns the string index at which the space before the value starts. -// It's the same as the end of the assignment operator. Example: -// #VAR= value -// The index is 5. -func (p *varalignParts) spaceBeforeValueIndex() int { - return len(p.leadingComment) + len(p.varnameOp) -} - func (p *varalignParts) spaceBeforeContinuation() string { if p.value == "" { return p.spaceBeforeValue @@ -796,28 +871,27 @@ func (p *varalignParts) spaceBeforeConti return p.spaceAfterValue } -func (p *varalignParts) uptoValueWidth() int { - return tabWidth(rtrimHspace(p.leadingComment + - p.varnameOp + p.spaceBeforeValue + - p.value)) +func (p *varalignParts) setSpaceBeforeContinuation(space string) { + if p.value == "" { + p.spaceBeforeValue = space + } else { + p.spaceAfterValue = space + } } -func (p *varalignParts) uptoValue() string { - return rtrimHspace(p.leadingComment + - p.varnameOp + p.spaceBeforeValue + - p.value) -} +func (p *varalignParts) spaceBeforeContinuationIndex() int { + assert(p.isContinuation()) + assert(p.value != "") -func (p *varalignParts) continuationColumn() int { - return tabWidthSlice(p.leadingComment, - p.varnameOp, p.spaceBeforeValue, - p.value, p.spaceAfterValue) + return p.spaceAfterValueIndex() } -func (p *varalignParts) continuationIndex() int { - return len(p.leadingComment) + - len(p.varnameOp) + len(p.spaceBeforeValue) + - len(p.value) + len(p.spaceAfterValue) +func (p *varalignParts) uptoValueWidth() int { + if p.value != "" { + return p.spaceAfterValueColumn() + } else { + return p.varnameOpColumn() + } } func (p *varalignParts) isCommentedOut() bool { @@ -827,13 +901,13 @@ func (p *varalignParts) isCommentedOut() // isCanonicalInitial returns whether the space between the assignment // operator and the value has its canonical form, which is either // at least one tab, or a single space, but only for lines that stick out. -func (p *varalignParts) isCanonicalInitial(width int) bool { +func (p *varalignParts) isCanonicalInitial(column int) bool { space := p.spaceBeforeValue if space == "" { return false } - if space == " " && p.varnameOpSpaceWidth() > width { + if space == " " && p.valueColumn() > column { return true } @@ -895,13 +969,15 @@ func (mi *bag) key(index int) interface{ return mi.slice[index].key } -func (mi *bag) Add(key interface{}, value int) { +func (mi *bag) add(key interface{}, value int) { mi.slice = append(mi.slice, struct { key interface{} value int }{key, value}) } -func (mi *bag) min() int { return mi.opt(0) } +func (mi *bag) first() int { return mi.opt(0) } + +func (mi *bag) last() int { return mi.opt(len(mi.slice) - 1) } -func (mi *bag) max() int { return mi.opt(len(mi.slice) - 1) } +func (mi *bag) len() int { return len(mi.slice) } Index: pkgsrc/pkgtools/pkglint/files/varalignblock_test.go diff -u pkgsrc/pkgtools/pkglint/files/varalignblock_test.go:1.8 pkgsrc/pkgtools/pkglint/files/varalignblock_test.go:1.9 --- pkgsrc/pkgtools/pkglint/files/varalignblock_test.go:1.8 Fri Dec 13 01:39:23 2019 +++ pkgsrc/pkgtools/pkglint/files/varalignblock_test.go Mon Dec 16 17:06:05 2019 @@ -95,23 +95,28 @@ func (vt *VaralignTester) run(autofix bo varalign.Process(mkline) } - mkinfos := varalign.mkinfos // since they are overwritten by Finish - varalign.Finish() + var actualInternals []string if len(vt.internals) > 0 { var actual []string - for _, mkinfo := range mkinfos { + for _, mkinfo := range varalign.mkinfos { for _, info := range mkinfo.infos { var minWidth, curWidth, continuation string - minWidth = condStr(info.rawIndex == 0, sprintf("%02d", info.varnameOpWidth()), " ") - curWidth = sprintf(" %02d", info.varnameOpSpaceWidth()) + minWidth = condStr(info.rawIndex == 0, sprintf("%02d", info.spaceBeforeValueColumn()), " ") + curWidth = sprintf(" %02d", info.valueColumn()) if info.isContinuation() { continuation = sprintf(" %02d", info.continuationColumn()) } actual = append(actual, minWidth+curWidth+continuation) } } - t.CheckDeepEquals(actual, vt.internals) + actualInternals = actual + } + + varalign.Finish() + + if len(vt.internals) > 0 { + t.CheckDeepEquals(actualInternals, vt.internals) } if autofix { @@ -2024,13 +2029,10 @@ func (s *Suite) Test_VaralignBlock__lead // // The value in the first line starts in column 16, which means that all // follow-up lines should also start in column 16 or further to the right. -// Line 2 though is already quite long, and since its right margin is in -// column 72, it may keep its lower-than-usual indentation of 8. -// Line 3 is not that long, therefore the rule from line 2 doesn't apply -// here, and it needs to be indented to column 16. // -// Since the above result would look inconsistent, all follow-up lines -// after a long line may be indented in column 8 as well. +// Line 2 though is already quite long, and even though its right margin +// is in column 72, it needs to be indented at least as much as the value +// of the first line. func (s *Suite) Test_VaralignBlock__var_tab_value63_space_cont_tab8_value71_space_cont_tab8_value(c *check.C) { vt := NewVaralignTester(s, c) vt.Input( @@ -2042,13 +2044,17 @@ func (s *Suite) Test_VaralignBlock__var_ " 08 72", " 08") vt.Diagnostics( - nil...) + "NOTE: Makefile:2: This continuation line should be "+ + "indented with \"\\t\\t\".", + "NOTE: Makefile:3: This continuation line should be "+ + "indented with \"\\t\\t\".") vt.Autofixes( - nil...) + "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\".", + "AUTOFIX: Makefile:3: Replacing \"\\t\" with \"\\t\\t\".") vt.Fixed( "PROGFILES= 67890 234567890 234567890 234567890 234567890 2 \\", - " 890 234567890 234567890 234567890 234567890 234567890 234567890 \\", - " value") + " 890 234567890 234567890 234567890 234567890 234567890 234567890 \\", + " value") vt.Run() } @@ -2271,8 +2277,8 @@ func (s *Suite) Test_VaralignBlock__mixe func (s *Suite) Test_VaralignBlock__long_line_followed_by_short_line_with_small_indentation(c *check.C) { vt := NewVaralignTester(s, c) vt.Input( - "VAR.567890123456+=\t----30 -------40 -------50 -------60 -------70 234567 \\", - "\t\t--20 -------30") + "VAR...........16+=\t....30........40........50........60........70 234567 \\", + "\t\t..20........30") vt.Internals( "18 24 78", " 16") @@ -2281,8 +2287,8 @@ func (s *Suite) Test_VaralignBlock__long vt.Autofixes( "AUTOFIX: Makefile:2: Replacing \"\\t\\t\" with \"\\t\\t\\t\".") vt.Fixed( - "VAR.567890123456+= ----30 -------40 -------50 -------60 -------70 234567 \\", - " --20 -------30") + "VAR...........16+= ....30........40........50........60........70 234567 \\", + " ..20........30") vt.Run() } @@ -2314,14 +2320,13 @@ func (s *Suite) Test_VaralignBlock__shif vt := NewVaralignTester(s, c) vt.Input( "INSTALLATION_DIRS+=\tvalue", - "CONF_FILES=\t--20 -------30 -------40 -------50 -------60 -------70 \\", - "\t\t--20") + "CONF_FILES=\t..20........30........40........50........60........70 \\", + "\t\t..20") vt.Internals( "19 24", "11 16 71", " 16") vt.Diagnostics( - // FIXME: No, it shouldn't, as that would make the continuation marker invisible on 80x25. "NOTE: Makefile:2: This variable value should be aligned to column 25.", "NOTE: Makefile:3: This continuation line should be indented with \"\\t\\t\\t\".") vt.Autofixes( @@ -2329,8 +2334,8 @@ func (s *Suite) Test_VaralignBlock__shif "AUTOFIX: Makefile:3: Replacing \"\\t\\t\" with \"\\t\\t\\t\".") vt.Fixed( "INSTALLATION_DIRS+= value", - "CONF_FILES= --20 -------30 -------40 -------50 -------60 -------70 \\", - " --20") + "CONF_FILES= ..20........30........40........50........60........70 \\", + " ..20") vt.Run() } @@ -2540,11 +2545,20 @@ func (s *Suite) Test_VaralignBlock__alig false) // The second line is indented with a single tab because otherwise - // it would be longer than 72 characters. In this case it is ok to - // use the smaller indentation. + // it would be longer than 72 characters. It could be argued that + // in this case it is ok to use the smaller indentation. That would + // make the indentation rules more complicated than necessary though. + // If absolutely necessary, it is possible to use a continued line + // with only the backslash in the first line. These may be indented + // with a single tab. test( "VAR.param=\tvalue \\", "\t10........20........30........40........50........60...65", + false) + + test( + "VAR.param=\tvalue \\", + "\t\t\t......32\t\t\t\t\t......80.", true) // Having the continuation line in column 0 looks even more confusing. @@ -2722,13 +2736,13 @@ func (s *Suite) Test_VaralignBlock__cont func (s *Suite) Test_VaralignBlock__realign_continuation_backslashes(c *check.C) { vt := NewVaralignTester(s, c) vt.Input( - "VAR4567890.234567890=\t----30--------40--------50\t\t\t\\", - "\t\t--20--------30--------40--------50\t\t\t\\", - "\t\t--20--------30--------40--------50") + "VAR4567890.234567890=\t....30........40........50\t\t\t\\", + "\t\t..20........30........40........50\t\t\t\\", + "\t\t..20........30........40........50") vt.InputDetab( - "VAR4567890.234567890= ----30--------40--------50 \\", - " --20--------30--------40--------50 \\", - " --20--------30--------40--------50") + "VAR4567890.234567890= ....30........40........50 \\", + " ..20........30........40........50 \\", + " ..20........30........40........50") vt.Internals( "21 24 72", " 16 72", @@ -2741,9 +2755,9 @@ func (s *Suite) Test_VaralignBlock__real "AUTOFIX: Makefile:2: Replacing \"\\t\\t\\t\\\\\" with \"\\t\\t\\\\\".", "AUTOFIX: Makefile:3: Replacing \"\\t\\t\" with \"\\t\\t\\t\".") vt.Fixed( - "VAR4567890.234567890= ----30--------40--------50 \\", - " --20--------30--------40--------50 \\", - " --20--------30--------40--------50") + "VAR4567890.234567890= ....30........40........50 \\", + " ..20........30........40........50 \\", + " ..20........30........40........50") vt.Run() } @@ -2788,13 +2802,11 @@ func (s *Suite) Test_VaralignBlock__long vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\\t \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\\t\\t\\t\\t\".", + "AUTOFIX: Makefile:2: Replacing \" \\t \\\\\" with \" \\\\\".", "AUTOFIX: Makefile:3: Replacing \"\\t\" with \"\\t\\t\\t\\t\\t\\t\".") vt.Fixed( "VAR= value \\", - // FIXME: The backslash should be aligned properly. - // It is not replaced because alignContinuation is called before fixAlign, - // which is the wrong order. - " value \\", + " value \\", " value") vt.Run() } @@ -3073,26 +3085,36 @@ func (s *Suite) Test_VaralignBlock_Finis // of the variable value. "NOTE: Makefile:1: The continuation backslash should be "+ "in column 73, not 81.", - // Line 2 is not indented to column 32 - // since that would make the line longer than 72 columns. + "NOTE: Makefile:2: This continuation line should be "+ + "indented with \"\\t\\t\\t\\t\".", "NOTE: Makefile:3: This continuation line should be "+ "indented with \"\\t\\t\\t\\t\".") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\\t\\t\\t\" with \"\\t\\t\\t\".", + "AUTOFIX: Makefile:2: Replacing \"\\t\\t\\t\" with \"\\t\\t\\t\\t\".", + "AUTOFIX: Makefile:2: Replacing \"\\t\\\\\" with \" \\\\\".", "AUTOFIX: Makefile:3: Replacing \"\\t\\t\\t\" with \"\\t\\t\\t\\t\".") vt.Fixed( "VAR....8......16..= ......40......48. \\", - " ......32......40......48......56......64.. \\", + " ......32......40......48......56......64.. \\", " ...29") vt.Run() } +func (s *Suite) Test_varalignLine_realignDetails(c *check.C) { + t := s.Init(c) + + // FIXME + + t.CheckOutputEmpty() +} + // This example is quite unrealistic since typically the first line is // the least indented. // // All follow-up lines are indented with at least one tab, to make clear // they are continuation lines. -func (s *Suite) Test_VaralignBlock_realignMultiEmptyFollow(c *check.C) { +func (s *Suite) Test_varalignLine_realignMultiEmptyFollow(c *check.C) { vt := NewVaralignTester(s, c) vt.Input( "VAR= \\", @@ -3135,7 +3157,7 @@ func (s *Suite) Test_VaralignBlock_reali vt.Run() } -func (s *Suite) Test_VaralignBlock_realignMultiInitial__spaces(c *check.C) { +func (s *Suite) Test_varalignLine_realignMultiInitial__spaces(c *check.C) { vt := NewVaralignTester(s, c) vt.Input( "VAR= value1 \\", @@ -3568,8 +3590,7 @@ func (s *Suite) Test_varalignLine_alignV info.alignValueSingle(column) t.CheckEqualsf( - mkline.raw[0].text(), - condStr(autofix, after, before), + mkline.raw[0].text(), after, "Line.raw.text, autofix=%v", autofix) // As of 2019-12-11, the info fields are not updated @@ -3714,14 +3735,10 @@ func (s *Suite) Test_varalignLine_alignV info.alignValueMultiInitial(column) t.CheckEqualsf( - mkline.raw[0].text(), - condStr(autofix, after, before), + mkline.raw[0].text(), after, "Line.raw.text, autofix=%v", autofix) - // As of 2019-12-11, the info fields are not updated - // accordingly, but they should. - // TODO: update info accordingly - t.CheckEqualsf(info.String(), before, + t.CheckEqualsf(info.String(), after, "info.String, autofix=%v", autofix) } @@ -3776,24 +3793,112 @@ func (s *Suite) Test_varalignLine_alignV test() } +func (s *Suite) Test_varalignLine_alignValueMultiFollow(c *check.C) { + t := s.Init(c) + + // newLine creates a line consisting of either 2 or 3 physical lines. + // The text ends up in the raw line with index 1. + newLine := func(text string, column, indentDiff int) (*varalignLine, *RawLine) { + t.CheckDotColumns("", text) + + leading := alignWith("VAR=", indent(column)) + "value \\" + trailing := indent(column) + "trailing" + n := condInt(hasSuffix(text, "\\"), 3, 2) + lines := []string{leading, text, trailing}[:n] + + mklines := t.NewMkLines("filename.mk", + lines...) + assert(len(mklines.mklines) == 1) + mkline := mklines.mklines[0] + + parts := NewVaralignSplitter().split(text, false) + isLong := parts.isTooLongFor(column + indentDiff) + return &varalignLine{mkline, 1, isLong, false, parts}, mkline.raw[1] + } + + test := func(before string, column, indentDiff int, after string, diagnostics ...string) { + + doTest := func(autofix bool) { + info, raw := newLine(before, column, indentDiff) + + info.alignValueMultiFollow(column, indentDiff) + + t.CheckEquals(raw.text(), after) + } + + t.ExpectDiagnosticsAutofix( + doTest, + diagnostics...) + } + + test( + "value", 24, 0, + "\t\t\tvalue", + + "NOTE: filename.mk:2: This continuation line should be "+ + "indented with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: Replacing \"\" with \"\\t\\t\\t\".") + + test( + "value \\", 24, 0, + "\t\t\tvalue \\", + + "NOTE: filename.mk:2: This continuation line should be "+ + "indented with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: Replacing \"\" with \"\\t\\t\\t\".") + + test( + "value \\", 24, 0, + "\t\t\tvalue \\", + + "NOTE: filename.mk:2: This continuation line should be "+ + "indented with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: Replacing \"\" with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: Replacing \" \\\\\" with \" \\\\\".") + + // As a special case, a continuation backslash in column 72 is preserved. + // TODO: Make this more general. + test( + "value\t\t\t\t\t\t\t\t\t\\", 24, 0, + "\t\t\tvalue\t\t\t\t\t\t\\", + + "NOTE: filename.mk:2: This continuation line should be indented "+ + "with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: Replacing \"\" with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: "+ + "Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\\t\\\\\" "+ + "with \"\\t\\t\\t\\t\\t\\t\\\\\".") + + // If the value is so wide that the continuation backslash cannot + // be kept in column 72, the line is still adjusted, and the + // continuation backslash is separated with a single space. + test( + "value\t\t\t\t\t\t\t......64\t\\", 24, 0, + "\t\t\tvalue\t\t\t\t\t\t\t......64 \\", + + "NOTE: filename.mk:2: This continuation line should be indented with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: Replacing \"\" with \"\\t\\t\\t\".", + "AUTOFIX: filename.mk:2: Replacing \"\\t\\\\\" with \" \\\\\".") +} + func (s *Suite) Test_varalignLine_alignValueMultiFollow__unindent_long_lines(c *check.C) { vt := NewVaralignTester(s, c) vt.Input( "SHORT=\tvalue", - "PROGRAM_AWK=\t\t\t\t--------50--------60--------70 \\", + "PROGRAM_AWK=\t\t\t\t........50........60........70 \\", "\t\t\t\t\t\t\t\t\t3 \\", "\t\t\t\t\t\t\t\t\t74 \\", "\t\t\t\t\t\t\t\t\t-75 \t\t\t \\", - "\t\t\t\t\t\t\t\t\t--76 \\", + "\t\t\t\t\t\t\t\t\t..76 \\", "\t\t\t\t\t\t\t\t66 \\", "\t\t\t\t\t\t\t\t1") vt.InputDetab( "SHORT= value", - "PROGRAM_AWK= --------50--------60--------70 \\", + "PROGRAM_AWK= ........50........60........70 \\", " 3 \\", " 74 \\", " -75 \\", - " --76 \\", + " ..76 \\", " 66 \\", " 1") vt.Internals( @@ -3808,12 +3913,8 @@ func (s *Suite) Test_varalignLine_alignV vt.Diagnostics( "NOTE: Makefile:1: This variable value should be aligned to column 17.", "NOTE: Makefile:2: This variable value should be aligned to column 17.", - // XXX: Wrong order; should be strictly from left to right. - "NOTE: Makefile:3: The continuation backslash should be preceded by a single space or tab.", "NOTE: Makefile:3: This continuation line should be indented with \"\\t\\t\\t\\t\\t\\t\".", - "NOTE: Makefile:4: The continuation backslash should be preceded by a single space or tab.", "NOTE: Makefile:4: This continuation line should be indented with \"\\t\\t\\t\\t\\t\\t\".", - "NOTE: Makefile:5: The continuation backslash should be preceded by a single space or tab.", "NOTE: Makefile:5: This continuation line should be indented with \"\\t\\t\\t\\t\\t\\t\".", "NOTE: Makefile:6: This continuation line should be indented with \"\\t\\t\\t\\t\\t\\t\".", "NOTE: Makefile:7: This continuation line should be indented with \"\\t\\t\\t\\t\\t\".", @@ -3821,12 +3922,12 @@ func (s *Suite) Test_varalignLine_alignV vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\\t\\t\\t\" with \"\\t\".", - "AUTOFIX: Makefile:3: Replacing \" \" with \" \".", "AUTOFIX: Makefile:3: Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\\t\" with \"\\t\\t\\t\\t\\t\\t\".", - "AUTOFIX: Makefile:4: Replacing \" \" with \" \".", + "AUTOFIX: Makefile:3: Replacing \" \\\\\" with \"\\t\\t\\t\\\\\".", "AUTOFIX: Makefile:4: Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\\t\" with \"\\t\\t\\t\\t\\t\\t\".", - "AUTOFIX: Makefile:5: Replacing \" \\t\\t\\t \" with \" \".", + "AUTOFIX: Makefile:4: Replacing \" \\\\\" with \"\\t\\t\\t\\\\\".", "AUTOFIX: Makefile:5: Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\\t\" with \"\\t\\t\\t\\t\\t\\t\".", + "AUTOFIX: Makefile:5: Replacing \" \\t\\t\\t \\\\\" with \"\\t\\t\\t\\\\\".", "AUTOFIX: Makefile:6: Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\\t\" with \"\\t\\t\\t\\t\\t\\t\".", "AUTOFIX: Makefile:7: Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\" with \"\\t\\t\\t\\t\\t\".", "AUTOFIX: Makefile:8: Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\" with \"\\t\\t\\t\\t\\t\".") @@ -3835,29 +3936,32 @@ func (s *Suite) Test_varalignLine_alignV // considered "long" anymore, therefore the backslashes are not // kept in column 72. Nevertheless they look unorganized right now. "SHORT= value", - "PROGRAM_AWK= --------50--------60--------70 \\", - " 3 \\", - " 74 \\", - " -75 \\", - " --76 \\", + "PROGRAM_AWK= ........50........60........70 \\", + " 3 \\", + " 74 \\", + " -75 \\", + " ..76 \\", " 66 \\", " 1") vt.Run() } +// In this example, the continued lines are indented less than the line +// containing the first value. This is not the common style, therefore all +// continuation lines are aligned to the value of the first line. func (s *Suite) Test_varalignLine_alignValueMultiFollow__unindent_long_initial_line(c *check.C) { vt := NewVaralignTester(s, c) vt.Input( - "VAR-----10!=\t\t----30--------40--------50-----6\t\t\t\\", - "\t\t --------30--------40-\t\t\t\t\\", - "\t\t --------30--------40--------50--------60-------8\t\\", - "\t\t ----5\t\t\t\t\t\t\\", + "VAR.....10!=\t\t....30........40........50....56\t\t\t\\", + "\t\t ........30........40.\t\t\t\t\\", + "\t\t ........30........40........50........60......68\t\\", + "\t\t ...25\t\t\t\t\t\t\\", "\t\t-7") vt.InputDetab( - "VAR-----10!= ----30--------40--------50-----6 \\", - " --------30--------40- \\", - " --------30--------40--------50--------60-------8 \\", - " ----5 \\", + "VAR.....10!= ....30........40........50....56 \\", + " ........30........40. \\", + " ........30........40........50........60......68 \\", + " ...25 \\", " -7") vt.Internals( "12 24 80", @@ -3868,26 +3972,48 @@ func (s *Suite) Test_varalignLine_alignV vt.Diagnostics( "NOTE: Makefile:1: The continuation backslash should be in column 73, not 81.", "NOTE: Makefile:2: This continuation line should be indented with \"\\t\\t\\t\".", + "NOTE: Makefile:3: This continuation line should be indented with \"\\t\\t\\t\".", "NOTE: Makefile:4: This continuation line should be indented with \"\\t\\t\\t\".", "NOTE: Makefile:5: This continuation line should be indented with \"\\t\\t\\t\".") vt.Autofixes( - // FIXME: Mention the continuation backslash in the replacement. "AUTOFIX: Makefile:1: Replacing \"\\t\\t\\t\" with \"\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\\t \" with \"\\t\\t\\t\".", + "AUTOFIX: Makefile:3: Replacing \"\\t\\t \" with \"\\t\\t\\t\".", + "AUTOFIX: Makefile:3: Replacing \"\\t\\\\\" with \" \\\\\".", "AUTOFIX: Makefile:4: Replacing \"\\t\\t \" with \"\\t\\t\\t\".", "AUTOFIX: Makefile:5: Replacing \"\\t\\t\" with \"\\t\\t\\t\".") vt.Fixed( - "VAR-----10!= ----30--------40--------50-----6 \\", - // FIXME: Preserve the original relative indentation. - " --------30--------40- \\", - // FIXME: Preserve the original relative indentation. - " --------30--------40--------50--------60-------8 \\", - // FIXME: Preserve the original relative indentation. - " ----5 \\", + "VAR.....10!= ....30........40........50....56 \\", + " ........30........40. \\", + " ........30........40........50........60......68 \\", + " ...25 \\", " -7") vt.Run() } +// The seemingly empty line 3 is actually a continuation from the line above it. +// Its indentation is is not fixed since that would lead to trailing whitespace. +func (s *Suite) Test_varalignLine_alignFollow(c *check.C) { + vt := NewVaralignTester(s, c) + vt.Input( + "VAR=\t\t...21 \\", + "\t\t...21 \\", + "") + vt.InputDetab( + "VAR= ...21 \\", + " ...21 \\", + "") + vt.Diagnostics( + nil...) + vt.Autofixes( + nil...) + vt.Fixed( + "VAR= ...21 \\", + " ...21 \\", + "") + vt.Run() +} + func (s *Suite) Test_varalignLine_alignContinuation(c *check.C) { t := s.Init(c) @@ -3907,14 +4033,10 @@ func (s *Suite) Test_varalignLine_alignC info.alignContinuation(valueColumn, rightMarginColumn) t.CheckEqualsf( - mkline.raw[rawIndex].text(), - condStr(autofix, after, before[rawIndex]), + mkline.raw[rawIndex].text(), after, "Line.raw.text, autofix=%v", autofix) - // As of 2019-12-11, the info fields are not updated - // accordingly, but they should. - // TODO: update info accordingly - t.CheckEqualsf(info.String(), before[rawIndex], + t.CheckEqualsf(info.String(), after, "info.String, autofix=%v", autofix) } @@ -4047,6 +4169,17 @@ func (s *Suite) Test_varalignLine_alignC // a single space, to keep it as close to the text as possible. test( lines( + "VAR=\t...13\t\t\t\t\t\t\t\t...77\t\\", + "\t...13"), + 0, 32, 48, + + "VAR=\t...13\t\t\t\t\t\t\t\t...77 \\", + "NOTE: filename.mk:1: The continuation backslash should be "+ + "preceded by a single space.", + "AUTOFIX: filename.mk:1: Replacing \"\\t\" with \" \".") + + test( + lines( "VAR=\t...13\t\t\t\t\t\t\t\t...77\t\t\t\\", "\t...13"), 0, 32, 48, @@ -4057,6 +4190,22 @@ func (s *Suite) Test_varalignLine_alignC "AUTOFIX: filename.mk:1: Replacing \"\\t\\t\\t\" with \" \".") } +func (s *Suite) Test_varalignLine_replaceSpaceBeforeValue(c *check.C) { + t := s.Init(c) + + // FIXME + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_varalignLine_replaceSpaceBeforeContinuationSilently(c *check.C) { + t := s.Init(c) + + // FIXME + + t.CheckOutputEmpty() +} + func (s *Suite) Test_varalignLine_explainWrongColumn(c *check.C) { t := s.Init(c) Index: pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.67 pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.68 --- pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.67 Sat Dec 14 18:04:16 2019 +++ pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go Mon Dec 16 17:06:05 2019 @@ -1641,7 +1641,8 @@ func (s *Suite) Test_VartypeCheck_SedCom "1d", "-e", "-i s,from,to,", - "-e s,$${unclosedShellVar") // Just for code coverage. + "-e s,$${unclosedShellVar", // Just for code coverage. + "-e s,...") // Syntactically invalid sed command. vt.Output( "NOTE: filename.mk:1: Please always use \"-e\" in sed commands, even if there is only one substitution.", @@ -1652,14 +1653,13 @@ func (s *Suite) Test_VartypeCheck_SedCom "ERROR: filename.mk:9: The -e option to sed requires an argument.", "WARN: filename.mk:10: Unknown sed command \"-i\".", "NOTE: filename.mk:10: Please always use \"-e\" in sed commands, even if there is only one substitution.", - // TODO: duplicate warning + // XXX: duplicate warning "WARN: filename.mk:11: Unclosed shell variable starting at \"$${unclosedShellVar\".", "WARN: filename.mk:11: Unclosed shell variable starting at \"$${unclosedShellVar\".") } func (s *Suite) Test_VartypeCheck_SedCommands__experimental(c *check.C) { vt := NewVartypeCheckTester(s.Init(c), BtSedCommands) - G.Experimental = true vt.Varname("SUBST_SED.dummy") @@ -2072,12 +2072,15 @@ func (s *Suite) Test_VartypeCheck_Yes(c vt.Varname("APACHE_MODULE") vt.Values( "yes", + "YES", "no", + "NO", "${YESVAR}") vt.Output( - "WARN: filename.mk:2: APACHE_MODULE should be set to YES or yes.", - "WARN: filename.mk:3: APACHE_MODULE should be set to YES or yes.") + "WARN: filename.mk:3: APACHE_MODULE should be set to YES or yes.", + "WARN: filename.mk:4: APACHE_MODULE should be set to YES or yes.", + "WARN: filename.mk:5: APACHE_MODULE should be set to YES or yes.") vt.Varname("BUILD_USES_MSGFMT") vt.Op(opUseMatch) @@ -2106,7 +2109,9 @@ func (s *Suite) Test_VartypeCheck_YesNo( vt.Varname("PKG_DEVELOPER") vt.Values( "yes", + "YES", "no", + "NO", "ja", "${YESVAR}", "yes # comment", @@ -2114,9 +2119,9 @@ func (s *Suite) Test_VartypeCheck_YesNo( "Yes indeed") vt.Output( - "WARN: filename.mk:3: PKG_DEVELOPER should be set to YES, yes, NO, or no.", - "WARN: filename.mk:4: PKG_DEVELOPER should be set to YES, yes, NO, or no.", - "WARN: filename.mk:7: PKG_DEVELOPER should be set to YES, yes, NO, or no.") + "WARN: filename.mk:5: PKG_DEVELOPER should be set to YES, yes, NO, or no.", + "WARN: filename.mk:6: PKG_DEVELOPER should be set to YES, yes, NO, or no.", + "WARN: filename.mk:9: PKG_DEVELOPER should be set to YES, yes, NO, or no.") vt.Op(opUseMatch) vt.Values( --_----------=_157651596543580--