Hold the three exit-code declarations to one contract (#125)

cmd/lab/main.go

@@ -22,6 +22,16 @@ import (
 // harness and is declared where its producer is, in internal/hardware. Record
 // 0011 refuses a code written into the runner as a branch nothing can reach,
 // and that is what one here would be.
+//
+// WHAT HOLDS THIS DECLARATION TO THE OTHERS. Keeping a code beside its producer
+// means the contract is written down in more than one place, and each place was
+// right about itself while nothing was right about all of them together. The
+// exit-code leg in internal/invariants reads every declaration in the tree and
+// refuses a code declared with two numbers, or a number declared under two
+// codes, so a number that moves here is red in the default suite rather than
+// green in this package. It finds a declaration by the shape of its name, so a
+// code named something that does not read like one is invisible to it: keep the
+// convention these three follow.
 const (
 	// exitClean means the run completed and refused nothing. It does not
 	// mean the tree is good, only that nothing this runner judges was found

cmd/pullrequest/main.go

@@ -30,6 +30,15 @@ import (
 // they are written here with the record named rather than invented. The
 // integration-hardware harness declares its own code where its producer is, and
 // this follows that shape: a code lives next to the thing that can return it.
+//
+// WHAT HOLDS THIS DECLARATION TO THE OTHERS. The exit-code leg in
+// internal/invariants reads every exit-code constant in the tree and refuses a
+// code declared with two numbers, or a number declared under two codes. Without
+// it these three numbers being the same as the runner's is a thing a reader
+// checked once, and a number that moved here would compile, pass this package's
+// own tests and change the meaning of anything keyed on it. It finds a
+// declaration by the shape of its name, so keep the convention these three
+// follow.
 const (
 	// exitClean means the run completed and refused nothing. It does not mean
 	// the pull request is good, only that nothing this check judges was found

internal/hardware/hardware.go

@@ -42,6 +42,15 @@ const Name = "integration-hardware"
 // returns. It lives here rather than beside those three because this is where
 // its producer is, and that record refuses a code written into the runner as a
 // branch nothing can reach.
+//
+// WHAT HOLDS THIS DECLARATION TO THE OTHERS. The exit-code leg in
+// internal/invariants reads every exit-code constant in the tree and refuses a
+// number declared under two codes, which is the direction that reaches this
+// one: it is declared exactly once, so there is no second copy of it for
+// anything to compare against, and what can go wrong is this number colliding
+// with a meaning one of the runner's three already has. It finds a declaration
+// by the shape of its name, so keep the convention this one and those three
+// follow.
 const ExitAskedAndDeliveredNothing = 3
 
 // BuildTag is the constraint its tests are behind. The default run does not

internal/invariants/exitcodes.go

@@ -0,0 +1,214 @@
+// The leg that holds record 0011's contract to itself.
+//
+// The numbers are declared in more than one place on purpose. A code lives next
+// to the thing that can return it, which is what keeps the code the hardware
+// harness returns out of the runner rather than written there as a branch
+// nothing can reach. What that shape costs is that each declaration is right
+// about itself and nothing was right about all of them together: a number that
+// moved in one of them compiled, passed the tests of the package it moved in,
+// and changed the meaning of every reader keyed on it without their files being
+// edited.
+//
+// This leg is the other half of that shape rather than a replacement for it.
+// The declarations stay where their producers are and this reads them all and
+// requires them to say one thing.
+
+package invariants
+
+import (
+	"fmt"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"regexp"
+	"sort"
+	"strconv"
+	"strings"
+)
+
+// goExtension is what this leg reads. Everything else the scan carries is prose
+// and declares no constants.
+const goExtension = ".go"
+
+// exitCodeName is how a declaration of one of record 0011's codes is
+// recognised. It is a naming convention read out of the tree rather than a list
+// of files, so a fourth declaration written somewhere nobody has thought of yet
+// is held to the same contract as the three that exist, and this leg does not
+// have to be edited to notice it.
+//
+// The cost of a convention is that it reaches only what follows it. A code
+// declared under a name that does not read like one is invisible here, and
+// nothing in this tree can see that it is a code, which is why the convention is
+// stated at each declaration rather than only in this file.
+var exitCodeName = regexp.MustCompile(`^[Ee]xit[A-Z]`)
+
+// An exitCodeDeclaration is one exit-code constant as one file declares it.
+type exitCodeDeclaration struct {
+	// name is the constant, which is what groups two declarations of one code.
+	name string
+
+	// value is the number, which is what the contract fixes.
+	value int
+
+	// path is the file as the walk reached it, which is what a reader opens.
+	path string
+
+	// line is where in that file, because a reader hitting this refusal is
+	// looking for two numbers and needs to be sent to both of them.
+	line int
+}
+
+// exitCodesLeg holds every declaration of record 0011's contract to every other
+// one, in both directions a disagreement can run.
+//
+// THE BOUND, and it is the one this package's harness already carries for the
+// notice leg. Both directions refuse under one property, because the repair is
+// one repair: make the declarations say what the record says. A case comparing
+// property sets cannot tell them apart, so a change that loses one of the two
+// stays green on the other's case. Each direction has its own case anyway, and
+// that is a convention here rather than something the harness requires.
+func exitCodesLeg(texts []textFile) (Leg, []Refusal) {
+	declarations, examined := exitCodeDeclarations(texts)
+	leg := Leg{Name: "the exit codes", Asked: true, Examined: examined}
+
+	var refusals []Refusal
+	refusals = append(refusals, oneCodeWithTwoNumbers(declarations)...)
+	refusals = append(refusals, oneNumberWithTwoCodes(declarations)...)
+	return leg, refusals
+}
+
+// oneCodeWithTwoNumbers refuses a code whose declarations do not agree on the
+// number. This is the drift the shape invites: the constant is spelled the same
+// everywhere, so a reader comparing two files by eye is looking at the one
+// character that differs.
+func oneCodeWithTwoNumbers(declarations []exitCodeDeclaration) []Refusal {
+	first := make(map[string]exitCodeDeclaration)
+	var refusals []Refusal
+
+	for _, declaration := range declarations {
+		reference, seen := first[declaration.name]
+		if !seen {
+			first[declaration.name] = declaration
+			continue
+		}
+		if declaration.value == reference.value {
+			continue
+		}
+		refusals = append(refusals, Refusal{
+			Property: ExitCodeDeclarationsDisagree,
+			Subject:  declaration.path,
+			Detail: fmt.Sprintf("it declares %s as %d at line %d, and %s declares the same code as %d, so the contract record 0011 fixes has two numbers in this tree",
+				declaration.name, declaration.value, declaration.line, reference.path, reference.value),
+		})
+	}
+	return refusals
+}
+
+// oneNumberWithTwoCodes refuses one number declared under two codes. This is
+// the direction that reaches a code declared exactly once, which the direction
+// above cannot: a number written into a new constant collides with the meaning
+// the record already gave it, and there is no second declaration of that
+// constant for anything to compare it against.
+func oneNumberWithTwoCodes(declarations []exitCodeDeclaration) []Refusal {
+	first := make(map[int]exitCodeDeclaration)
+	var refusals []Refusal
+
+	for _, declaration := range declarations {
+		reference, seen := first[declaration.value]
+		if !seen {
+			first[declaration.value] = declaration
+			continue
+		}
+		if declaration.name == reference.name {
+			continue
+		}
+		refusals = append(refusals, Refusal{
+			Property: ExitCodeDeclarationsDisagree,
+			Subject:  declaration.path,
+			Detail: fmt.Sprintf("it declares %s as %d at line %d, and %s declares %s as the same number, so one code in record 0011's contract answers to two names",
+				declaration.name, declaration.value, declaration.line, reference.path, reference.name),
+		})
+	}
+	return refusals
+}
+
+// exitCodeDeclarations reads every Go file the scan carries and returns the
+// exit-code constants it declares, with how many files were parsed.
+//
+// It reads the text the scan already read rather than the filesystem again, so
+// the declarations it compares are the ones in the tree the report is about.
+func exitCodeDeclarations(texts []textFile) ([]exitCodeDeclaration, int) {
+	var found []exitCodeDeclaration
+	parsed := 0
+
+	for _, file := range texts {
+		if !strings.HasSuffix(file.relative, goExtension) {
+			continue
+		}
+		fset := token.NewFileSet()
+		syntax, err := parser.ParseFile(fset, file.relative, file.text, 0)
+		if err != nil {
+			// A file the parser cannot read is not judged here. The compiler
+			// refuses it, and a leg that refused it too would be a second copy
+			// of a rule that already has a better mechanism.
+			continue
+		}
+		parsed++
+
+		for _, declaration := range syntax.Decls {
+			group, ok := declaration.(*ast.GenDecl)
+			if !ok || group.Tok != token.CONST {
+				continue
+			}
+			for _, spec := range group.Specs {
+				values, ok := spec.(*ast.ValueSpec)
+				if !ok {
+					continue
+				}
+				for i, name := range values.Names {
+					if i >= len(values.Values) || !exitCodeName.MatchString(name.Name) {
+						continue
+					}
+					number, ok := integerLiteral(values.Values[i])
+					if !ok {
+						continue
+					}
+					found = append(found, exitCodeDeclaration{
+						name:  name.Name,
+						value: number,
+						path:  file.path,
+						line:  fset.Position(name.Pos()).Line,
+					})
+				}
+			}
+		}
+	}
+
+	sort.Slice(found, func(i, j int) bool {
+		if found[i].path != found[j].path {
+			return found[i].path < found[j].path
+		}
+		return found[i].line < found[j].line
+	})
+	return found, parsed
+}
+
+// integerLiteral reads a constant's value where that value is a plain number,
+// and says no for everything else.
+//
+// A constant whose name reads like an exit code and whose value is not a number
+// is not an exit code. That is not a hypothetical: the property identifier this
+// leg refuses under is spelled ExitCodeDeclarationsDisagree, it sits in this
+// package, and a leg that took every matching name would refuse this repository
+// for declaring the rule it is refusing under.
+func integerLiteral(expression ast.Expr) (int, bool) {
+	literal, ok := expression.(*ast.BasicLit)
+	if !ok || literal.Kind != token.INT {
+		return 0, false
+	}
+	number, err := strconv.Atoi(literal.Value)
+	if err != nil {
+		return 0, false
+	}
+	return number, true
+}

internal/invariants/invariants.go

@@ -116,6 +116,14 @@ const (
 	// this for experiment records; this is the same rule over the documents a
 	// visitor reads first, which is where a dead pointer does the most damage.
 	DocumentNamesAPathThatDoesNotResolve = "document-names-a-path-that-does-not-resolve"
+
+	// ExitCodeDeclarationsDisagree refuses a tree whose exit-code constants do
+	// not say one thing. Record 0011 fixes four numbers and their meanings, and
+	// the tree declares them in more than one place because a code lives next
+	// to the thing that can return it. Each declaration is right about itself
+	// and nothing held them to each other, so a number that moved in one of
+	// them compiled and stayed green in the package it moved in.
+	ExitCodeDeclarationsDisagree = "exit-code-declarations-disagree"
 )
 
 // A Refusal is one rule refusing one subject. It carries the subject separately
@@ -226,6 +234,10 @@ func Scan(root, declaredLicence string) (Report, error) {
 	rep.Legs = append(rep.Legs, leg)
 	rep.Refusals = append(rep.Refusals, refusals...)
 
+	leg, refusals = exitCodesLeg(texts)
+	rep.Legs = append(rep.Legs, leg)
+	rep.Refusals = append(rep.Refusals, refusals...)
+
 	return rep, nil
 }
 

testdata/invariants/an-exit-code-declared-with-two-numbers/expected

@@ -0,0 +1 @@
+text-files 4

testdata/invariants/an-exit-code-declared-with-two-numbers/expected-refusals

@@ -0,0 +1 @@
+exit-code-declarations-disagree

testdata/invariants/an-exit-code-declared-with-two-numbers/near-neighbour

@@ -0,0 +1 @@
+exit-code-declarations-that-agree

testdata/invariants/an-exit-code-declared-with-two-numbers/tree/NOTICE.md

@@ -0,0 +1,5 @@
+# Notice
+
+This software is developed for lawful use. Operators and users are responsible
+for making sure that their deployment and use comply with the laws that apply
+to them.

testdata/invariants/an-exit-code-declared-with-two-numbers/tree/README.md

@@ -0,0 +1,3 @@
+# a tree
+
+See [NOTICE.md](NOTICE.md) for the intended-use notice.

testdata/invariants/an-exit-code-declared-with-two-numbers/tree/cmd/one/main.go

@@ -0,0 +1,10 @@
+// Command one is a fixture. It declares two of the codes the contract fixes,
+// beside the thing that returns them.
+package main
+
+const (
+	exitClean   = 0
+	exitRefused = 1
+)
+
+func main() {}

testdata/invariants/an-exit-code-declared-with-two-numbers/tree/cmd/two/main.go

@@ -0,0 +1,11 @@
+// Command two is a fixture. It spells the second code the way command one
+// spells it and gives it a different number, which is the drift a reader
+// comparing the two files by eye has to catch one character of.
+package main
+
+const (
+	exitClean   = 0
+	exitRefused = 5
+)
+
+func main() {}

testdata/invariants/exit-code-declarations-that-agree/expected

@@ -0,0 +1 @@
+text-files 4

testdata/invariants/exit-code-declarations-that-agree/tree/NOTICE.md

@@ -0,0 +1,5 @@
+# Notice
+
+This software is developed for lawful use. Operators and users are responsible
+for making sure that their deployment and use comply with the laws that apply
+to them.

testdata/invariants/exit-code-declarations-that-agree/tree/README.md

@@ -0,0 +1,3 @@
+# a tree
+
+See [NOTICE.md](NOTICE.md) for the intended-use notice.

testdata/invariants/exit-code-declarations-that-agree/tree/cmd/one/main.go

@@ -0,0 +1,10 @@
+// Command one is a fixture. It declares two of the codes the contract fixes,
+// beside the thing that returns them.
+package main
+
+const (
+	exitClean   = 0
+	exitRefused = 1
+)
+
+func main() {}

testdata/invariants/exit-code-declarations-that-agree/tree/cmd/two/main.go

@@ -0,0 +1,10 @@
+// Command two is a fixture. It declares the same two codes as command one,
+// with the same numbers, which is the whole of what this leg asks for.
+package main
+
+const (
+	exitClean   = 0
+	exitRefused = 1
+)
+
+func main() {}

testdata/invariants/one-number-declared-under-two-exit-codes/expected

@@ -0,0 +1 @@
+text-files 4

testdata/invariants/one-number-declared-under-two-exit-codes/expected-refusals

@@ -0,0 +1 @@
+exit-code-declarations-disagree

testdata/invariants/one-number-declared-under-two-exit-codes/near-neighbour

@@ -0,0 +1 @@
+exit-code-declarations-that-agree

testdata/invariants/one-number-declared-under-two-exit-codes/tree/NOTICE.md

@@ -0,0 +1,5 @@
+# Notice
+
+This software is developed for lawful use. Operators and users are responsible
+for making sure that their deployment and use comply with the laws that apply
+to them.

testdata/invariants/one-number-declared-under-two-exit-codes/tree/README.md

@@ -0,0 +1,3 @@
+# a tree
+
+See [NOTICE.md](NOTICE.md) for the intended-use notice.

testdata/invariants/one-number-declared-under-two-exit-codes/tree/cmd/one/main.go

@@ -0,0 +1,10 @@
+// Command one is a fixture. It declares two of the codes the contract fixes,
+// beside the thing that returns them.
+package main
+
+const (
+	exitClean   = 0
+	exitRefused = 1
+)
+
+func main() {}