feat(irverify): hold Naming.Aliases to a rule - #361
Open
OmarAlJarrah wants to merge 1 commit into
Open
Conversation
Naming has four channels and irverify read three. Aliases was read by nothing anywhere in the pipeline -- it appeared in production code exactly once, at its own declaration -- so a model whose aliases were empty and duplicated verified clean and validated clean. Which rules apply had to be settled first, and the answer is not Canonical's. An alias is matched against a name some other schema wrote: an Avro alias is a full name such as com.example.User, and neutralizing it to words discards the separators and the casing the match is made of. That makes it a verbatim channel like Source rather than a neutral one like Canonical, and holding it to the neutrality rules would be the lossy direction invariant 2 forbids. The IR is not deciding this spelling, it is recording one. What is left is decidable without a grammar. An empty alias matches nothing and a repeated one matches twice, so neither can be what a producer intended: both say a list was built wrong rather than that a name was spelled wrong. checkNaming reports each as its own violation, at a path naming the offending entry, and the field's doc comment now states what a well-formed entry looks like instead of only what the field is for.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Naminghas four channels andirverifyread three.Source,CanonicalandHintare read bycheckNaming;Aliaseswas read by nothing anywhere in thepipeline — it appeared in production code exactly once, at its own declaration:
So a model whose aliases were cased, punctuated, empty and duplicated verified
clean and validated clean.
Which rules an alias is held to
The issue flagged this as the thing to settle before writing any check, and
warned against applying
Canonical's rules unexamined. The answer is that theydo not apply.
An alias is matched against a name some other schema wrote. An Avro alias is
a full name such as
com.example.User, and resolution compares it verbatimagainst the writer schema's full name. Neutralizing it to a word sequence
discards the separators and the casing the match is made of —
com.example.Userand
com_example_userare not the same key, and only one of them matches. So analias is a verbatim channel like
Source, not a neutral one likeCanonical.Sourceis the internal precedent: it carriesUserIDtoday and no content ruletouches it, because it records what the spec said rather than deciding a
spelling. Invariant #4 governs names the IR decides; invariant #2 forbids the
lossy normalization holding an alias to
Canonical's grammar would require.What is left is decidable without any grammar, and is what this check makes:
ir/naming-alias-empty— an empty alias matches nothing.ir/naming-alias-duplicate— a repeated alias matches twice.Neither can be what a producer intended; both say a list was built wrong rather
than that a name was spelled wrong. Each violation's path names the offending
entry (
doc.Types[t/x/M].Name.Aliases[2]), spelled the way the walk would havereached it, so a violation on a list of several says which one.
ir/naming.go's field comment now states what a well-formed entry looks like.Before this it documented what the field is for and nothing about what may go
in it, which is why the question had to be re-derived to answer the issue.
Test plan
Seven tests in
ir/irverify, including the issue's exact reproducer. Each rulewas confirmed to redden against a planted defect:
appendAliasViolationscall with_ = aliasesfails the emptyand duplicate tests with
"[]" should have 1 item(s), but has 0.should have 1 item(s), but has 3, namingAliases[0],[1]and[2].Canonical's grammar to aliases failsTestVerify_VerbatimAliasesAreCleanwith two violations oncom.example.UserandUserID— so the settlement above is pinned by a test,and a later attempt to tighten it has to argue with that test rather than slip
through.
TestVerify_RepeatedEmptyAliasReportsEachAsEmptyholds the interaction betweenthe two rules: a second empty entry is a repeat as well, and reporting it as a
duplicate would name the wrong repair.
Coverage note, stated rather than implied: no compiler in the tree writes an
alias yet, so no committed fixture reaches this check. That is the expected
state — the rule exists so the first compiler that does write one (Avro,
Protobuf
json_namealternates) is held from its first commit — and the unitfixtures above are what exercise it.
Full gate green:
gofmt,go vet ./...,golangci-lint run(0 issues),go build ./...,./scripts/check-coverage.sh(all 4958 statements covered).Closes #317