diff --git a/cmd/declscope/load.go b/cmd/declscope/load.go index 26c83d2..87248a8 100644 --- a/cmd/declscope/load.go +++ b/cmd/declscope/load.go @@ -124,31 +124,6 @@ func loadIsAnalyzable(pkg *packages.Package) bool { return !loadIsTestMain(pkg) } -// loadIsTestMain identifies the synthetic executable built to run a package's -// tests. Depending on the go command's response, its file is either the -// generated _testmain.go or a build-cache archive; a real package is loaded -// from .go files even when its import path happens to end in ".test". -func loadIsTestMain(pkg *packages.Package) bool { - if pkg.Name != "main" || !strings.HasSuffix(pkg.PkgPath, ".test") { - return false - } - paths := append(slices.Clone(pkg.GoFiles), pkg.CompiledGoFiles...) - for _, path := range paths { - if filepath.Base(path) == "_testmain.go" { - return true - } - } - for _, path := range paths { - // A real package is loaded from Go source. With the go command's - // driver, the synthetic executable instead points at its build-cache - // archive (a path ending in -d), even when NeedSyntax is requested. - if filepath.Ext(path) == ".go" { - return false - } - } - return len(paths) > 0 -} - // loadIsExternalTest identifies package foo_test as loaded for package foo. // ForTest is the build system's answer; trimming an import-path suffix would // also discard an ordinary package that is genuinely named foo_test. @@ -260,3 +235,28 @@ func loadedPackageDir(pkg *packages.Package) string { // and every caller drops those first. return "" } + +// loadIsTestMain identifies the synthetic executable built to run a package's +// tests. Depending on the go command's response, its file is either the +// generated _testmain.go or a build-cache archive; a real package is loaded +// from .go files even when its import path happens to end in ".test". +func loadIsTestMain(pkg *packages.Package) bool { + if pkg.Name != "main" || !strings.HasSuffix(pkg.PkgPath, ".test") { + return false + } + paths := append(slices.Clone(pkg.GoFiles), pkg.CompiledGoFiles...) + for _, path := range paths { + if filepath.Base(path) == "_testmain.go" { + return true + } + } + for _, path := range paths { + // A real package is loaded from Go source. With the go command's + // driver, the synthetic executable instead points at its build-cache + // archive (a path ending in -d), even when NeedSyntax is requested. + if filepath.Ext(path) == ".go" { + return false + } + } + return len(paths) > 0 +} diff --git a/internal/collect.go b/internal/collect.go index c07cfdd..678189b 100644 --- a/internal/collect.go +++ b/internal/collect.go @@ -142,6 +142,51 @@ func (c *collection) collectTargets(pass *analysis.Pass, opts Options) { }) } +// collectRefs records every ident naming a tracked object, along with the file +// it appears in. +// +// An ident can play both roles at once: an embedded field's ident defines the +// field and uses the type, so Defs and Uses are consulted independently rather +// than one shadowing the other. Returning after a hit in Defs would drop the +// type use, and with it both a rename edit and a boundary diagnostic. +// +//declscope:shared // the pipeline's third stage, driven from analyzer.go +func (c *collection) collectRefs(pass *analysis.Pass) { + in := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + // The inspector holds pass.Files in order, and c.files is a subsequence of + // them, so the files are visited in the order c.files lists them. + for fc := range in.Root().Children() { + fi := c.byFile[fc.Node().(*ast.File)] + if fi == nil { + continue + } + for cur := range fc.Preorder((*ast.CompositeLit)(nil), (*ast.Ident)(nil)) { + if lit, ok := cur.Node().(*ast.CompositeLit); ok { + c.collectUnkeyedFields(pass, fi, lit) + continue + } + ident := cur.Node().(*ast.Ident) + // Only a target's idents and refs are ever read, and byObj is + // complete, so nothing else is recorded. Most idents in a package + // name locals, parameters and imported objects. + if obj := pass.TypesInfo.Defs[ident]; c.byObj[obj] != nil { + c.idents[obj] = append(c.idents[obj], ident) + } + obj := origin(pass.TypesInfo.Uses[ident]) + if obj == nil { + continue + } + if c.byObj[obj] != nil { + c.idents[obj] = append(c.idents[obj], ident) + c.refs[obj] = append(c.refs[obj], ref{node: ident, file: fi}) + } + if tn := embeddedTypeName(obj); c.byObj[tn] != nil { + c.idents[tn] = append(c.idents[tn], ident) + } + } + } +} + func (c *collection) addFuncToCollection(pass *analysis.Pass, opts Options, fi *fileInfo, d *ast.FuncDecl) { // Parsed before anything is skipped, so that a directive on a function // declscope does not check, the blank one or init, is reported unused @@ -428,48 +473,3 @@ func (c *collection) collectUnkeyedFields(pass *analysis.Pass, fi *fileInfo, lit } } } - -// collectRefs records every ident naming a tracked object, along with the file -// it appears in. -// -// An ident can play both roles at once: an embedded field's ident defines the -// field and uses the type, so Defs and Uses are consulted independently rather -// than one shadowing the other. Returning after a hit in Defs would drop the -// type use, and with it both a rename edit and a boundary diagnostic. -// -//declscope:shared // the pipeline's third stage, driven from analyzer.go -func (c *collection) collectRefs(pass *analysis.Pass) { - in := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - // The inspector holds pass.Files in order, and c.files is a subsequence of - // them, so the files are visited in the order c.files lists them. - for fc := range in.Root().Children() { - fi := c.byFile[fc.Node().(*ast.File)] - if fi == nil { - continue - } - for cur := range fc.Preorder((*ast.CompositeLit)(nil), (*ast.Ident)(nil)) { - if lit, ok := cur.Node().(*ast.CompositeLit); ok { - c.collectUnkeyedFields(pass, fi, lit) - continue - } - ident := cur.Node().(*ast.Ident) - // Only a target's idents and refs are ever read, and byObj is - // complete, so nothing else is recorded. Most idents in a package - // name locals, parameters and imported objects. - if obj := pass.TypesInfo.Defs[ident]; c.byObj[obj] != nil { - c.idents[obj] = append(c.idents[obj], ident) - } - obj := origin(pass.TypesInfo.Uses[ident]) - if obj == nil { - continue - } - if c.byObj[obj] != nil { - c.idents[obj] = append(c.idents[obj], ident) - c.refs[obj] = append(c.refs[obj], ref{node: ident, file: fi}) - } - if tn := embeddedTypeName(obj); c.byObj[tn] != nil { - c.idents[tn] = append(c.idents[tn], ident) - } - } - } -} diff --git a/internal/config/config.go b/internal/config/config.go index 0a723c5..0b1dbf9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -83,6 +83,25 @@ func (b *boolSetting) UnmarshalYAML(node *yaml.Node) error { return nil } +// File is the on-disk configuration. Every field is optional, and no setting +// has a zero value that means anything, so a field left empty is skipped by +// Apply: omitting a key keeps the built-in default rather than silently +// disabling a rule. +type File struct { + Defaults defaultsSection `yaml:"defaults"` + Rules rulesSection `yaml:"rules"` + + // Filter narrows and subtracts. only alone keeps nothing outside it, omit + // alone takes files out of everything, and both together narrow first. + Filter filterSection `yaml:"filter"` + + // Baseline is a path relative to this config file. + Baseline string `yaml:"baseline"` + + // path is where this config was read from, used to resolve Baseline. + path string +} + // Each section is a named type so that go-yaml's strict-decoding error can name // the section a misspelled key sits in, rather than printing the anonymous // struct's whole type literal. @@ -128,25 +147,6 @@ type namingSection struct { Vocabulary map[string][]string `yaml:"vocabulary"` } -// File is the on-disk configuration. Every field is optional, and no setting -// has a zero value that means anything, so a field left empty is skipped by -// Apply: omitting a key keeps the built-in default rather than silently -// disabling a rule. -type File struct { - Defaults defaultsSection `yaml:"defaults"` - Rules rulesSection `yaml:"rules"` - - // Filter narrows and subtracts. only alone keeps nothing outside it, omit - // alone takes files out of everything, and both together narrow first. - Filter filterSection `yaml:"filter"` - - // Baseline is a path relative to this config file. - Baseline string `yaml:"baseline"` - - // path is where this config was read from, used to resolve Baseline. - path string -} - // Resolve produces the options for analyzing a package directory: built-in // defaults, with the nearest config file layered on top, and the baseline // loaded — the one the config names, else the nearest default-named file @@ -280,6 +280,33 @@ func DefaultBaseline(dir, root string) (string, bool) { return "", false } +// Load reads and parses a config file. +// +//declscope:ignore overexported // config_test parses files through it +func Load(path string) (*File, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, err + } + // The path is kept absolute because the exclude patterns and the baseline + // are resolved against the directory holding it. A -config given relative + // to the working directory would otherwise name a directory that no + // reported file path is ever taken from. + if abs, err := filepath.Abs(path); err == nil { + path = abs + } + f := File{path: path} + dec := yaml.NewDecoder(bytes.NewReader(data)) + // Unknown keys are an error rather than a silent no-op: a typo in a rule + // name would otherwise leave the rule at its default with no sign of it. + dec.KnownFields(true) + // An empty document is a valid config that changes nothing. + if err := dec.Decode(&f); err != nil && !errors.Is(err, io.EOF) { + return nil, fmt.Errorf("%s: %w", path, namedKeys(err)) + } + return &f, nil +} + func findUp(dir string, names []string) string { for d := range dirsUp(dir) { if path := firstFile(d, names); path != "" { @@ -335,33 +362,6 @@ func sameDir(a, b string) bool { return errA == nil && errB == nil && os.SameFile(sa, sb) } -// Load reads and parses a config file. -// -//declscope:ignore overexported // config_test parses files through it -func Load(path string) (*File, error) { - data, err := os.ReadFile(path) - if err != nil { - return nil, err - } - // The path is kept absolute because the exclude patterns and the baseline - // are resolved against the directory holding it. A -config given relative - // to the working directory would otherwise name a directory that no - // reported file path is ever taken from. - if abs, err := filepath.Abs(path); err == nil { - path = abs - } - f := File{path: path} - dec := yaml.NewDecoder(bytes.NewReader(data)) - // Unknown keys are an error rather than a silent no-op: a typo in a rule - // name would otherwise leave the rule at its default with no sign of it. - dec.KnownFields(true) - // An empty document is a valid config that changes nothing. - if err := dec.Decode(&f); err != nil && !errors.Is(err, io.EOF) { - return nil, fmt.Errorf("%s: %w", path, namedKeys(err)) - } - return &f, nil -} - // The sections a key can sit in, by the type name go-yaml puts in its // strict-decoding complaint. var sections = map[string]struct { diff --git a/internal/ignore.go b/internal/ignore.go index dce9fcf..6b98d85 100644 --- a/internal/ignore.go +++ b/internal/ignore.go @@ -32,7 +32,6 @@ type ignoreBook struct { // per target, it would be reported unused whenever any sibling did not need // it, and a wholly unused one would be reported once per sibling. type ignoreSite struct { - ig directive.Ignore // decls names the declarations the directive reaches, in source order, // for the report. It is empty for a file-level directive, and for one // carried only by declarations declscope does not check (init, _, an @@ -42,7 +41,6 @@ type ignoreSite struct { decls []string //declscope:shared // collect.go marks it when the directive is the file's fileLevel bool - used bool // targets are the declarations the directive reaches, decls' objects. A // file the build excluded may cross into one of them by name. // @@ -53,6 +51,9 @@ type ignoreSite struct { // //declscope:shared // collect.go records them as it parses the group siblings []directive.Ignore + + ig directive.Ignore + used bool } // siteOfIgnore returns the accounting entry for ig, keyed by where it is written. @@ -117,30 +118,6 @@ func (c *collection) ignoreWouldSilence(t *target, r rule.Rule) bool { return false } -// ignoreSilencesFile reports whether the file stands the problem's rule down -// for everything it holds, and marks the ignore that did it used. -// -// The marking is what keeps an ignore written for a directive problem from -// being reported as unused itself: it silences a report that is not attached to -// any declaration, so the per-target accounting never sees it work. -// -// An ignore never silences the report written at its own position, which is -// the report that it is unused. One that could would never be called unused, -// and that report exists to catch it. -func (c *collection) ignoreSilencesFile(fi *fileInfo, p directive.Problem) bool { - if fi == nil { - return false - } - hit := false - for _, ig := range fi.ignores { - if ig.Pos != p.Pos && ig.Covers(p.Rule) { - c.siteOfIgnore(ig).used = true - hit = true - } - } - return hit -} - // ignored reports whether any directive silences r, marking every directive // that does as used. All of them are marked, not just the first, so that // overlapping directives are not reported as unused. @@ -318,3 +295,27 @@ func (c *collection) problemsLeftByIgnores(pass *analysis.Pass) []directive.Prob return c.ignoreSilencesFile(c.fileAt(pass, p.Pos), p) }) } + +// ignoreSilencesFile reports whether the file stands the problem's rule down +// for everything it holds, and marks the ignore that did it used. +// +// The marking is what keeps an ignore written for a directive problem from +// being reported as unused itself: it silences a report that is not attached to +// any declaration, so the per-target accounting never sees it work. +// +// An ignore never silences the report written at its own position, which is +// the report that it is unused. One that could would never be called unused, +// and that report exists to catch it. +func (c *collection) ignoreSilencesFile(fi *fileInfo, p directive.Problem) bool { + if fi == nil { + return false + } + hit := false + for _, ig := range fi.ignores { + if ig.Pos != p.Pos && ig.Covers(p.Rule) { + c.siteOfIgnore(ig).used = true + hit = true + } + } + return hit +} diff --git a/internal/namespace/namespace.go b/internal/namespace/namespace.go index d20374c..f3e9a31 100644 --- a/internal/namespace/namespace.go +++ b/internal/namespace/namespace.go @@ -34,37 +34,6 @@ import ( "github.com/mpyw/declscope/internal/buildtag" ) -// of returns the default namespace for a file path. -// -// The path is reduced to its base name and then normalized: -// -// user_repository.go -> userRepository -// user_repository_test.go -> userRepository (tests share their subject's namespace) -// parser_linux.go -> parser (GOOS/GOARCH suffixes are build -// parser_linux_amd64.go -> parser constraints, not namespaces) -// user_id.go -> userID (initialisms are spelled the way Go does) -// foo-bar.go, foo.bar.go -> fooBar (any separator, not only _) -// Foo.go, HTTPServer.go -> foo, httpServer (a PascalCase stem is lowered) -// v2_client.go -> v2Client -// 2fa_auth.go -> 2faAuth -// -// of always returns the normalized stem when there is one, so that a file's -// identity never depends on whether the stem also makes a usable prefix: -// 2fa_test.go must share the namespace of 2fa.go even though no identifier can -// start with a digit. Whether the result can serve as a prefix is a separate -// question, answered by CanPrefix. Only a file with no stem at all yields "". -func of(path string) string { - base := strings.TrimSuffix(filepath.Base(path), ".go") - - // _test must be stripped before the build suffixes: the canonical shape is - // name_GOOS_GOARCH_test.go. - base = trimSegment(base, func(s string) bool { return s == "test" }) - base = trimSegment(base, buildtag.KnownArch) - base = trimSegment(base, buildtag.KnownOS) - - return camel(base) -} - // CanPrefix reports whether ns can be written as a prefix: prepended to an // unexported identifier, it has to yield another unexported identifier. // @@ -150,6 +119,37 @@ func Unexported(name string) (string, bool) { } } +// of returns the default namespace for a file path. +// +// The path is reduced to its base name and then normalized: +// +// user_repository.go -> userRepository +// user_repository_test.go -> userRepository (tests share their subject's namespace) +// parser_linux.go -> parser (GOOS/GOARCH suffixes are build +// parser_linux_amd64.go -> parser constraints, not namespaces) +// user_id.go -> userID (initialisms are spelled the way Go does) +// foo-bar.go, foo.bar.go -> fooBar (any separator, not only _) +// Foo.go, HTTPServer.go -> foo, httpServer (a PascalCase stem is lowered) +// v2_client.go -> v2Client +// 2fa_auth.go -> 2faAuth +// +// of always returns the normalized stem when there is one, so that a file's +// identity never depends on whether the stem also makes a usable prefix: +// 2fa_test.go must share the namespace of 2fa.go even though no identifier can +// start with a digit. Whether the result can serve as a prefix is a separate +// question, answered by CanPrefix. Only a file with no stem at all yields "". +func of(path string) string { + base := strings.TrimSuffix(filepath.Base(path), ".go") + + // _test must be stripped before the build suffixes: the canonical shape is + // name_GOOS_GOARCH_test.go. + base = trimSegment(base, func(s string) bool { return s == "test" }) + base = trimSegment(base, buildtag.KnownArch) + base = trimSegment(base, buildtag.KnownOS) + + return camel(base) +} + // trimSegment drops the final underscore-separated segment when match accepts // it. The segment is only dropped when something would remain, mirroring // go/build: linux.go is an ordinary file, foo_linux.go is constrained. @@ -315,34 +315,6 @@ func containsForm(name, form string) bool { return false } -// inflections returns the spellings of ns that English writes with a changed -// stem, which the free right edge of Contains cannot reach: -// -// store → storing (the final e drops before -ing) -// apply → applies, applied (the final y turns to i) -// -// Each is a complete form, never a bare stem: matching stor with a free right -// edge would accept story and storm, and matching appli would accept appliance. -// In a compound namespace only the final word inflects (azureAppconfigParam), -// and the final word is where the tail letters sit, so inspecting the last two -// runes is enough. A final e or y after a vowel does not change spelling -// (freeing, deploys) and generates nothing. -func inflections(ns string) []string { - last, lastSize := utf8.DecodeLastRuneInString(ns) - prev, _ := utf8.DecodeLastRuneInString(ns[:len(ns)-lastSize]) - if prev == utf8.RuneError || !unicode.IsLetter(prev) || isVowel(prev) { - return nil - } - stem := ns[:len(ns)-lastSize] - switch unicode.ToLower(last) { - case 'e': - return []string{stem + "ing"} - case 'y': - return []string{stem + "ies", stem + "ied"} - } - return nil -} - // LooksInflected reports whether ns looks like a form that inflections // generates from another namespace: tracing from trace, applies from apply. // The naming rule uses it only to point at rules.naming.vocabulary in its @@ -375,6 +347,34 @@ func LooksInflected(ns string) bool { return (generates(stem+"e") || generates(stem+"y")) && strings.ContainsFunc(stem, isVowel) } +// inflections returns the spellings of ns that English writes with a changed +// stem, which the free right edge of Contains cannot reach: +// +// store → storing (the final e drops before -ing) +// apply → applies, applied (the final y turns to i) +// +// Each is a complete form, never a bare stem: matching stor with a free right +// edge would accept story and storm, and matching appli would accept appliance. +// In a compound namespace only the final word inflects (azureAppconfigParam), +// and the final word is where the tail letters sit, so inspecting the last two +// runes is enough. A final e or y after a vowel does not change spelling +// (freeing, deploys) and generates nothing. +func inflections(ns string) []string { + last, lastSize := utf8.DecodeLastRuneInString(ns) + prev, _ := utf8.DecodeLastRuneInString(ns[:len(ns)-lastSize]) + if prev == utf8.RuneError || !unicode.IsLetter(prev) || isVowel(prev) { + return nil + } + stem := ns[:len(ns)-lastSize] + switch unicode.ToLower(last) { + case 'e': + return []string{stem + "ing"} + case 'y': + return []string{stem + "ies", stem + "ied"} + } + return nil +} + // isVowel reports an English vowel letter, case-folded. y is deliberately not // one here: apply's y is what inflects, so for the letter before the tail the // question never arises in a real namespace. diff --git a/internal/rename.go b/internal/rename.go index 9c9ab64..9e8b4b3 100644 --- a/internal/rename.go +++ b/internal/rename.go @@ -42,13 +42,6 @@ type renameState struct { outsideDone bool } -func (c *collection) renames() *renameState { - if c.rename == nil { - c.rename = &renameState{reserved: make(map[string]bool)} - } - return c.rename -} - // renameFix rewrites every ident naming the target. All of them are inside the // package, so the edits stay within the pass. // @@ -162,6 +155,13 @@ func (c *collection) reserveForRename(name string) { c.renames().reserved[name] = true } +func (c *collection) renames() *renameState { + if c.rename == nil { + c.rename = &renameState{reserved: make(map[string]bool)} + } + return c.rename +} + // renameBoundByImport reports whether any file scope of the package binds name, // which is where imports live. func renameBoundByImport(pkg *types.Scope, name string) bool { diff --git a/internal/scopesite.go b/internal/scopesite.go index 3116688..f8dc20e 100644 --- a/internal/scopesite.go +++ b/internal/scopesite.go @@ -50,7 +50,6 @@ type scopesiteBook struct { // has, so it is provably inert; //declscope:private narrows it, so it is // not. type scopeSite struct { - dir directive.Decl // decls names the declarations in its reach, in source order, for the // report. Empty for a file-level directive. // @@ -58,7 +57,9 @@ type scopeSite struct { decls []string //declscope:shared // collect.go marks it when the directive is the file's fileLevel bool - bound bool + + dir directive.Decl + bound bool // shadowed records that something in its reach took a nearer directive's // scope instead. A block's directive that every spec overrides reaches no diff --git a/internal/shrink/module/module.go b/internal/shrink/module/module.go index 913b2f2..dcf00ca 100644 --- a/internal/shrink/module/module.go +++ b/internal/shrink/module/module.go @@ -31,9 +31,7 @@ import ( // Module is every package of the main modules, loaded with its tests, and // what the build left out of them. type Module struct { - // mains are the main modules, in the order the go command lists them. - mains []mainModule - Fset *token.FileSet + Fset *token.FileSet // Pkgs is every variant go/packages returned: a package, the package // with its in-package tests, and its external test package, each // separately. Paths lists their import paths in order. @@ -47,6 +45,8 @@ type Module struct { // Their packages are never judged. Importers []Importer + // mains are the main modules, in the order the go command lists them. + mains []mainModule byPath map[string][]*packages.Package // importedBy maps each loaded package's import path to the paths of the // loaded packages importing it, and reaching caches, per package, every @@ -60,14 +60,6 @@ type Module struct { nested []nestedModule } -// nestedModule is one module below a main module's root. -type nestedModule struct { - path, dir string - // why says what keeps this run from reading every use the module makes, - // or is empty once it has read them. - why string -} - // Importer is one nested module this run loaded to read what it uses. // // It is loaded on its own, from its own directory, since its go.mod decides @@ -84,6 +76,14 @@ type Importer struct { Imported []*packages.Package } +// nestedModule is one module below a main module's root. +type nestedModule struct { + path, dir string + // why says what keeps this run from reading every use the module makes, + // or is empty once it has read them. + why string +} + // mainModule is one main module: its import path and root directory. type mainModule struct { path, dir string diff --git a/internal/subject.go b/internal/subject.go index aede7ea..4ead9ac 100644 --- a/internal/subject.go +++ b/internal/subject.go @@ -289,6 +289,10 @@ type collection struct { // declare, which is how many boundaries there are to enforce. namespaces int + // aliasNames maps a type of the package to the names of its aliases in + // the package scope. It is filled on first use. + aliasNames map[types.Object][]string + // unseenScan is what the package directory holds that this pass does not // see: in-package _test.go files under the non-test variant, and files the // build configuration excluded. The rename fix and the unused-ignore @@ -296,9 +300,6 @@ type collection struct { // //declscope:private unseenScan *unseenFiles - // aliasNames maps a type of the package to the names of its aliases in - // the package scope. It is filled on first use. - aliasNames map[types.Object][]string // linknamedScan is every local name a //go:linkname or //export directive // in the package binds. The surplus evidence and the rename guard both diff --git a/skills.go b/skills.go index b025a8e..53c2e03 100644 --- a/skills.go +++ b/skills.go @@ -6,12 +6,6 @@ import ( skillembed "github.com/mpyw/go-skill-embed" ) -// The skills hold no file whose name begins with a dot or an underscore, so -// the bare form is enough. all:skills is what to write when one does. -// -//go:embed skills -var skillsFS embed.FS - // Skills are the skills this module carries: declscope-authoring, for writing // code under declscope, and declscope-adoption, for introducing it. // @@ -20,3 +14,9 @@ var skillsFS embed.FS // root. It sits there because that is where `gh skill install` looks, which // is the other way to reach it. var Skills = skillembed.MustSkillsFromFS(skillsFS, "skills") + +// The skills hold no file whose name begins with a dot or an underscore, so +// the bare form is enough. all:skills is what to write when one does. +// +//go:embed skills +var skillsFS embed.FS diff --git a/skills/declscope-authoring/SKILL.md b/skills/declscope-authoring/SKILL.md index 9d7f406..3dedf68 100644 --- a/skills/declscope-authoring/SKILL.md +++ b/skills/declscope-authoring/SKILL.md @@ -183,7 +183,7 @@ When another namespace uses a struct, give each field the smallest scope it need **Do not restate the type's scope on a field.** It binds nothing and is reported as unused. -**Put the private fields last.** The fields other files read are the struct's interface, so they come first: +**Put the private fields last,** as [Order by reach](#order-by-reach) says. The fields other files read are the struct's interface, so they come first: ```go // callee is a resolved call target. @@ -201,15 +201,6 @@ type callee struct { } ``` -Do not reorder fields where the order is observable. Add the directives in place instead: - -| Order is observable through | Effect | -| --- | --- | -| Unkeyed composite literals | `callee{f, in, fn}` binds by position | -| Positional or binary encodings | The wire format follows the field order | -| `unsafe` offsets | `unsafe.Offsetof` changes | -| 64-bit atomics | They rely on first-word alignment on 32-bit platforms | - Under `surplus: strict`, declscope reports each field no other namespace reads. Where `surplus` is off or stands down, find them by hand. Mark every field `//declscope:private`. Then delete the directive from each field reported as used from another namespace. **A type nobody else spells needs no directive.** If callers only get it from a constructor, `//declscope:shared` on it is reported: @@ -220,6 +211,60 @@ Under `surplus: strict`, declscope reports each field no other namespace reads. Keep that type and its fields private. Expose small shared functions that return what callers need. +## Order by reach + +**Order declarations from the widest reach to the narrowest.** A reader meets the file's interface first: what other packages use, then what other files use, then how it works. declscope does not check this order. It is a convention for code you write. + +| Reach | Declarations | +| --- | --- | +| 1. Other packages | Exported, and not `private` | +| 2. Other files of the package | Unexported, and `shared` by any directive in force: its own, its block's, its type's or the file's | +| 3. Its own namespace | `private`, exported or not | + +Scope decides the reach, not the case of the first letter. An exported declaration marked `//declscope:private` goes with the private ones, since its author said no other file uses it. + +**Order only inside a sequence the file already has.** Keep the order of kinds, and keep each type with its constructors and methods. Within each reach, keep the existing order, so helpers that sit together stay together. + +| Sequence | Order by reach | +| --- | --- | +| The fields of a struct, the methods of an interface | Yes | +| A run of adjacent declarations of one kind, such as consecutive functions or one `const` block | Yes | +| The constructors of a type, and separately its methods | Yes. The type still comes first, then its constructors, then its methods | +| Declarations of different kinds | No. Do not move a declaration past one of another kind | + +**A private helper may stay right after its users.** When the declarations that use it sit next to each other, put it directly after the last of them, or after another helper already placed there. Reading from the top then follows the calls. A helper whose users are spread through the file goes in reach order. + +```go +func Resolve(dir, explicit string) (Options, error) { ... } + +func ResolveForBaseline(dir, explicit string) (Options, string, error) { ... } + +// resolve is used only by the two above, so it stays after them. +func resolve(dir, explicit string) (Options, string, error) { ... } + +func Find(dir string) string { ... } +``` + +Fields have no such exception. A private field goes after the shared ones, even the field the struct is built around. + +Do not reorder where the order is observable: + +| Order is observable through | Effect | +| --- | --- | +| `iota`, or a `const` spec with its expression omitted | Each value follows its position | +| Unkeyed composite literals | `callee{f, in, fn}` binds fields by position | +| Positional or binary encodings | The wire format follows the field order | +| `unsafe` offsets | `unsafe.Offsetof` changes | +| 64-bit atomics | They rely on first-word alignment on 32-bit platforms | +| Package-level `var` initializers with side effects and no dependencies between them | They run in declaration order | +| Several `init` functions in one file | They run in source order | + +Where the repository's linters fix an order, such as `decorder` or `funcorder`, follow them first. `funcorder`'s exported-before-unexported methods is the same direction, so order by reach inside it. + +Test files and generated files are out of scope. `TestXxx` functions are exported but called by nobody. + +**When your change moves a declaration to another reach, move the declaration too.** That is the default, for a new `//declscope:shared`, a `//declscope:private` from `surplus`, or a name `shrink` unexported. Say in your report that you moved it: the move makes the diff larger, and the owner may prefer it in place. Do not reorder declarations your change does not touch. + ## Acting on a diagnostic your change caused **A boundary report points at the declaration you reached, not at your code.** Your use is listed under it as `used here, in namespace ...`. A directive or an ignore that answers it goes on the declaration. @@ -228,7 +273,7 @@ Keep that type and its fields private. Expose small shared functions that return | --- | --- | --- | | `X is private to namespace "a"` (or `to the core namespace`), `but is used from namespace "b"`, or `X is declared private by ...` | Should your code live beside `X`? | Move your code beside `X`. If `X` is shared on purpose and sits in one caller's file, move it to a file named for its concept first. Then write `//declscope:shared // why` on `X`. When `X` states its own `//declscope:private`, that was a decision: move your code, and do not flip the directive | | `//declscope:shared on X: no use from another namespace is visible` | Did the use that justified it go away? | Delete the directive | -| `X takes shared scope from //declscope:shared on Y, but no use ...` | Does any other file read `X`? | Accept `-fix`'s `//declscope:private` on `X`. For a field, move it to the end of the struct | +| `X takes shared scope from //declscope:shared on Y, but no use ...` | Does any other file read `X`? | Accept `-fix`'s `//declscope:private` on `X`. Move `X` after the declarations of wider reach. See [Order by reach](#order-by-reach) | | `unused //declscope:...` | Does the directive still change anything? | Delete it | | `X does not carry namespace "a"` | Is the name accurate, and is `X` in the right file? | See [Naming](#naming). Rename only when both answers are yes | | `X is exported, but nothing outside ... uses it` (`declscope shrink`) | Does another package really need it? | See [Reading what shrink reports](#reading-what-shrink-reports) |