[dzatona]

For #1834. The filtered view is a modifier passed alongside --dump.

What it does

--filtered narrows what --dump prints. It leaves out two kinds of entry: those whose span is
no longer a removal candidate, and those whose recorded text no longer reads the same at that
span.

necessist --dump --filtered
necessist --dump --filtered --verbose
necessist --dump --filtered tests/a.rs

The candidate set comes from parsing, along the --dump-candidates path. That is what makes the
configuration file apply: a stored row holds only a span, its text, an outcome, and a URL. It
carries no framework and no enclosing test, so the existing ignore and selection rules cannot be
replayed against it directly. Parsing produces them
already, and this intersects the result with the stored rows.

How an entry is decided

is_current_candidate requires both halves:

if !span_test_maps.statement.contains_key(span)
    && !span_test_maps.method_call.contains_key(span)
{
    return false;
}

span.source_text().is_ok_and(|current| current == *text)

The candidate check is first for three reasons. It is the cheaper of the two. It keeps
source_text off spans this parse did not produce. And computing a span's offsets asserts that if
the text up to its end is all ASCII then so is the text up to its start -- automatic when the end
follows the start, but a span read back from the database can hold one where it does not.

The text comparison is not redundant with Ok. An overlong column is clamped rather than rejected,
so source_text can return an Ok holding the wrong text; the comparison decides either way.

What it prints

One note always, stating what the filter means and that the outcome shown is the recorded one, not
one re-established against the current source. A second note when entries were hidden, with the
count -- only entries that would otherwise have been printed are counted, so --verbose changes
the number. The existing --verbose suggestion appears only when it would actually add something.

Why a modifier works here

The filtered view needs the parse, so it cannot return as early as --dump does. --dump returns
from prepare before backend_for_framework, which is why it keeps working on a tree that does
not parse and on one where no backend resolves. So there are two branches rather than one, and
they are spelled out rather than left to fall through:

// before the backend is resolved and before anything is parsed
if context.opts.dump && !context.opts.filtered { ... }

// after the parse, next to --dump-candidates and --dump-candidate-counts
if context.opts.dump && context.opts.filtered { ... }

The only change on the plain --dump path is the added && !context.opts.filtered, which is
trivially true when --filtered is absent.

Checking that plain --dump did not change

Since that is the thing worth being sure about, I checked it by running rather than by reading. I
compared a necessist binary from main at 29382632, which has no --filtered at all, against
this branch over eighteen --dump scenarios. Each pair used the same project and database; exit code and stderr matched exactly, and captured stdout matched in
every scenario. All eighteen agree.

They include the two cases the early return exists for -- a source file that no longer parses, and
a project with no resolvable backend -- as well as a missing database, deleted sources,
--verbose, --no-lines-or-columns, and each option --dump refuses to be combined with.

Options it cannot be combined with

--filtered requires --dump; on its own it is rejected with

--filtered can be used only with --dump

which follows the shape of the existing --write check. It appears in no incompatible! group,
because as a modifier it inherits every restriction --dump has and repeating them would let the
two drift apart. A test passes --dump --filtered with each option --dump is incompatible with
and asserts the existing message, so the inheritance is pinned rather than assumed.

After backend resolution and parsing succeed, a missing database uses the same message --dump
already prints:

--dump was passed, but no sqlite database found at "..."

sqlite::init is untouched -- core/src/sqlite.rs is byte-identical to main.

The help text states the scope limitation: the view is limited to the files this invocation
parses, so naming fewer paths hides more entries.

What it does not do

It does not re-establish outcomes. A matching location and matching text do not make an old
outcome current -- the test may pass now for a different reason. The note says so; the flag cannot
check it without running the tests, which is the thing being avoided.

It relies on source_text returning an error rather than panicking on an out-of-range span, which
is in main as of 6fae088.

Tests

40 trycmd cases in necessist/tests/necessist_db_filtered, run after the necessist_db_present
cases, plus three in necessist_db_absent and two in necessist_db_present. Cases that need a modified fixture run in a copy at
target/necessist_db_filtered/<test-case>, made by necessist/tests/fixture_copies from a fixture
in which the trycmd test has just run Necessist. They
cover rewritten spans and method calls, configured-out candidates, whitespace and case changes,
deleted and unparsable files, the hidden count with and without --verbose,
--no-lines-or-columns, the scope limitation, the inherited --dump restrictions, --filtered
without --dump, and that no tests are run. check_toml_files covers the new directory, and
fixture_copies_match_tomls checks that every copy has a case and every case a copy.

Since this changes necessist/tests/trycmd.rs, the snapshot of the necessist end-to-end test,
necessist/tests/end_to_end_tests/1/necessist.with_config.stdout, is regenerated with BLESS=1.

[dzatona]

The macOS third_party_0 job differs from the expected output in one line:

-$DIR/test/Sync.t.sol: `.executeActions(actions, params)` failed
+$DIR/test/Sync.t.sol: `.executeActions(actions, params)` timed-out

That job does not pass --dump-filtered, so the branch added in prepare is not exercised there.

Could you rerun that job? I only have pull permission.

[smoelius]

The macOS third_party_0 job differs from the expected output in one line:

-$DIR/test/Sync.t.sol: `.executeActions(actions, params)` failed
+$DIR/test/Sync.t.sol: `.executeActions(actions, params)` timed-out

That job does not pass --dump-filtered, so the branch added in prepare is not exercised there.

Could you rerun that job? I only have pull permission.

Sorry, a recent upgrade of Forge has caused some of the third-party tests to become flaky. I need to look into this problem more and determine how to handle it. In the meantime, please don't worry if one of the third-party tests fails.

Thanks very much for this PR! It's clear that you've put a lot of thought into this. Also, having the example code makes your comments in #1834 much easier to intuit.

I have a little bit of a reviewing backlog, so it may be a few weeks before I comment on it. Also, this PR is non-trivial, so it may require a few rounds of review. But please rest assured that I won't let it drop!

[smoelius]

@dzatona I would like to propose a design change and would like your thoughts.

Rather than have a completely new option --dump-filtered, what if we had a --rescan flag that one passed along with --dump?

Note that there is already a --no-lines-or-columns flag. --rescan would adjust how --dump outputs information, similar to --no-lines-or-columns.

Also, --rescan would fit nicely with --reset and --resume. Each says, "something has changed". For --reset and --resume, a database now exists where there was none before. For --rescan, the configuration file has changed.

Is there some reason why this idea would not work or would be awkward?

What do you think about this idea, generally?

[dzatona]

@smoelius Passing it alongside --dump is the right shape -- better than a separate top-level option. The word rescan I am less sure about, and the two are separable.

You asked whether it would be awkward. The one place I think it is: --rescan would change what --dump means, not how it prints. Today necessist --dump foo.rs ignores the path -- --dump returns from prepare before source paths are canonicalized or the backend parses. With --rescan those same positional arguments start deciding what appears, because the candidate set is whatever this invocation parses. That same early return is why --dump still works on a tree that does not parse or where no backend resolves, and why --rescan could not.

That is also why the comparison to --no-lines-or-columns does not quite hold: that one is a general output flag -- it applies to a normal run and to --dump-candidates as well -- and it never changes which rows appear or whether the command can run.

On the name: --reset and --resume are about the lifecycle of a run and its database, which is why they read naturally together. --rescan sits in that row grammatically but is passed with --dump, so it reads as a mode of running rather than a modifier of dumping. It also suggests outcomes are re-established, when what happens is that stored outcomes are filtered.

If the shape is settled I will make the change; the word we can pick separately.

[smoelius]

The word rescan I am less sure about...

You would prefer --filtered?

A reason I hesitate is that it suggests "the configuration file has changed" implies the output will be smaller.

Currently, that is the case. But I worry that, in the future, Necessist might have a configuration that, when changed, could cause the output to grow.

Here is a hypothetical (not something I really want to support). Currently, the database stores Skipped outcomes for dry-run failures. Suppose Neccesist had a show_skipped_candidates configuration that defaulted to false. Then changing that configuration to true could cause the output to grow.

Mind you, if we added such a configuration in the future, we could change the flag name at the same time.

Is there a rscan-like term that you would prefer?

I realize I said "filtered" in #1834, and I may be feature-creeping here a little bit. So if you would prefer --filtered, we can go with that.

[dzatona]

@smoelius Agreed, let's use --filtered.

The direction worry does not attach to the word, I think: --filtered says the printed database rows are a subset of what --dump prints, and that stays true whatever a future configuration does. What could grow is one filtered run compared with an earlier one, and no name promises anything about that.

--rescan bothered me for a different reason anyway: it suggests the outcomes were re-established, when they are the ones already recorded.

I will rework the PR as --dump --filtered.

[dzatona]

Reworked as discussed: the filtered view is now --dump --filtered, and the title and description
are updated to match.

Plain --dump is unchanged. Rather than argue that from the diff, I compared a necessist binary
from 064d2188 against this branch over eighteen --dump scenarios. The paired runs used the same
project and database; exit code and stderr matched exactly, and captured stdout matched in every
scenario, including the unparsable-tree and unresolvable-backend cases that justify the early
return. core/src/sqlite.rs is no longer touched at all.

--filtered is in no incompatible! group; it requires --dump and inherits its restrictions,
and there is a test that pins that.

[smoelius]

@dzatona I would like to request another architectural change.

necessist/tests/general/dump_filtered.rs is over 1300 lines. Rather than add a test file that big, I would prefer to add something with more structure.

So I would like to propose the following:

• Alongside necessist_db_absent and necessist_db_present, add a directory necessist_db_filtered.
• In that directory, put a toml file for each new test case.
• Have those test cases run after the necessist_db_present test cases.
• Adapt the trycmd test so that it copies the relevant fixture, matches on the name of the test case, and then performs whatever modifications are needed to the fixture copy.

I think there is no way to set the current director of a trycmd test case at runtime. So I think the fixture copies will need to have predictable paths (something like <dir-name>/<test-case>), and those paths placed under [cwd] in the toml files.

To copy the fixtures, please use a Rust helper function rather than cp -r. (I think the code currently uses cp -r.)

Please let me know if this idea is unclear or if you disagree with it. Also, if this is too large of an ask, please let me know. I could make these changes and push to your branch.

[dzatona]

@smoelius That makes sense, and it is not too large. I'll make the changes on this branch and rebase onto main in the same push.

All the tests in dump_filtered.rs fit this structure, so that file goes away entirely. Cases that need a modified fixture will run in a copy at target/necessist_db_filtered/<test-case>, set as fs.cwd in their toml file. Cases that need an unmodified fixture, like the flag incompatibilities, will use --root=fixtures/basic as the existing cases do, and the --help and --check-skill cases need no project at all.

Three points where I'll take the default below unless you prefer otherwise:

• Several tests currently build small projects from strings: for whitespace-only changes (indentation, line endings), an ASCII case change, ignored_functions, and a project whose test deletes a sentinel file. Since the helper copies fixtures, I'll add these as fixtures/whitespace, fixtures/case, fixtures/ignored_function, and fixtures/marker.
• The test that --dump --filtered runs no tests currently checks that a marker file was not created. trycmd can only check for files that are expected, so I'll invert it: the fixture's test deletes sentinel, the helper recreates it after the populating run, and the case has a .out directory expecting sentinel to still be there. I tried this locally, and a case that does run the tests fails with Expected file, was missing.
• check_toml_files requires fs.cwd = "../../.." and a --root=fixtures/... argument. In necessist_db_filtered I'll keep that shape and also accept two more: fs.cwd pointing to the case's copy with no --root, and neither fs nor --root for the --help and --check-skill cases. That seems better than leaving the directory unchecked.

[smoelius]

@smoelius That makes sense, and it is not too large. I'll make the changes on this branch and rebase onto main in the same push.

Thanks very much. I'll try to get back to this soon.

[smoelius]

@dzatona I think the change regarding the necessist invocation may be substantial. I will comment again once you had a chance to consider it.

[smoelius]

Nit: Please rename this directory to something that doesn't end in cases.

To me, *_cases sounds like the directory contains test cases.

[smoelius]

I would prefer that this enum go away, and that string literals be used instead (e.g., "basic").

I think that will reduce cognitive load for future reviewers of this code. I think it will also make grepping for references to fixture directories easier.

[smoelius]

I probably didn't make my intentions clear, but I was imagining that the existing necessist invocation in the trycmd test would play the role of this one:

necessist/necessist/tests/trycmd.rs

Lines 33 to 36 in 2938263

	cargo_bin_cmd!("necessist")
	.args(["--root", ROOT, "--timeout", TIMEOUT])
	.assert()
	.success();

In other words, for each test case that requires a necessist.db file (i.e., the present and filtered cases), the statement in the trycmd test would provide it.

Note that this may require:

• expanding that statement, e.g., to loop over multiple fixtures
• adding additional fixtures

To determine the fixtures in which to run necessist, you could use the CASES array.

If I understand correctly, the marker case does not fit this pattern cleanly. When looping over the fixtures, I think it would be fine to treat the marker case specially with an if statement.

Please tell me if this idea is not clear or if you think it would not work.

[dzatona]

@smoelius Thanks, that is clear, and I think it will work. I'll have the existing invocation run Necessist in each fixture named in CASES, add fixtures for the three masters that are currently edited before the run, treat marker specially with an if, replace the Master enum with string literals, and rename the directory. I'll push this together with a rebase onto main.

[dzatona]

@smoelius I've pushed the changes from your review, rebased onto main. Since trycmd.rs changed, the snapshot of the necessist end-to-end test is regenerated in the same commit. On Windows one entry in it comes out differently (job): in_tomls.sort(); is passed there rather than failed. That one is mine, and I'll fix it tomorrow. The supply_chain failure in the same run reports a missing or invalid crates.io cache and an HTTP 429 error while fetching publisher information (job). Whenever you have a chance to take another look.

[smoelius]

Whenever you have a chance to take another look.

I will. Thanks for your patience with this review, @dzatona.

[smoelius]

Suggested change

	uses, so naming fewer paths can hide more entries"
	help = "Restrict --dump to entries that are still removal candidates according to the current configuration file"

[smoelius]

Oops. This was meant to be part of a larger review. I did plan to request this change, though.