Repository navigation
Add References, every reference between packs that pks lists - #61
Merged
Merged
Conversation
References will show their sources the way violations do, so the methods that do it move out of Violation and Violations into two abstract classes they now subclass: ConstantUse, for one pack's use of a constant another pack defines, and ConstantUses, for a collection of them. A ConstantUse leaves only one thing to its subclass, how to find its sources in a file. For a Violation that's still wherever the file names its constant, since package_todo.yml records no positions. Nothing changes for callers. Violation#anonymous_sources anonymizes each source itself, rather than through FileCache#get_full_anonymous_sources, which had no other caller and is gone. Violation#count counts the sources the other methods show, which are the constants it counted before. including_files and excluding_files return the class they're called on. sorbet/rbi/todo.rbi stubs ApplicationRecord in ConstantUse, where runtime_keys now names it. Regenerating the file with tapioca would also turn ActionView::ActionViewError back into a module, which Sorbet rejects, so only that line changes.
where checks its keys against the first value in the collection, and with no values it raised NoMethodError on nil, so in an app whose package_todo.yml files record no violations, Violations.where and violations_for raised instead of finding nothing. It now finds nothing, without checking the keys, since nothing could match them.
package_todo.yml files record only the references that break a pack's rules, so query_packwerk couldn't show how other packs use a pack where a declared dependency or a public constant allows it (#8). pks 0.7.0 lists every reference from one pack to another pack's constants, with the violations each makes. References runs `pks list-references` and has the same methods as Violations: References.where(producing_pack: "packs/foo", allowed: true).anonymous_source_counts QueryPackwerk.references_for and references_from, and Package's incoming_references and outgoing_references, get the references to or from a pack, as violations_for and todos_for do for violations. where can query a reference's constant, file, packs and owners, as for a violation, and also allowed and violation_types. reload! forgets the references too, so the next query runs pks again. query_packwerk runs bin/pks if the app has one, as an app that pins pks's version does, or else pks on the PATH. It checks that the version has list-references, and when pks is missing or too old, the error says where to get it. Violations still come from package_todo.yml files and don't need pks. pks says where each reference is, so a reference finds its source at that line and column rather than by its constant's name. That finds a constant that the code names relatively, as `Bar` inside `module Foo`, which matching names misses, and makes the association call the source of a reference that pks infers from one, as `has_many :tacos` is for `::Taco`. pks counts columns in bytes, where the parser counts characters, so the file cache converts them. pks gives no positions in ERB, and lists a template's references to a constant once, so there the sources are wherever the template names the constant, as for violations. spec/support/referenced_packs.rb writes an app with each kind of reference, along with what pks 0.7.0 lists for it, which the specs use in place of pks. spec/query_packwerk/pks_spec.rb checks that listing against pks when pks is installed, and skips the check when it isn't.
A source is a constant with the calls chained on it, and finding those
calls climbed through any parent whose receiver equaled the node. That
compared nodes by structure, and blocks and singleton method
definitions have receivers too, so in `Foo.bar(Foo)` the second `Foo`'s
source was the whole call, in `Foo.each { Foo }` the inner one's was
the whole block, and in `def Foo.patched` it was the whole method. Now
only a call on the node itself extends it. Violations share the code,
so they counted such a call twice, once for each `Foo`, and now don't.
Finding a constant by name also matched the `Foo` in `Foo::Bar`, which
neither packwerk nor pks counts as a use of `::Foo`, since both resolve
`Foo::Bar` by its whole name. So a template that names `Foo::Bar` gave
a reference to `::Foo` an extra source, as any file did a violation of
`::Foo`.
A file that FileCache can't read or parse stood in as `x`, which is a
call, so a reference at line 1, column 0 of such a file got `x` as its
source. It stands in as `nil` now, which has no constant or call.
A source is the code of its node, and a heredoc's body isn't in that
code, since it's on the lines after the call, as in
Foo.execute(1, <<~SQL)
SELECT 1
SQL
So `Foo.execute(1, <<~SQL)` doesn't parse, and anonymizing it raised
NoMethodError on the nil AST, which failed anonymous_sources,
anonymous_source_counts, anonymous_sources_with_locations and
sources_with_contexts for every violation or reference in a
collection. That was so before References, but References include
every use of a pack's constants, so one with a heredoc is likelier.
Such a source now keeps its arguments as they are.
ParsePackwerk keeps the packs it reads, with the violations in their package_todo.yml files, and reload! didn't make it forget them. So after a change to a package.yml or package_todo.yml file, reload! still gave the violations from before, though the README says it picks up such changes. With References that got worse: pks lists the packs as they are now, so a pack added or renamed since the packs were read made every query of references raise Pks::Error until the console restarted. reload! busts ParsePackwerk's cache now too. pks's version was checked once for each path, so after checking out a branch whose bin/pks is older, the next query failed on `list-references` itself rather than saying to upgrade. The version is checked each time pks runs now, which is once until the next reload!.
A DotSlash file saved from a pks release, which is how an app pins pks's version in bin/pks, isn't executable until it's made so, and query_packwerk then ran pks on the PATH instead, or said there was no bin/pks. It says to `chmod +x bin/pks` now. The version check read pks's stdout and stderr together and expected the version at the start, so a wrapper that warns first failed it. It reads only stdout now. pks prints UTF-8, but Open3 gives what it prints in the locale's encoding, so with LANG=C, a file or constant with a character outside ASCII made JSON.parse raise Encoding::InvalidByteSequenceError. The output is read as UTF-8 now. Its strings are also frozen, so each one that repeats, as a file's name does for each of its references, is kept once. pks follows a symlink to a pack, but ParsePackwerk's default `**/` glob doesn't, so a symlinked pack made every query of references raise Pks::Error. A pack that ParsePackwerk doesn't find is read from its package.yml now, and only a pack without one raises. The fake pks in the specs prints its JSON with printf, since echo in dash, Ubuntu's /bin/sh, reads the backslashes in it as escapes.
A query, as with where, including_files or excluding_files, built its result with a new FileCache, and pointed the violations or references in it, which all keeps too, at that cache. So asking again, or asking about some of the same code, parsed every file again, and the earlier results' caches stayed in memory. References made that worse, since a pack's references are in more files than its violations are, and each reference is in two packs' results. A result shares all's cache now, so a file is parsed once until reload!, and setting a use's cache to the one it has leaves it loaded. The progress line counted a file once for each use in it, and printed a dot for each, though the file is parsed once. It counts, and dots, each file once now. Finding the node at pks's line and column converted the column of every node on the line from characters to bytes, which on a line with thousands of references, as in a generated list of constants, took minutes. pks's column is converted to characters once instead.
full_name puts packs/ before any name that doesn't start with packs/ or components/, so violations_for, todos_for, references_for, references_from and the methods that use them found nothing for the root pack, ".", or for a pack elsewhere, as engines/wax. The root pack is often where references come from, as for code in app/. A name that ParsePackwerk finds is kept as it is now.
help_violations and help_references, which copied it, said that sources_with_contexts and anonymous_sources_with_locations return arrays of [file:line, source] pairs, but they return hashes of each constant's anonymized sources, and showed consumers and producers taking a threshold that is a keyword. help_reference lists column, source_counts and anonymous_sources_with_locations too now, and help_references lists raw_sources, as for violations. The README says that reload! is for changes to code too, since a reference's position is where pks found it, and that the parsed files stay in memory until then. A file that doesn't parse is said to have no sources, rather than no violations, since references have them too, and the comments on Reference and ConstantUses say what they are more exactly.
A reference finds its sources where pks says it is, but only sources, sources_with_locations and anonymous_sources were checked that way, and on references whose files name their constants once. So count, source_counts, anonymous_sources_with_locations and sources_with_contexts could have found sources by name and passed. They're checked on apiary_report.rb:2 now, whose file names its constant again on line 4, as are its two violations, one that a package_todo.yml records and one that it doesn't. Every reference in the specs' app was to packs/honey_production, so references_for and incoming_references were checked against the count of all references, and producers and owned: false weren't checked. The app has a reference to a pack without an owner now, as pks 0.7.0 lists it. The specs also check a constant with no call on it, as an argument or a superclass, a strict pack, and excluding_files.
count was an alias of size, so it ignored a block or an item, as in
`References.all.count(&:allowed?)`, and gave how many references
there are in all. It counts what they match now, as Enumerable does.
where gave its block the place of its keys, so
`References.where(allowed: true) { |reference| ... }` found what the
block matched, allowed or not. The block narrows what the keys find
now.
In a Rails app, is_active_record was `constant < ApplicationRecord` for a class, and Class#< gives nil, not false, for a class that isn't related to ApplicationRecord. So `where(is_active_record: false)` found modules and constants that aren't loaded, but no classes. It's false for such a class now.
sources_with_locations gives [file:line, source] pairs, but its sig said an array of strings, or of strings and arrays of nodes, and violation_sources_for's said the same. So Sorbet let a caller treat each pair as a string, which raised at runtime, since sorbet-runtime doesn't check what's in an array. anonymous_sources_with_locations, sources_with_contexts and raw_sources returned T.untyped. Each says what it returns now.
where asks each violation or reference in a collection for the keys it queries, and deconstruct_keys built all of them before slicing out those, so a query of one key cost what a query of all of them did. In a Rails app, that included looking up every reference's constant, for is_active_record and is_constant, though no query asked about them, which can load the constant. Violation and Reference each keep a table of how to read a key now, and ConstantUse's deconstruct_keys reads only the keys asked for, looking constants up only for those two. On an app with 61,000 references, a query of the references to one pack took 0.29 seconds and takes 0.12 now. A query of violations took 0.13 seconds and takes 0.08.
A reference's violations say whether a package_todo.yml records each
one, but where couldn't ask, so finding what `pks check` reports took
a block. `where(unrecorded: true)` finds the references with such a
violation now, and `where(unrecorded_violation_types: 'privacy')` those
with one of a type, from Reference#unrecorded_violation_types:
References.where(producing_pack: "packs/foo", unrecorded: true).anonymous_source_counts
A violation's strictness is left out, as it's how the pack is set up,
and a recorded violation of a strict rule isn't what `pks check`
reports.
Loading a collection's sources printed a dot for each violation or reference in it, after the dots for the files it parsed, and did it for every new query, though the sources load quickly once the files are parsed. For a pack with 24,000 references, that was 24,000 dots each time, even when nothing was parsed. Only the files parsed get a dot now, so a repeated query prints a few lines.
Every spec of references uses what spec/support/referenced_packs.rb says pks lists for its app, and pks_spec.rb checks that against pks, but only where pks is installed, which CI wasn't. So a change to the app or to that listing that pks wouldn't agree with passed in CI. bin/install-pks downloads pks 0.7.0, the oldest version that query_packwerk supports, into tmp/pks, and checks the archive's sha256 before unpacking it. CI runs it before the specs and puts tmp/pks on the PATH, and the check fails in CI rather than skipping when pks isn't there. It covers Linux on x86_64, as in CI, and macOS on Apple Silicon.
A violation's owned compared its producing pack's owner with nil, but Package#owner gives 'Unowned' rather than nil for a pack without one, so owned was always false: `where(owned: true)` found no violations, and `where(owned: false)` found them all. It compares the owner with Packages::UNOWNED now, as Package's and Reference's owned do.
References.all kept each pack it found in a hash, so a pack is looked up once however many references name it, but each of the two keyword arguments did that inline, naming its key twice in a line of more than 100 characters. A lambda does it now, so each argument reads as the pack it is.
package_todo.yml files and pks name each constant from the top, as ::Foo::Bar, so `where(constant_name: 'Foo::Bar')` found nothing, though that's how code names it. Violations and References qualify a constant name that where is given, alone or in an array, before they match it, and leave a pattern such as a Regexp as it is.
violations_for, references_for and the rest take a name in packs/ as
honey_production, through full_name, but package didn't, so
`package("honey_production")`, as the README writes it, gave nil, and
`package("honey_production").incoming_references` raised
NoMethodError. package goes through full_name too now.
reload! and each collection's reload! had `void` sigs, and sorbet-runtime makes a void method return T::Private::Types::Void::VOID, so after `reload!` the console printed `# => T::Private::Types::Void::VOID`. They return nil now, as the console's help methods do, so it prints `# => nil`.
5efecb8 read the version from a line of pks's stdout that is only `pks <version>`, so a build that prints more after it, as `pks 0.7.0 (abc1234)`, would fail the check, though it's new enough. The version is the word after `pks ` at the start of a line now, as before 5efecb8, which anchored it at the start of the output.
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
1 participant
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
Adds
References: every reference from one pack to a constant that another pack defines, whether or not it breaks the packs' rules, aspks list-referenceslists them. It has the same methods asViolations, so query_packwerk can show how a pack is used where the rules allow it, which package_todo.yml files don't record:QueryPackwerk.references_for(pack)andreferences_from(pack), andPackage#incoming_referencesand#outgoing_references, get the references to or from a pack, asviolations_forandtodos_fordo for violations. They, andviolations_for,packageand the rest, take the name of any pack that ParsePackwerk finds, as., rather than puttingpacks/before it, or a pack's short name in packs/.wherecan query what it can for a violation (the constant, packs and owners), along withfile,allowed,violation_types, andunrecordedandunrecorded_violation_typesfor the violations that no package_todo.yml records, whichpks checkreports. It reads only the keys it's asked for, a block narrows what they find, rather than taking their place, and a constant named without its leading::, asFoo::Bar, finds::Foo::Bar.reload!forgets the references too, so the next query runs pks again, and it makes ParsePackwerk read the packs and their package_todo.yml files again, which it didn't before. It returns nil, which the console shows plainly, rather than Sorbet's void.help_reference,help_referencesand the README describe them.Fixes #8.
pks
bin/pksif the app has one, as an app that pins pks's version does, or elsepkson thePATH. It checks for 0.7.0 or later, the first version withlist-references, each time it runs pks, and when pks is missing or older, the error links to pks's installation instructions. Abin/pksthat isn't executable, as a DotSlash file saved from a release isn't at first, gets an error that says tochmod +xit, rather than pks on thePATHrunning instead. pks's warnings, as about a file it can't parse, go to stderr.**/glob doesn't, so a pack that ParsePackwerk doesn't find is read from its package.yml.Violationsdon't use pks, so nothing changes for an app without it.Finding sources
Barinsidemodule Foo, which matching names misses. A reference that pks infers from an association, ashas_many :tacosis for::Taco, gets the call as its source (Say where an association's reference is, and split out list-references' violations pks#80 documents both).FileCache#get_full_source_atconverts them.Foo.bar(Foo), the secondFoo's source isn't the whole call. Finding a constant by name skips theFooinFoo::Bar, which neither packwerk nor pks counts as a use of::Foo. Both change what violations find too, in those cases.Commits
ViolationandViolationsshare into two abstract classes,ConstantUseandConstantUses, whichReferenceandReferencesalso subclass. A subclass says only how to find its sources in a file. Nothing changes for callers, andgit show --color-moved 70d82e9shows the move.FileCache#get_full_anonymous_sourceslost its only caller and is gone.sorbet/rbi/todo.rbichanges by one line, because regenerating it would also turnActionView::ActionViewErrorback into a module, which Sorbet rejects.whereon an empty collection, which raisedNoMethodErroronnil.violations_fordid that in an app whose package_todo.yml files record nothing, andreferences_forwould in an app with no references between packs.References.x.reload!bust ParsePackwerk's cache, and checks pks's version each time it runs.all's file cache, so a file is parsed once untilreload!, for violations too.violations_for,references_forand the rest take the name of any pack.counttake a block or an item, as Enumerable's does, andwheretake a block along with its keys. Both ignored them before.is_active_recordfalse, rather than nil, for a class that isn't a model.sources_with_locationsand the other source methods as what they return.whereasks for, so in a Rails app it no longer looks up each reference's constant.unrecordedandunrecorded_violation_typeskeys.bin/install-pks, which checks its sha256, so the check of the specs' listing against pks runs there.ownedtrue when its producing pack has an owner. It compared the owner withnil, butPackage#ownergives'Unowned'rather thannil, so it was always false.References.all.wherefind a constant named without its leading::, for violations too.packagetake a pack's short name, asviolations_fordoes.reload!, so the console prints# => nilrather than# => T::Private::Types::Void::VOID.pks 0.7.0 (abc1234), passes the check.Design notes
References.allruns pks once and keeps what it lists, asViolations.allkeeps its violations.references_forfilters that, rather than running pks with--defining-pack, since pks reads every file either way.references_fromfilters it too. pks with--referencing-packreads only that pack's files, but a run for one pack couldn't answer the next query.all's file cache, so the files it parses stay in memory untilreload!, about 200 KB for a file of 100 lines. The README says so.ownedis true when the producing pack has an owner, for references and, since 8c065bc, violations too.Test plan
spec/support/referenced_packs.rbwrites an app with an allowed reference, recorded and unrecorded violations, a relatively named constant, an association, an ERB template, a constant after multibyte characters, and a reference to a pack without an owner, along with what pks 0.7.0 lists for it. The specs use that listing in place of pks. With pks 0.7.0 on thePATH, as CI has it frombin/install-pks,pks_spec.rbchecks the listing against pks. Without pks, it skips that check, except in CI, where it fails.pksscripts coverbin/pkstaking precedence over thePATH, abin/pksthat isn't executable, a missing pks, an old pks, a version that can't be read, a version that changes between runs, a failing run, passing on warnings, and output in a locale that isn't UTF-8.reference_spec.rbandreferences_spec.rbcover each kind of reference's sources,where, the aggregate methods,reload!, a pack that has no package.yml, and a pack that a symlink points to.References.alltook a few seconds longer than pks did.FooinFoo::Bar. A query of the references to one pack takes 0.12 seconds, where it took 0.29.bundle exec rspecin random order on Ruby 3.3, 3.4 and 4.0 (170 examples, with the pks check pending when pks isn't installed, and passing with pks 0.7.0),rubocop, andsrb tc.