tidy watcher #114209

klensy · 2023-07-29T16:34:29Z

MVP of tidy watcher:

Allows to check that changes to some pieces of text synced in different files (or at least warn about it). Should resolve situations like: "Oh no, I've changed that constant here and here, but forgot to update it in other 3 places."

Current state is very MVP'ish, so it simply works and nothing more, posted to get some feedback.

Usage: wrap text that need to be synced with arbitrarily chosen tags:

some.rs:

//  tidy-keep-sync-with=tidy-ticket-foo
const FOO: usize = 42;
//  tidy-keep-sync-with=tidy-ticket-foo

some.sh:

#  tidy-keep-sync-with=tidy-ticket-foo
export FOO=42
#  tidy-keep-sync-with=tidy-ticket-foo

and add them to watcher.rs in tidy, adding into one group:

    add_group!(
        ("src/tools/opt-dist/src/main.rs", "728c2783154a52a30bdb1d66f8ea1f2a", "tidy-ticket-perf-commit"),
        ("src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile", "76c8d9783e38e25a461355f82fcd7955", "tidy-ticket-perf-commit")
    ),

If hashes differs, actual and expected ones will be printed:

tidy error: The code blocks tagged with tidy watcher has changed.
It's likely that code blocks with the following tags need to be changed too. Check src/tools/tidy/src/watcher.rs, find t
ag/hash in TIDY_WATCH_LIST list and verify that sources for provided group of tags in sync. Once that done, run tidy aga
in and update hashes in TIDY_WATCH_LIST with provided actual hashes.
tidy error: hash for tag `tidy-ticket-sess-time-item_types_checking` in path `compiler/rustc_hir_analysis/src/lib.rs` mi
smatch:
  actual: `842e23fb65caf3a96681686131093316`, expected: `842e23fb65caf3a96681686131093317`
  Verify that tags `["tidy-ticket-sess-time-item_types_checking", "tidy-ticket-sess-time-item_types_checking"]` in sync.

Added few sync me commented code as examples.

--bless implemented, but i don't like current json config, it's huge.

r? @ghost

klensy · 2023-07-30T15:16:11Z

Added few more entries a49ed53, but i don't know if all of them actually sync as of now, this should be reviewed.

bors · 2023-07-31T19:18:13Z

☔ The latest upstream changes (presumably #114294) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-09-18T16:29:29Z

☔ The latest upstream changes (presumably #115795) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2023-10-02T17:55:51Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

klensy · 2023-10-02T17:58:31Z

Okay. Implementation is trivial, but most of asserted pieces of code in compiler, so r? compiler. Didn't checked if current state is sync, as i don't always understand whats functions do.

RalfJung · 2023-10-02T18:13:42Z

compiler/rustc_const_eval/src/interpret/projection.rs

@@ -149,6 +149,7 @@ where
                None => {
                    // For unsized types with an extern type tail we perform no adjustments.
                    // NOTE: keep this in sync with `PlaceRef::project_field` in the codegen backend.
+                    // FIXME: that one? compiler/rustc_codegen_ssa/src/mir/place.rs


Specifically this:

rust/compiler/rustc_codegen_ssa/src/mir/place.rs

Lines 148 to 154 in 79bfd93

_ if self.llextra.is_none() => {

debug!(

"unsized field `{}`, of `{:?}` has no metadata for adjustment",

ix, self.llval

);

return simple();

}

I don't see how an automatic checker can help here. This is about keeping the semantics in sync.

I just run over codebase, found few comments like "keep in sync with" and wrapped them in tags. Of course this check can't say if two places is valid, only ping author to give it chance to check things manually.

RalfJung · 2023-10-02T18:15:40Z

If hashes differs, actual and expected ones will be printed:

This needs to give very clear guidance on what I should do now, otherwise it will be very frustrating.

klensy · 2023-10-02T18:20:41Z

If hashes differs, actual and expected ones will be printed:

This needs to give very clear guidance on what I should do now, otherwise it will be very frustrating.

It slightly more descriptive, forgot to update first message, but can be improved.

Nadrieril · 2023-10-02T19:16:27Z

compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs

@@ -1144,6 +1150,7 @@ impl<'p, 'tcx> Fields<'p, 'tcx> {
        })
    }

+    // tidy-ticket-wildcards


I would have expected that we reuse the tag for all the places that need to be in sync. E.g. if I modify wildcards() here and not arity(), it would ping me "you should also check arity".

Added grouping for tags, should write

tidy error: hash for tag `tidy-ticket-ast-from_token` in path `compiler/rustc_ast/src/token.rs` mismatch: actual: `eecacc0fe4c615f7c71409c3ebfcff80`, expected: `70666de80ab0194a67524deeda3c01b8` Verify that tags `["tidy-ticket-ast-from_token", "tidy-ticket-ast-can_begin_literal_maybe_minus", "tidy-ticket-rustc_p arse-can_begin_literal_maybe_minus"]` in sync.

Nice! Could you make it explain what it's about for those who don't know? Something like: "the code block tagged with tidy-ticket-ast-from_token has changed. It's likely that code blocks with the following tags need to be changed too. Once that is done, run tidy again with the --blessed flag to update the hashes. The related tags are ["tidy-ticket-ast-from_token", "tidy-ticket-ast-can_begin_literal_maybe_minus", "tidy-ticket-rustc_parse-can_begin_literal_maybe_minus"]"

Added tidy-keep-sync-with= and updated error message with parts from your suggestion.

Nadrieril · 2023-10-02T19:32:41Z

The tag should have a name that explains what it's for if we encounter it in code. Something like tidy-keep-sync-with=<tag>.

klensy · 2023-10-04T15:54:25Z

The tag should have a name that explains what it's for if we encounter it in code. Something like tidy-keep-sync-with=<tag>.

Don't want to implement smarter parsing for tags (at least for now), can just make it longer, like tidy-keep-sync-try_visit_primitive tidy-keep-sync-visit_value?

Anyway, accidental removing of tag from code will enrage tidy, which will say something special about it.

Nadrieril · 2023-10-04T16:17:06Z

You don't need to do any parsing, just allow "=" in the list of allowed symbols in a tag. I just think it's more legible

Nadrieril · 2023-10-04T16:19:26Z

You might want to wait and see if compiler contributors actually want this before investing more time in it though. As far as I am concerned this adds unnecessary friction; I'm waiting to hear from ppl who work on files where this would be helpful.

jieyouxu · 2024-11-11T17:44:57Z

Re-nominating this PR for T-compiler triage meeting to gauge team interest.

Notes for T-compiler triage meeting

This experimental PR proposes a tidy watcher mechanism which is intended to check if pieces of code between multiple locations are kept in sync properly by comparing their hashes.

Yes, this is for code that will not emit errors; for example git hash for rustc-perf in dockerfile and opt-dist, or code commented in style "please keep me in sync", which will not immediately cause errors, but will silently do wrong things; or assert pieces in rust, bash, python code simultaneously (which will hard to sync other way).

How does the team feel about such a mechanism:

Do we have the interest for such a mechanism?
Do we have concerns about the mechanism design (see below)?
Do we have the resources available to support the PR author in finishing the (initial) implementation?

Known implementation limitations and concerns (as of writing this comment)

It currently does not have a --bless mechanism in place to update the hashes if the changes are intentional, e.g. from a larger refactor PR, cf. tidy watcher #114209 (comment).
It's not super clear how to check "keep in sync" (semantic in-sync?), cf. tidy watcher #114209 (comment).
It's not clear if team members and compiler contributors actually want this mechanism because it can add friction, cf. tidy watcher #114209 (comment)
- We can propose an MCP for this, but I wanted to check for some initial preferences in the triage meeting.

@rustbot label +I-compiler-nominated +S-waiting-on-team -S-waiting-on-author

Also cc @rust-lang/bootstrap because this concerns tidy which we (T-bootstrap) maintain.

klensy · 2024-11-11T19:46:17Z

This PR was already discussed: https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202023-10-26/near/398708149, so i need rebase this stuff and reread it again.

apiraino · 2024-11-13T16:26:12Z

removing nomination, postponing a possible further discussion after the rebase (this summary will be useful anyway). Thanks @klensy for your patience.

@rustbot label -I-compiler-nominated

rustbot · 2025-01-17T11:21:08Z

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in coverage instrumentation.

cc @Zalathar

RalfJung · 2025-01-17T11:26:49Z

Once that done, run tidy aga
in and update hashes in TIDY_WATCH_LIST with provided actual hashes.

Is there any way this could be automated? Currently this all seems somewhat tedious to use.

klensy · 2025-01-17T11:28:57Z

Okay, rebased.

Current state: rebased with almost mindlessly reapplied checks (so need to review if linked pieces of code actually sync) and added more fixme's to sync more codepieces.

Once that done, run tidy aga
in and update hashes in TIDY_WATCH_LIST with provided actual hashes.

Is there any way this could be automated? Currently this all seems somewhat tedious to use.

Yes, i'm thinking about way to implement bless #114209 (comment)

klensy · 2025-01-17T12:07:42Z

There exist https://github.com/stepchowfun/tagref with somehow similar idea, but looks like it can only check existence of tag links, not content.

Zalathar · 2025-01-17T13:04:53Z

compiler/rustc_mir_transform/src/coverage/query.rs

@@ -109,6 +109,7 @@ fn coverage_ids_info<'tcx>(
        // to any particular point in the control-flow graph.
        // (Keep this in sync with the injection of `ExpressionUsed`
        // statements in the `InstrumentCoverage` MIR pass.)
+        // FIXME: sync me


This one is a semantic sync rather than a data sync, and violating it would cause a bunch of tests to break, so it's probably not worth the hassle of adding sync-check tags here.

impl bless

klensy · 2025-01-17T16:18:28Z

Okay, bless works; but saving state into json still ugly.

Urgau · 2025-01-17T17:33:22Z

compiler/rustc_session/src/config/cfg.rs

@@ -176,6 +176,7 @@ pub(crate) fn default_configuration(sess: &Session) -> Cfg {
    //
    // NOTE: These insertions should be kept in sync with
    // `CheckCfg::fill_well_known` below.
+    // FIXME: sync me


default_configuration and CheckCfg::fill_well_known are loosely synced, there are some cfgs in default_configuration like emscripten_wasm_eh that shouldn't be in CheckCfg::fill_well_known and the inverse is also for some cfgs (miri, rustfmt, ...).

Is it possible to still apply this tidy annotation in that case ?

This pr/feature can only assert that some spans unchanged, nothing more. It's up to author/reviewer check actual content. Should i wrap entire default_configuration/fill_well_known fns?

rustbot · 2025-01-18T10:52:12Z

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

klensy · 2025-01-18T11:19:59Z

src/librustdoc/core.rs

+    // tidy-keep-sync-with=tidy-ticket-sess-time-item_types_checking
    // NOTE: These are copy/pasted from typeck/lib.rs and should be kept in sync with those changes.
    let _ = tcx.sess.time("wf_checking", || {
        tcx.hir().try_par_for_each_module(|module| tcx.ensure().check_mod_type_wf(module))
    });
+    // tidy-keep-sync-with=tidy-ticket-sess-time-item_types_checking


This looks clearly desynced (or not), only similar thing is

rust/compiler/rustc_hir_analysis/src/lib.rs

Lines 140 to 151 in 8e59cf9

tcx.sess.time("coherence_checking", || {

tcx.hir().par_for_each_module(|module| {

let _ = tcx.ensure().check_mod_type_wf(module);

});

for &trait_def_id in tcx.all_local_trait_impls(()).keys() {

let _ = tcx.ensure().coherent_trait(trait_def_id);

}

// these queries are executed for side-effects (error reporting):

let _ = tcx.ensure().crate_inherent_impls_validity_check(());

let _ = tcx.ensure().crate_inherent_impls_overlap_check(());

});

bors · 2025-01-18T15:49:13Z

☔ The latest upstream changes (presumably #135678) made this pull request unmergeable. Please resolve the merge conflicts.

This comment has been minimized.

Sign in to view

klensy force-pushed the better-than-remembrall branch from 8b8f8f8 to 434286f Compare July 29, 2023 16:44

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Jul 30, 2023

klensy force-pushed the better-than-remembrall branch from a49ed53 to ca2cbb3 Compare July 31, 2023 09:48

klensy mentioned this pull request Aug 3, 2023

refactor builtin unsize handling, extend comments #114169

Merged

klensy force-pushed the better-than-remembrall branch from ca2cbb3 to 3f2a374 Compare September 7, 2023 16:19

rustbot added the O-unix Operating system: Unix-like label Sep 7, 2023

This comment has been minimized.

Sign in to view

klensy force-pushed the better-than-remembrall branch from 3f2a374 to e4c6a7c Compare October 2, 2023 17:22

klensy marked this pull request as ready for review October 2, 2023 17:55

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 2, 2023

rustbot assigned cjgillot Oct 2, 2023

RalfJung reviewed Oct 2, 2023

View reviewed changes

Nadrieril reviewed Oct 2, 2023

View reviewed changes

klensy force-pushed the better-than-remembrall branch from e4c6a7c to fa7b8ae Compare October 4, 2023 15:46

This comment was marked as resolved.

Sign in to view

rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Nov 13, 2024

apiraino added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 20, 2024

apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 5, 2024

klensy added 3 commits January 17, 2025 14:17

impl watcher

652f116

refresh expected ones

10d4a98

add fixmes

429ca32

klensy force-pushed the better-than-remembrall branch from 0b311d0 to 429ca32 Compare January 17, 2025 11:21

rustbot added the A-tidy Area: The tidy tool label Jan 17, 2025

Zalathar reviewed Jan 17, 2025

View reviewed changes

impl deser from config for watcher

1c5b209

impl bless

This comment has been minimized.

Sign in to view

fix tests

d66235a

Urgau reviewed Jan 17, 2025

View reviewed changes

refresh fixmes

fd7acd4

klensy commented Jan 18, 2025

View reviewed changes

	_ if self.llextra.is_none() => {
	debug!(
	"unsized field `{}`, of `{:?}` has no metadata for adjustment",
	ix, self.llval
	);
	return simple();
	}

	tcx.sess.time("coherence_checking", \|\| {
	tcx.hir().par_for_each_module(\|module\| {
	let _ = tcx.ensure().check_mod_type_wf(module);
	});

	for &trait_def_id in tcx.all_local_trait_impls(()).keys() {
	let _ = tcx.ensure().coherent_trait(trait_def_id);
	}
	// these queries are executed for side-effects (error reporting):
	let _ = tcx.ensure().crate_inherent_impls_validity_check(());
	let _ = tcx.ensure().crate_inherent_impls_overlap_check(());
	});

tidy watcher #114209

Are you sure you want to change the base?

tidy watcher #114209

Conversation

klensy commented Jul 29, 2023 • edited Loading

This comment has been minimized.

klensy commented Jul 30, 2023

bors commented Jul 31, 2023

This comment has been minimized.

bors commented Sep 18, 2023

rustbot commented Oct 2, 2023

klensy commented Oct 2, 2023

RalfJung Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Oct 2, 2023

klensy commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nadrieril commented Oct 2, 2023 • edited Loading

klensy commented Oct 4, 2023 • edited Loading

Nadrieril commented Oct 4, 2023 • edited Loading

Nadrieril commented Oct 4, 2023

This comment was marked as resolved.

This comment was marked as resolved.

jieyouxu commented Nov 11, 2024 • edited Loading

Notes for T-compiler triage meeting

Known implementation limitations and concerns (as of writing this comment)

klensy commented Nov 11, 2024

apiraino commented Nov 13, 2024

rustbot commented Jan 17, 2025

RalfJung commented Jan 17, 2025

klensy commented Jan 17, 2025 • edited Loading

klensy commented Jan 17, 2025

Choose a reason for hiding this comment

klensy commented Jan 17, 2025

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Jan 18, 2025

klensy Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

bors commented Jan 18, 2025

klensy commented Jul 29, 2023 •

edited

Loading

RalfJung Oct 2, 2023 •

edited

Loading

Nadrieril commented Oct 2, 2023 •

edited

Loading

klensy commented Oct 4, 2023 •

edited

Loading

Nadrieril commented Oct 4, 2023 •

edited

Loading

jieyouxu commented Nov 11, 2024 •

edited

Loading

klensy commented Jan 17, 2025 •

edited

Loading

klensy Jan 18, 2025 •

edited

Loading