Repository navigation
story(cli): a copybook composed with COPY resolves through the layout's copy-member forms, and init converts it as its expansion - #432
Conversation
…e literals it now reads Moves cobol-go to 28b9286a090c (Zaba505/cobol-go#153) and follows CONTRIBUTING's "Bumping the cobol-go pin": a prefixed literal is literal text with its prefix, an ALL literal is (all <repeated>), a word that begins with a digit is (word "..."), and VALUES ARE converts with ARE as noise. The misread refusal is withdrawn, the kitchen sink and the fingerprint are extended, and EntryDepths, CheckEntries and Positioned replace what this repository restated. Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ber forms One copymember.Resolver over a member map, matched by text-name and library-name without regard to case and read once per run, expands every copybook a run reads. The schema declares copy-member and of; the layout reader reports two forms naming one member, and the run reports a COPY naming no form, a missing member file, a cycle and an unreached form, with every span into a member carrying the chain of COPY statements that reached it. Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… as its expansion Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ity suites Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ecs record the pin Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ument Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review of #432 (COPY members through copy-member / --copy-member, cobol-go pin to 28b9286a090c)
I read the whole diff. Most of it holds up, and these parts match the acceptance criteria and the spec text the diff adds:
- The resolver. Matching has no fallback between a qualified and an unqualified member. Each file is read once per resolver.
CopyChainis walked innermost first. Tests cover matching, nesting, a cycle and REPLACING. - The layout side. The
copy-memberandofschema forms are in, the top-level forms table has its new row, and a form named twice is reported with two spans. - The notation work that came with the pin. A prefixed literal is text with an upper-case prefix, and a lower-case prefix is a diagnostic.
(all …)is a form. A word that begins with a digit is written(word …).VALUES AREis read as noise. The misread refusal is gone. The writer, the reader,constructand the schema agree on each of these. - The API claims I checked against the cobol-go module.
Pos.Text,Pos.CopyChain,LibraryText,CopyBookNotFoundErrorandCopyBookCycleErroras value types,Positioned,CheckEntries/EntryCheckError,EntryDepths, andLiteralKind.Prefixcovering every declared kind are all used as the library defines them.
I have three findings, all inline:
- (correctness)
initnotes members as named by noCOPYwhen the parse simply stopped before reaching them. The run guards against exactly this case andinitdoes not. - (error path) A member file that exists but cannot be read is reported as
no copy member at …, and the real OS error is dropped, in both the run andinit. - (diagnostics/API)
init's notes and errors about a--copy-memberclaim to quote the value as typed. They actually rebuild it from the parsed member, which loses literal delimiters andIN, and can print a value that splits differently.
Two smaller gaps I could not settle from the diff alone:
- NewLayout faults in a member. docs/cli/SPEC.md now says every positioned error,
NewLayout's included, points into the member. In a run, only theParseandcopybook.Buildfaults of a copybook-bound record go throughmemberSpan. No test covers acopybook.NewLayout(layout) fault raised by an entry a member contributed. If that path still rendersrecord.Pathwith the member's line and column, it points at a line in the copybook that says something else. - Mapping rows in docs/cli/SPEC.md. The diff changes no mapping row there. The layout SPEC rows already named #423 before this change, so this may already be done; worth confirming against the last acceptance criterion.
| files = append(files, File{Name: DefinitionsName(book.Path), Bytes: text}) | ||
| } | ||
|
|
||
| notes := unnamedMembers(resolver) |
There was a problem hiding this comment.
False notes on failure. unnamedMembers is computed, and attached to the refusal, even when a copybook's parse stopped partway. cobol-go expands COPY as it tokenizes, so every COPY after the first parse error in that copybook is never resolved, and its member is never marked reached.
Example: a copybook writes COPY STDHRD. and then COPY ORDDTL., with --copy-member STDHDR=… --copy-member ORDDTL=…. You get the refusal for STDHRD, plus a note saying no COPY in any copybook names ORDDTL. That note is false, and it sits right beside a refusal it seems to explain.
The run has a guard for exactly this (if !unread { b.unreached(members) } in internal/project/copybook.go, whose comment calls the alternative "a fault this program invented"). init should apply the same rule: drop the unreached notes, or at least say they are incomplete, when any copybook failed to parse. None of the new tests covers it. TestACopyNamingNoMemberIsARefusalWithTheNoteBesideIt puts the misspelled COPY last, so no later member goes unreached.
There was a problem hiding this comment.
Fixed in 6d3dc57. If any copybook was refused, an unreached member is now noted as named by no COPY read before the refusal, and the note adds that a COPY after the refusal may still name it. I kept the note instead of dropping it, because docs/cli/SPEC.md wants it next to the refusal so a misspelling reads as one. It just no longer claims more than the stopped read could know. TestANoteAfterARefusalSaysTheReadingStopped covers the case you describe: a misspelled COPY first, then COPY ORDDTL.
| } | ||
|
|
||
| notes = append(notes, fmt.Sprintf( | ||
| "--copy-member %s=%s: no COPY in any copybook names %s, so %s was not read", |
There was a problem hiding this comment.
The note does not quote the value as typed. The note is documented as "naming the value as typed" (here, in the CLI's copyMember doc, and in TestAMemberNoCopyNamesIsNoted). It actually rebuilds the value from form.Member.String() and form.Path, and cmd/cpybkc/scaffold.go drops given.value when it builds the copymember.Form.
So 'stdhdr'=x.cpy is noted as --copy-member stdhdr=x.cpy, and HDR IN PAYLIB=… as HDR OF PAYLIB=…. For 'A=B'=x.cpy the note prints --copy-member A=B=x.cpy, which the CLI would split at a different =, so the person cannot paste it back.
MissingMemberError and MemberTwiceError in internal/scaffold/errors.go have the same issue. Either carry the typed value on the form (a field next to Path), or stop claiming the value is quoted as typed.
There was a problem hiding this comment.
Fixed in 6d3dc57. copymember.Form gains Given, the --copy-member value as typed, and the CLI sets it. The unreached note, MissingMemberError and MemberTwiceError all quote it through Form.Typed(). A layout form has no typed value, so Typed() falls back to member=path. TestANoteQuotesTheValueAsTyped pins 'UNUSED' IN PAYLIB=unused.cpy.
| // Diagnostic is what the error says, and where. | ||
| func (e *MissingCopyMemberError) Diagnostic() diag.Diagnostic { | ||
| return diag.Diagnostic{ | ||
| Message: "no copy member at " + e.Path, |
There was a problem hiding this comment.
The cause of an unreadable member is dropped. MissingCopyMemberError.Diagnostic never renders e.Err. copymember.UnreadableError.Unwrap makes every read failure look like fs.ErrNotExist, so FaultOf sends it here: a permission error, a path that names a directory, or an I/O error.
A member file that exists but cannot be read is therefore reported as no copy member at cpy/x.cpy / looked for /abs/cpy/x.cpy. The person then finds the file exactly where the message says it is missing. Either tell "not there" (errors.Is(e.Err, fs.ErrNotExist)) apart from "there but unreadable", or at least put the OS error on a continuation line. scaffold.MissingMemberError (internal/scaffold/errors.go:107) has the same problem. The only tests here use files that are missing, so nothing exercises this path.
There was a problem hiding this comment.
Fixed in 6d3dc57. A member path that exists but can't be read (a directory, no permission, an I/O error) is now reported as the copy member at … could not be read, followed by looked for … and the OS error. no copy member at … is kept for fs.ErrNotExist only. The same applies to scaffold.MissingMemberError. Both run and init tests cover this with a directory as the member path. Your summary's NewLayout point is fixed in the same commit: a layout fault at an entry a member contributed is now a MemberFaultError that points into the member with its chain (TestALayoutFaultInAMemberPointsIntoTheMember). On the mapping rows: the cli SPEC rows naming #407 already named #423 as implemented before this PR.
A member file that is there and unreadable is said to be that, with its cause; a layout fault at an entry a member contributed points into the member; init quotes a --copy-member as typed and, after a refusal, notes an unreached member as unread rather than unnamed. Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Refs #423 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Closes #423
A copybook that composes with
COPYnow resolves through the layout'scopy-memberforms, andinitconverts it to its expansion. The owner asked for the cobol-go update to land with this work. Member provenance (Zaba505/cobol-go#153) is the newest commit there, so moving the pin to it also brings in #144–#152. This PR adopts all of them in the Definitions notation instead of continuing to refuse constructs cobol-go now reads.Notation decisions
These are written into
docs/layout/SPEC.mdand listed here for review.VALUE X '00') are still two values:(value X "'00'").MisreadValueErroris deleted."X'00'","N'AB'","Z'ABC'","H'FF'","O'17'","B'101'". This is the existing leaf rule unchanged ("every COBOL literal token is text holding exactly that token"), because cobol-go now reads one as a single token. The old SPEC text already pointed this way ("an entry body cannot write one either, since"X'00'"is two tokens to the pinned cobol-go"). The prefix is always upper case. cobol-go reads a prefix case-insensitively and stores it asKind, not as text, so its printer writes upper case. A lower-case prefix in an entry body is aLiteralTextError, which keeps one spelling per literal. The body after the prefix stays as written:X'c1'→"X'c1'".(all <repeated>).<repeated>is literal text or a data-name (a figurative constant):(value (all "'*'")),(value (all "X'FF'")),(value (all SPACES)). It is allowed wherever a value operand is, including either end of athrough. It is notALLwritten beside its operand, because that would be two values (the old misreading). It is not text either, becauseALL '*'is two tokens. This matches cobol-go'sAllLiteralnode. Which operandsALLmay precede is cobol-go's call: the judge refuses(all "5")and(all "H'FF'"). The schema declaresallincobol-operand, and a newrepeatablesort covers its argument. The schema stays at version 1.(word "1ST-FIELD"). The data-name rule is unchanged. The leaf rule used to say "today means onlynil". It now saysniland every word beginning with a digit, since sexpr-go reads none of those as a symbol (checked for1ST-FIELD,1000-INIT,9A,1E5and others). This applies in a layout's item positions too.VALUES AREneeds no notation.AREis a noise word likeIS, and is not written.COPYresolves through the layout'scopy-memberforms, andinitconverts it as its expansion #423, each with its new spelling.cobol.EntryDepths(story(cli): wire the generation pipeline behind the CLI #148) replaces the writer's own copy of the depth rule.cobol.CheckEntries(story(cli): expose --emit-ir and --emit-ir-format on the CLI #149) is the judge's validity rule, still asked one entry at a time so each refusal is reported where it occurs.cobol.Positioned(story(cli): scaffold cmd/cpybkc with version and help #147) replaces the list of ten named error types inbuildFault.As CONTRIBUTING's Bumping the cobol-go pin requires, the kitchen sink and the fingerprint were updated in this PR. The kitchen sink gains every prefix kind, both delimiters, ALL around a literal, a hex literal and a figurative constant,
VALUES ARE,VALUE IS, and two digit-led words. The fingerprint gainsAllLiteralandStringLiteral.Kind. The four failinginternal/definitionstests were rewritten to match the new behaviour, not deleted:TestTheWriterKnowsEveryClauseandTestTheNotationsTypesHaveNotChangednow includeAllLiteralandKind.TestConversionTranscribesWhatCobolGoOnceMisread.X'0', an odd digit count that the tokenizer refuses.Acceptance criteria
One resolver.
internal/copymember.Resolverworks over a member map. It matches text-name and library-name without regard to case, with no fallback between the qualified and unqualified shapes. It reads each member file once per run and never opens one that noCOPYnames. Bothcobol.Parsecall sites pass it throughWithCopyBooks:internal/project/copybook.gofor a run andinternal/scaffold/derive.goforinit. The only othercobol.Parseis the judge re-reading its own printed entries, and those never containCOPY. Tests cover matching, nesting, a cycle,REPLACING(including the as-written column after a substitution changes a word's length), and reading once.The layout form.
schema/layout.sexprdeclarescopy-member(top-level,zero-or-more, record definitions) and(of <text-name> <library-name>), still at version 1. Its row is in The top-level forms.layoutmodel.ReadCopyMembersreports two forms naming one member with two spans. The run reports a form noCOPYreaches as an error at its member and leaves its file unopened. A layout whose records name no copybook has only unreached forms. Unreached forms are not reported when some copybook could not be read, because what it would have copied is unknown.A run. A copybook that writes
COPYresolves through the forms, with paths relative to the layout. What aCOPYfault looks like is implemented and tested:COPYthat names no form, reported at the statement with "the layout names …".no copy member at …pluslooked for <absolute>, at the form's path), reported once however many copybooks copy it.COPYthat closes it, with the open members listed outermost first.Spans into a member.
resolve.Sourcecarries the resolver, so every span into an item that a member contributed points into the member's file, as itscopy-memberform spells it.diag.SpangainsVia *Chain, which adds one continuation line perCOPY, innermost first. It is a pointer soSpanstays comparable. cobol-go faults inside a member (parse andcopybook.Build) do the same. The pin is at 28b9286a090c, bumped per CONTRIBUTING, and the go.mod comment says what came with it.init.--copy-memberbehaves as Where the members come from states:=outside a literal, with doubled delimiters undoubled.=, an empty or malformed member, an empty path, the path-, and two members that aCOPYcannot tell apart.COPYnames becomes anote:. It is printed even when the run is refused, throughscaffold.Noted, so a misspelling shows up next to theCOPYrefusal it causes.COPYrefusals are reported in the run's shapes, worded for the command line.usageandinitUsagedescribe the flag. The synopsis is updated in docs/cli/SPEC.md and main.go.The Dagger module mirrors the CLI.
daggerverse/cpybkc'sInittakes an optionalcopyMember []string.argv.Initpasses each value through as given.companionCoveragemaps--copy-membertoInit.checkInitnow also compares the curated and hand-typed runs overtestdata/composed, with all three members.dagger generate -yfrom an ordinary clone.Composed fixtures.
testdata/composed/has:COPY,COPY STDTRL OF GLLIB.Its copybooks are in story(layout): every copybook cpybkc can read converts to a definitions file that cobol-go reads back as the same entries #401's round trip, parsed through the layout's members. They are also in story(resolve): a record whose definitions file the layout names resolves to the descriptor its copybook gives, with every fault pointing into that file #405's and story(resolve): a record whose definitions are written in a block in the layout resolves to the descriptor its copybook gives, with every fault pointing into the layout #409's equality suites. On the file side, the definitions files are the ones
scaffold.Derive(whatinitwrites) produces with one--copy-memberper form, and thecopy-memberforms are removed once nothing copies. On the block side, the blocks hold those files' entries. Both sides resolve to byte-identicalemit.Marshaldescriptors.The mapping rows in docs/cli/SPEC.md and docs/layout/SPEC.md that name story(cli): decide where a
COPYmember is found, so a copybook composed withCOPYcan be read #407 already named story(cli): a copybook composed withCOPYresolves through the layout'scopy-memberforms, andinitconverts it as its expansion #423. The Definitions, surface-syntax, published-schema and validation rows now also record the pin and the notation it brought. The CLI SPEC's "needs member provenance … which it does not have" paragraph now says what the provenance is and that story(cli): a copybook composed withCOPYresolves through the layout'scopy-memberforms, andinitconverts it as its expansion #423 moved the pin to it.Judgment calls
init, the unnamed-member message readsCOPY X names no --copy-member on the command line/the command line names …. A missing member file is reported at theCOPYthat needed it, becauseinithas no layout span to point at, and names the path as typed.copymember(UnnamedMemberError,CycleError, parameterised bycopymember.Layout/copymember.CommandLine), so the run andinitproduce the same shape. The missing-file diagnostic differs because only the run has a form to point at.scaffold.Derivesignature. It is nowDerive(books, members ...copymember.Form), so existing callers are unchanged.Verification
go test ./...on the root module passes everywhere exceptinternal/manifest. Its three JSON-truncation tests fail on main as well, under the local go1.27 toolchain; CI's go1.26 passes them.daggerverse/cpybkcand.daggermodules build and vet.internal/argvtests pass.dagger call ciwas run from an ordinary clone at 041517e: thecitrace passed and the command exited 0. The one failedgenerator stalecheck is the deliberate stale-copy proof, matching main (checks: 2/2 done (1 failed)).🤖 Generated with Claude Code