tidy: add if-installed prefix condition to extra checks system

[Shunpoco]

Ref: https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Should.20Spellcheck.20check.20all.20files.3F/with/543227610

tidy now runs spellcheck (typos-cli) without adding --extra-checks=spellcheck option if the tool is already installed under ./build/misc-tools and the version is expected.
It will improve code quality without bothering engineers who doesn't want to use typos or who cleans up ./build directory frequently.

[Shunpoco]

r? @Kobzol

[rustbot]

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

[rustbot]

tidy extra checks were modified.

cc @lolbinarycat

[lolbinarycat]

Basically what you are recommending is that, once ./x test tidy --extra-checks=spellcheck is run for the first time, spellchecking will always be checked by future calls to ./x test tidy, unless the user removes the build directory. This sort of inconsistency seems like a very bad idea, and will likely lead to confusing to triage issues. Generally I object on principle to making tools more stateful, especially considering we've done a bunch of work to make it less stateful, and removing pieces of code that depend on the system environment.

I also object to adding more undocumented behavior. Even if this behavior was documented, the way it is implemented currently needlessly ups the complexity of tidy, making spellcheck both an extra-check an a regular check. I think all the spellcheck logic should be contained in the extra-checks dir.

Additionally, we already have a way to control if tidy is run automatically, which is build.tidy-extra-checks in bootstrap.toml, which is already enabled by default for the tools profile.

If we do want this behavior, I think it should be opt-in, at least as far as tidy is concerned, likely through another prefix along the lines of auto, perhaps if-installed. That way the pre-push hook could opt-in to that behavior by default, while other users (such as CI!!) would be unaffected.

I would also like to remind everyone that an ad-hoc implementation of typo checking has already broken CI once, so let's not repeat that mistake.

[lolbinarycat]

I'm going to give the if-installed idea a shot, but your contribution is still appreciated, even if I have my issues with the approach. The ensure_version function seems quite useful and flexible, for example.

[Kobzol]

FWIW, we agreed to use this mechanism with @jieyouxu in #t-infra/bootstrap > Should Spellcheck check all files? @ 💬.

I don't think that this would ever affect CI. Both because it will never have the binary preinstalled, and in the tidy job we do actually opt into spellcheck, so the best effort "if-installed" mode will not trigger.

Tidy is inherently stateful, it depends a lot on git state and it skips some checks if the filesystem is read-only (maybe there are also some other things).

The original idea hwere was to make more people use spellcheck locally. I'm not actually sure if it's needed, I don't often see spellcheck errors on CI, but it also sounded like it wouldn't do much harm.

The problem is that installing the typos binary everytime is a non-starter, but also if we keep it installed in ./build, then it will just get removed from time to time, and people will stop running spellcheck. That won't be solved neither by this PR, nor by build.tidy-extra-checks. With this PR, once you remove the build dir, you stop running spellcheck. With build.tidy-extra-checks, once you remove the build dir, you'll always rebuild typos from scratch, which also doesn't sound ideal.

[lolbinarycat]

FWIW, we agreed to use this mechanism with @jieyouxu in #t-infra/bootstrap > Should Spellcheck check all files? @ 💬.

Yes, and I would've appreciated a ping on that honestly because you seemed to be missing a lot of context (like with opt-level, I'm pretty sure I did a bunch of manual testing about that to figure out the best level).

I don't think that this would ever affect CI.

I still think it should have an explicit check.

Tidy is inherently stateful, it depends a lot on git state and it skips some checks if the filesystem is read-only (maybe there are also some other things).

I think we have different definitions to what "stateful" means. To me, all of these things (git state, fs metadata, every file in the tree) act as the "input" to tidy, and tidy merely acts as a pure function on top of these, never mutating them unless --bless is called. Every file is either an input or an output, not both.

The problem is that installing the typos binary everytime is a non-starter

We're already doing that for everyone on the "tools" profile, and I haven't heard anyone complain. It takes 13 seconds for me, compared to the total 2 minute 14 second runtime of ./x test tidy --extra-checks=spellcheck when running from scratch.

The original idea here was to make more people use spellcheck locally. I'm not actually sure if it's needed, I don't often see spellcheck errors on CI, but it also sounded like it wouldn't do much harm.

The cost is in code complexity and maintenance burden, as it always is. The current PR has a lot of duplicated code, and it also breaks encapsulation on the extra_checks module for no good reason.

[Kobzol]

Yes, and I would've appreciated a ping on that honestly because you seemed to be missing a lot of context (like with opt-level, I'm pretty sure I did a bunch of manual testing about that to figure out the best level).

Fair enough, will do next time!

I think we have different definitions to what "stateful" means. To me, all of these things (git state, fs metadata, every file in the tree) act as the "input" to tidy, and tidy merely acts as a pure function on top of these, never mutating them unless --bless is called. Every file is either an input or an output, not both.

Sure, but in that case the typos-cli binary being present on disk is just another input, right? That's no different from the other things you mentioned.

We're already doing that for everyone on the "tools" profile, and I haven't heard anyone complain. It takes 13 seconds for me, compared to the total 2 minute 14 second runtime of ./x test tidy --extra-checks=spellcheck when running from scratch.

Hmm, it seems surprising to me that tidy runs so slowly for you. Well, maybe we don't really need to do anything after all. People can opt into spellcheck with build.tidy-extra-checks, we can't force them to do it, but also maybe we really shouldn't.

Maybe to suggest a compromise: would you agree with running typos on a best effort basis (without installing it) only in the pre-push hook? That was the original-original idea.

The cost is in code complexity and maintenance burden, as it always is. The current PR has a lot of duplicated code, and it also breaks encapsulation on the extra_checks module for no good reason.

That seems surprising to me. If anything, this PR seems to remove duplication? Also, I would prefer to actually move stuff out from the extra_checks module into individual modules for the individual extra checks. The extra checks module now contains a bunch of unrelated things and because everything is stuffed inside a single check, it doesn't correctly use the recently introduced machinery for nice error messages and warnings.

[lolbinarycat]

Sure, but in that case the typos-cli binary being present on disk is just another input, right? That's no different from the other things you mentioned.

It's different because tidy installs typos-cli, thus modifying it.

Maybe to suggest a compromise: would you agree with running typos on a best effort basis (without installing it) only in the pre-push hook? That was the original-original idea.

I would not oppose that change, as long as there is a reasonable way to opt out. That's basically just a stripped down version of my if-installed idea. (maybe I could actually try implementing if-installed only for spellcheck initially, that would simplify things greatly)

That seems surprising to me. If anything, this PR seems to remove duplication?

ensure_version is basically a copy-pase of the first half of the existing function.

Also, I would prefer to actually move stuff out from the extra_checks module into individual modules for the individual extra checks. The extra checks module now contains a bunch of unrelated things and because everything is stuffed inside a single check, it doesn't correctly use the recently introduced machinery for nice error messages and warnings.

Sure, but if we do that, we should move the actual logic into the new file, not just a shim.

We could also make them into individual checks without moving them out of their current module, it would just likely mean not using the existing macro.

[lolbinarycat]

If you're happy, can I implement the if-installed mode on this PR? (of course I reset my changes at first)

@Shunpoco I have strong opinions on the interface, and parsing, so I went ahead and implemented that below, but if you want to implement the actual checking logic, go ahead!

If we want to refactor stuff into separate checks, I think that should be done in its own PR, we would already be doing enough here, and doing that properly would be quite involved.

diff --git a/src/tools/tidy/src/extra_checks/mod.rs b/src/tools/tidy/src/extra_checks/mod.rs
index a45af7fcf15..fdae949c408 100644
--- a/src/tools/tidy/src/extra_checks/mod.rs
+++ b/src/tools/tidy/src/extra_checks/mod.rs
@@ -736,9 +739,14 @@ enum ExtraCheckParseError {
     Empty,
     /// `auto` specified without lang part.
     AutoRequiresLang,
+    /// `if-installed` specified without lang part.
+    IfInstalledRequiresLang,
+
 }
 
 struct ExtraCheckArg {
+    /// Only run the check if the requisite software is already installed.
+    if_installed: bool,
     auto: bool,
     lang: ExtraCheckLang,
     /// None = run all extra checks for the given lang
@@ -750,8 +758,14 @@ fn matches(&self, lang: ExtraCheckLang, kind: ExtraCheckKind) -> bool {
         self.lang == lang && self.kind.map(|k| k == kind).unwrap_or(true)
     }
 
-    /// Returns `false` if this is an auto arg and the passed filename does not trigger the auto rule
-    fn is_non_auto_or_matches(&self, filepath: &str) -> bool {
+    /// Returns `false` if this is an auto arg and the passed filename does not trigger the auto rule,
+    /// or if it has `if_installed` set, and the required software is not installed.
+    fn is_unconditional_or_matches(&self, filepath: &str) -> bool {
+        if self.if_installed {
+            match self.lang {
+                _ => todo!("return false here if software not found"),
+            }
+        }
         if !self.auto {
             return true;
         }
@@ -792,22 +806,35 @@ impl FromStr for ExtraCheckArg {
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let mut auto = false;
+        let mut if_installed = false;
         let mut parts = s.split(':');
         let Some(mut first) = parts.next() else {
             return Err(ExtraCheckParseError::Empty);
         };
-        if first == "auto" {
-            let Some(part) = parts.next() else {
-                return Err(ExtraCheckParseError::AutoRequiresLang);
-            };
-            auto = true;
-            first = part;
+        loop {
+            if !auto && first == "auto" {
+                let Some(part) = parts.next() else {
+                    return Err(ExtraCheckParseError::AutoRequiresLang);
+                };
+                auto = true;
+                first = part;
+                continue;
+            }
+            if !if_installed && first == "if-installed"  {
+                let Some(part) = parts.next() else {
+                    return Err(ExtraCheckParseError::IfInstalledRequiresLang);
+                };
+                if_installed = true;
+                first = part;
+                continue;
+            }
+            break;
         }
         let second = parts.next();
         if parts.next().is_some() {
             return Err(ExtraCheckParseError::TooManyParts);
         }
-        let arg = Self { auto, lang: first.parse()?, kind: second.map(|s| s.parse()).transpose()? };
+        let arg = Self { auto, if_installed, lang: first.parse()?, kind: second.map(|s| s.parse()).transpose()? };
         if !arg.has_supported_kind() {
             return Err(ExtraCheckParseError::UnsupportedKindForLang);
         }

[Shunpoco]

@rustbot review

[Shunpoco]

Ah, waiting-on-review was already added.

@Kobzol @lolbinarycat Could you review in your spare time?

[lolbinarycat]

I'll do my best to take a look tomorrow

[Kobzol]

Suggested change

	"error: spellchecker has a problem. --extra-check=specllcheck may solve the problem."
	"error: spellchecker has a problem. --extra-checks=spellcheck may solve the problem."

[Shunpoco]

The file was already removed from this PR since we've discussed that (at least for now) spellcheck code should be under extra_checks/

[Kobzol]

Suggested change

	IfInsatlledRequiresLang,
	IfInstalledRequiresLang,

[Shunpoco]

Uh, thanks. I addressed in b49e56d

[Kobzol]

I think we shouldn't print the warning here always, because we use this function just for checking the version at some places, and then it shouldn't print. Maybe we could model that with an explicit error type that specifies the result (tool not found, generic error, version doesn't match), and print a warning only in places where it makes sense.

[Shunpoco]

It makes sense to me, thanks. But extra_checks/mod.rs already has such an Error enum. Do you think I can make a very similar one on lib.rs, or can I move ensure_version / ensure_version_or_cargo_install to extra_checks/ since they are only used from extra_checks for now?

[Shunpoco]

I tried to move them into extra_checks/mod.rs in dfe7d8a. Please check if the change is what you expected 🙏

[lolbinarycat]

A few nitpicks, but also, I don't feel the best about how many untested and rarely used code paths this introduces, seems like those have some potential to bitrot.

View changes since this review

[Shunpoco]

@lolbinarycat Thanks for the review, I addressed your feedback in 68ea14a.

I don't feel the best about how many untested and rarely used code paths this introduces, seems like those have some potential to bitrot.

I understand your concern. We're currently not sure if if-installed will be used frequently, although I believe that if-installed (especially for spellcheck) helps to improve code quality without breaking productivity. And my changes are tested just on my local but there is no test code for them. I will write some tests code, although so far I haven't come up with how to write a portable/meaningful test for a function which calls external tools like has_spellcheck().

[lolbinarycat]

although I believe that if-installed (especially for spellcheck) helps to improve code quality without breaking productivity.

that's the issue though, how often are people going to use if-installed:shellcheck and similar?

Personally I would say move ahead with this and try to implement more robust testing for tidy in a followup, but ultimately it's not my decision.

[Shunpoco]

Note: During writing a unit test, I found that parts.next() doesn't return None if the given s is empty string (""). It returns an empty string instead.

[Kobzol]

This looks good! I fully entrust lolbinarycat to have the final say, as they wrote the parsing code originally, but it's r+ from me.

r? @lolbinarycat

[lolbinarycat]

Looks good, just some small nitpicks and a question about what OS you used to test this locally.

View changes since this review

[lolbinarycat]

we should check if-installed first, as a quick version check is faster than enumerating all the changed files, and by doing that first, we could potentially skip that enumeration.

[lolbinarycat]

Huh, never gave this much though since I'm not on windows... I guess it makes sense? Surprised we haven't gotten a bug report about this though. I'm assuming you're on windows? We definitely should test this on windows, if it hasn't already been.

[Shunpoco]

No, I'm using Linux so I completely overlooked this EXE_EXTENSION 😅
But I found that the value in EXE_EXTENSION depends on the platform, so it's safe.
On Linux, it is an empty str. On the other hand, on Windows it is "exe".

[lolbinarycat]

whoops, yeah that's not a new addition, sorry.

[lolbinarycat]

Suggested change

continue;

No longer needed.

[lolbinarycat]

Suggested change

	assert_eq!(ExtraCheckArg::from_str(s), expected);
	assert_eq!(ExtraCheckArg::from_str(s), Err(expected));

Instead of wrapping each test case in Err, you can just wrap them all here, which should hopefully make it easier to read. You can also do the same thing above with the Ok tests, though that's a bit less important due to the different formatting.

[Shunpoco]

I put all my changes for your review in 466c3a9
Update: I force pushed 0dfff23 since the commit contained my debug eprintln.