diff --git a/cmd/lab/main.go b/cmd/lab/main.go index e9b9be3..3b56333 100644 --- a/cmd/lab/main.go +++ b/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 diff --git a/cmd/pullrequest/main.go b/cmd/pullrequest/main.go index 6a27481..eb51a98 100644 --- a/cmd/pullrequest/main.go +++ b/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 diff --git a/internal/hardware/hardware.go b/internal/hardware/hardware.go index 27b367f..5b781b8 100644 --- a/internal/hardware/hardware.go +++ b/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 diff --git a/internal/invariants/exitcodes.go b/internal/invariants/exitcodes.go new file mode 100644 index 0000000..c5161c1 --- /dev/null +++ b/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 +} diff --git a/internal/invariants/invariants.go b/internal/invariants/invariants.go index 256be17..fd7d11d 100644 --- a/internal/invariants/invariants.go +++ b/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 } diff --git a/testdata/invariants/an-exit-code-declared-with-two-numbers/expected b/testdata/invariants/an-exit-code-declared-with-two-numbers/expected new file mode 100644 index 0000000..3d7cc38 --- /dev/null +++ b/testdata/invariants/an-exit-code-declared-with-two-numbers/expected @@ -0,0 +1 @@ +text-files 4 diff --git a/testdata/invariants/an-exit-code-declared-with-two-numbers/expected-refusals b/testdata/invariants/an-exit-code-declared-with-two-numbers/expected-refusals new file mode 100644 index 0000000..34fe4fd --- /dev/null +++ b/testdata/invariants/an-exit-code-declared-with-two-numbers/expected-refusals @@ -0,0 +1 @@ +exit-code-declarations-disagree diff --git a/testdata/invariants/an-exit-code-declared-with-two-numbers/near-neighbour b/testdata/invariants/an-exit-code-declared-with-two-numbers/near-neighbour new file mode 100644 index 0000000..be8ea6c --- /dev/null +++ b/testdata/invariants/an-exit-code-declared-with-two-numbers/near-neighbour @@ -0,0 +1 @@ +exit-code-declarations-that-agree diff --git a/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/NOTICE.md b/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/NOTICE.md new file mode 100644 index 0000000..e3cfa99 --- /dev/null +++ b/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. diff --git a/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/README.md b/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/README.md new file mode 100644 index 0000000..315a136 --- /dev/null +++ b/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. diff --git a/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/cmd/one/main.go b/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/cmd/one/main.go new file mode 100644 index 0000000..266b242 --- /dev/null +++ b/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() {} diff --git a/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/cmd/two/main.go b/testdata/invariants/an-exit-code-declared-with-two-numbers/tree/cmd/two/main.go new file mode 100644 index 0000000..ce329ff --- /dev/null +++ b/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() {} diff --git a/testdata/invariants/exit-code-declarations-that-agree/expected b/testdata/invariants/exit-code-declarations-that-agree/expected new file mode 100644 index 0000000..3d7cc38 --- /dev/null +++ b/testdata/invariants/exit-code-declarations-that-agree/expected @@ -0,0 +1 @@ +text-files 4 diff --git a/testdata/invariants/exit-code-declarations-that-agree/expected-refusals b/testdata/invariants/exit-code-declarations-that-agree/expected-refusals new file mode 100644 index 0000000..e69de29 diff --git a/testdata/invariants/exit-code-declarations-that-agree/tree/NOTICE.md b/testdata/invariants/exit-code-declarations-that-agree/tree/NOTICE.md new file mode 100644 index 0000000..e3cfa99 --- /dev/null +++ b/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. diff --git a/testdata/invariants/exit-code-declarations-that-agree/tree/README.md b/testdata/invariants/exit-code-declarations-that-agree/tree/README.md new file mode 100644 index 0000000..315a136 --- /dev/null +++ b/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. diff --git a/testdata/invariants/exit-code-declarations-that-agree/tree/cmd/one/main.go b/testdata/invariants/exit-code-declarations-that-agree/tree/cmd/one/main.go new file mode 100644 index 0000000..266b242 --- /dev/null +++ b/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() {} diff --git a/testdata/invariants/exit-code-declarations-that-agree/tree/cmd/two/main.go b/testdata/invariants/exit-code-declarations-that-agree/tree/cmd/two/main.go new file mode 100644 index 0000000..ee71c4d --- /dev/null +++ b/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() {} diff --git a/testdata/invariants/one-number-declared-under-two-exit-codes/expected b/testdata/invariants/one-number-declared-under-two-exit-codes/expected new file mode 100644 index 0000000..3d7cc38 --- /dev/null +++ b/testdata/invariants/one-number-declared-under-two-exit-codes/expected @@ -0,0 +1 @@ +text-files 4 diff --git a/testdata/invariants/one-number-declared-under-two-exit-codes/expected-refusals b/testdata/invariants/one-number-declared-under-two-exit-codes/expected-refusals new file mode 100644 index 0000000..34fe4fd --- /dev/null +++ b/testdata/invariants/one-number-declared-under-two-exit-codes/expected-refusals @@ -0,0 +1 @@ +exit-code-declarations-disagree diff --git a/testdata/invariants/one-number-declared-under-two-exit-codes/near-neighbour b/testdata/invariants/one-number-declared-under-two-exit-codes/near-neighbour new file mode 100644 index 0000000..be8ea6c --- /dev/null +++ b/testdata/invariants/one-number-declared-under-two-exit-codes/near-neighbour @@ -0,0 +1 @@ +exit-code-declarations-that-agree diff --git a/testdata/invariants/one-number-declared-under-two-exit-codes/tree/NOTICE.md b/testdata/invariants/one-number-declared-under-two-exit-codes/tree/NOTICE.md new file mode 100644 index 0000000..e3cfa99 --- /dev/null +++ b/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. diff --git a/testdata/invariants/one-number-declared-under-two-exit-codes/tree/README.md b/testdata/invariants/one-number-declared-under-two-exit-codes/tree/README.md new file mode 100644 index 0000000..315a136 --- /dev/null +++ b/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. diff --git a/testdata/invariants/one-number-declared-under-two-exit-codes/tree/cmd/one/main.go b/testdata/invariants/one-number-declared-under-two-exit-codes/tree/cmd/one/main.go new file mode 100644 index 0000000..266b242 --- /dev/null +++ b/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() {} diff --git a/testdata/invariants/one-number-declared-under-two-exit-codes/tree/cmd/two/main.go b/testdata/invariants/one-number-declared-under-two-exit-codes/tree/cmd/two/main.go new file mode 100644 index 0000000..de8962f --- /dev/null +++ b/testdata/invariants/one-number-declared-under-two-exit-codes/tree/cmd/two/main.go @@ -0,0 +1,11 @@ +// Command two is a fixture. It gives the number command one already uses a +// second name, so the contract carries one code answering to two of them and +// neither declaration is wrong about itself. +package main + +const ( + exitClean = 0 + exitDenied = 1 +) + +func main() {}