Merge pull request #1085 from hippietrail/let_lets_let_us_improvements

feat: handle some false positives in let/lets/let's/let us

Author: elijah-potter

harper-core/dictionary.dict

@@ -30577,7 +30577,7 @@ juryman/1M
 jurymen/9
 jurywoman/1M
 jurywomen/9
-just/~514RYPT
+just/~54RYPT        # removed `1` noun. archaic and interferes with linter heuristics
 justice/~1IMS
 justifiable/~5U
 justifiably/Uj
@@ -42132,7 +42132,7 @@ rumourmonger/14SM!@
 rump/~14MYS
 rumple/41DSMG
 rumpus/1MS
-run/~415AM
+run/~41AMS          # removed `5` adj. a bit marginal and interferes with linter heuristics
 runabout/1MS
 runaround/1SM
 runaway/~15MS
@@ -42147,7 +42147,6 @@ runner/~1SM
 running/~451+M
 runny/5RT
 runoff/~1SM
-runs/~4
 runt/1MS
 runtime/~51
 runty/5RT

harper-core/src/linting/compound_nouns/general_compound_nouns.rs

@@ -11,15 +11,20 @@ use crate::{
     patterns::{Pattern, SequencePattern},
 };
 
-/// Covers the general cases of accidentally split compound nouns.
+/// Two adjacent words separated by whitespace that if joined would be a valid noun.
 pub struct GeneralCompoundNouns {
     pattern: Box<dyn Pattern>,
     split_pattern: Lrc<SplitCompoundWord>,
 }
 
+// This heuristic identifies potential compound nouns by:
+// 1. Looking for a determiner or adjective (e.g., "a", "big", "red")
+// 2. Followed by two content words (not determiners, adverbs, or prepositions)
+// 3. Finally, checking if the combination forms a noun in the dictionary
+//    that is not also an adjective
 impl Default for GeneralCompoundNouns {
     fn default() -> Self {
-        let exceptions_pattern = SequencePattern::default()
+        let context_pattern = SequencePattern::default()
             .then(|tok: &Token, _: &[char]| {
                 let Some(Some(meta)) = tok.kind.as_word() else {
                     return false;
@@ -44,22 +49,22 @@ impl Default for GeneralCompoundNouns {
                 tok.span.len() > 1 && !meta.determiner && !meta.is_adverb() && !meta.preposition
             });
 
-        let split_pattern = Lrc::new(SplitCompoundWord::new(|meta| {
-            meta.is_nominal() && !meta.is_adjective()
+        let compound_pattern = Lrc::new(SplitCompoundWord::new(|meta| {
+            meta.is_noun() && !meta.is_adjective()
         }));
 
         let mut pattern = All::default();
-        pattern.add(Box::new(exceptions_pattern));
+        pattern.add(Box::new(context_pattern));
         pattern.add(Box::new(
             SequencePattern::default()
                 .then_anything()
                 .then_anything()
-                .then(split_pattern.clone()),
+                .then(compound_pattern.clone()),
         ));
 
         Self {
             pattern: Box::new(pattern),
-            split_pattern,
+            split_pattern: compound_pattern,
         }
     }
 }

harper-core/src/linting/compound_nouns/implied_ownership_compound_nouns.rs

@@ -11,6 +11,11 @@ use crate::{
 
 /// Looks for closed compound nouns which can be condensed due to their position after a
 /// possessive noun (which implies ownership).
+/// See also:
+/// harper-core/src/linting/lets_confusion/mod.rs
+/// harper-core/src/linting/lets_confusion/let_us_redundancy.rs
+/// harper-core/src/linting/lets_confusion/no_contraction_with_verb.rs
+/// harper-core/src/linting/pronoun_contraction/should_contract.rs
 pub struct ImpliedOwnershipCompoundNouns {
     pattern: Box<dyn Pattern>,
     split_pattern: Lrc<SplitCompoundWord>,
@@ -38,9 +43,14 @@ impl PatternLinter for ImpliedOwnershipCompoundNouns {
 
     fn match_to_lint(&self, matched_tokens: &[Token], source: &[char]) -> Option<Lint> {
         // "Let's" can technically be a possessive noun (of a lease, or a let in tennis, etc.)
-        // but in practice it's almost always a contraction of "let us" before a verb.
-        let possessive = matched_tokens[0].span.get_content(source);
-        if possessive == ['l', 'e', 't', '\'', 's'] || possessive == ['L', 'e', 't', '\'', 's'] {
+        // but in practice it's almost always a contraction of "let us" before a verb
+        // or a mistake for "lets", the 3rd person singular present form of "to let".
+        let word_apostrophe_s = matched_tokens[0].span.get_content(source);
+        if word_apostrophe_s
+            .iter()
+            .map(|&c| c.to_ascii_lowercase())
+            .eq(['l', 'e', 't', '\'', 's'].iter().copied())
+        {
             return None;
         }
         let span = matched_tokens[2..].span()?;

harper-core/src/linting/lets_confusion/let_us_redundancy.rs

@@ -5,6 +5,11 @@ use crate::{
 
 use crate::linting::{Lint, LintKind, PatternLinter, Suggestion};
 
+/// See also:
+/// harper-core/src/linting/compound_nouns/implied_ownership_compound_nouns.rs
+/// harper-core/src/linting/lets_confusion/mod.rs
+/// harper-core/src/linting/lets_confusion/no_contraction_with_verb.rs
+/// harper-core/src/linting/pronoun_contraction/should_contract.rs
 pub struct LetUsRedundancy {
     pattern: Box<dyn Pattern>,
 }

harper-core/src/linting/lets_confusion/mod.rs

@@ -5,6 +5,11 @@ use super::merge_linters::merge_linters;
 use let_us_redundancy::LetUsRedundancy;
 use no_contraction_with_verb::NoContractionWithVerb;
 
+// See also:
+// harper-core/src/linting/compound_nouns/implied_ownership_compound_nouns.rs
+// harper-core/src/linting/lets_confusion/let_us_redundancy.rs
+// harper-core/src/linting/lets_confusion/no_contraction_with_verb.rs
+// harper-core/src/linting/pronoun_contraction/should_contract.rs
 merge_linters!(LetsConfusion => LetUsRedundancy, NoContractionWithVerb => "It's often hard to determine where the subject should go with the word `let`. This rule attempts to find common errors with redundancy and contractions that may lead to confusion for readers.");
 
 #[cfg(test)]
@@ -41,14 +46,25 @@ mod tests {
         );
     }
 
+    // "play" is also a noun so in a context like "Sometimes the umpire lets play continue"
+    // #[test]
+    // fn issue_470_missing_apostrophe() {
+    //     assert_suggestion_result("lets play", LetsConfusion::default(), "let's play");
+    // }
+
+    // #[test]
+    // fn issue_470_missing_subject() {
+    //     assert_suggestion_result("let play", LetsConfusion::default(), "let's play");
+    // }
+
     #[test]
     fn issue_470_missing_apostrophe() {
-        assert_suggestion_result("lets play", LetsConfusion::default(), "let's play");
+        assert_suggestion_result("lets proceed", LetsConfusion::default(), "let's proceed");
     }
 
     #[test]
     fn issue_470_missing_subject() {
-        assert_suggestion_result("let play", LetsConfusion::default(), "let's play");
+        assert_suggestion_result("let proceed", LetsConfusion::default(), "let's proceed");
     }
 
     #[test]

harper-core/src/linting/lets_confusion/no_contraction_with_verb.rs

@@ -1,24 +1,69 @@
 use crate::{
     Token,
     linting::{Lint, LintKind, Suggestion},
-    patterns::{Pattern, SequencePattern, WordSet},
+    patterns::{EitherPattern, Pattern, SequencePattern, WordSet},
 };
 
 use crate::linting::PatternLinter;
 
+/// See also:
+/// harper-core/src/linting/compound_nouns/implied_ownership_compound_nouns.rs
+/// harper-core/src/linting/lets_confusion/mod.rs
+/// harper-core/src/linting/lets_confusion/let_us_redundancy.rs
+/// harper-core/src/linting/pronoun_contraction/should_contract.rs
 pub struct NoContractionWithVerb {
     pattern: Box<dyn Pattern>,
 }
 
 impl Default for NoContractionWithVerb {
     fn default() -> Self {
-        let pattern = SequencePattern::default()
+        let let_ws = SequencePattern::default()
             .then(WordSet::new(&["lets", "let"]))
+            .then_whitespace();
+
+        // Word is only a verb, and not the gerund/present participle form.
+        let non_ing_verb = SequencePattern::default().then(|tok: &Token, source: &[char]| {
+            let Some(Some(meta)) = tok.kind.as_word() else {
+                return false;
+            };
+
+            if !meta.is_verb() || meta.is_noun() || meta.is_adjective() {
+                return false;
+            }
+
+            let tok_chars = tok.span.get_content(source);
+
+            // If it ends with 'ing' and is at least 5 chars long, it could be a gerund or past participle
+            // TODO: replace with metadata check when affix system supports verb forms
+            if tok_chars.len() < 5 {
+                return true;
+            }
+
+            let is_ing_form = tok_chars
+                .iter()
+                .skip(tok_chars.len() - 3)
+                .map(|&c| c.to_ascii_lowercase())
+                .collect::<Vec<_>>()
+                .ends_with(&['i', 'n', 'g']);
+
+            !is_ing_form
+        });
+
+        // Ambiguous word is a verb determined by heuristic of following word's part of speech
+        let verb_due_to_following_pos = SequencePattern::default()
+            .then(|tok: &Token, _source: &[char]| tok.kind.is_verb() || tok.kind.is_noun())
             .then_whitespace()
-            .then_verb();
+            .then(|tok: &Token, _source: &[char]| {
+                tok.kind.is_determiner() || tok.kind.is_pronoun() || tok.kind.is_conjunction()
+            });
+
+        let let_then_verb = let_ws.then(EitherPattern::new(vec![
+            Box::new(non_ing_verb),
+            Box::new(verb_due_to_following_pos),
+        ]));
 
         Self {
-            pattern: Box::new(pattern),
+            pattern: Box::new(let_then_verb),
         }
     }
 }
@@ -39,12 +84,116 @@ impl PatternLinter for NoContractionWithVerb {
                 Suggestion::replace_with_match_case_str("let's", template),
                 Suggestion::replace_with_match_case_str("let us", template),
             ],
-            message: "It seems you forgot to include a subject here.".to_owned(),
+            message: "To suggest an action, use 'let's' or 'let us'.".to_owned(),
             priority: 31,
         })
     }
 
     fn description(&self) -> &'static str {
-        "Make sure you include a subject when giving permission to it."
+        "Checks for `lets` meaning `permits` when the context is about suggesting an action."
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::NoContractionWithVerb;
+    use crate::linting::tests::{assert_lint_count, assert_suggestion_result};
+
+    // Corrections
+
+    #[test]
+    fn fix_lets_inspect() {
+        assert_suggestion_result(
+            "In the end lets inspect with git-blame the results.",
+            NoContractionWithVerb::default(),
+            "In the end let's inspect with git-blame the results.",
+        );
+    }
+
+    // False positives where verb is also a noun
+
+    #[test]
+    fn dont_flag_let_chance() {
+        assert_lint_count("Let chance decide", NoContractionWithVerb::default(), 0);
+    }
+
+    #[test]
+    fn dont_flag_let_time() {
+        assert_lint_count(
+            "Let time granularity be parametrized",
+            NoContractionWithVerb::default(),
+            0,
+        );
+    }
+
+    #[test]
+    fn dont_flag_lets_staff() {
+        assert_lint_count(
+            "A plugin that backs up player's inventories and lets staff restore them or export it as a shulker.",
+            NoContractionWithVerb::default(),
+            0,
+        );
+    }
+
+    #[test]
+    fn dont_flag_lets_time() {
+        assert_lint_count(
+            "This is very different than demo recording, which just simulates a network level connection and lets time move at its own rate.",
+            NoContractionWithVerb::default(),
+            0,
+        );
+    }
+
+    #[test]
+    fn dont_flag_lets_play() {
+        assert_lint_count(
+            "Sometimes the umpire lets play continue",
+            NoContractionWithVerb::default(),
+            0,
+        );
+    }
+
+    // False positives where verb is a gerund/past participle
+
+    #[test]
+    fn dont_flag_let_sleeping() {
+        assert_lint_count(
+            "Let sleeping logs lie.",
+            NoContractionWithVerb::default(),
+            0,
+        );
+    }
+
+    // False positives where verb is also an adjective
+
+    #[test]
+    fn dont_flag_let_processed() {
+        assert_lint_count(
+            "Let processed response be a new structure analogous to server auction response.",
+            NoContractionWithVerb::default(),
+            0,
+        );
+    }
+
+    // Disambiguated noun/verb by following determiner
+
+    #[test]
+    fn corrects_lets_make_this() {
+        assert_suggestion_result(
+            "Lets make this joke repo into one of the best.",
+            NoContractionWithVerb::default(),
+            "Let's make this joke repo into one of the best.",
+        );
+    }
+
+    // Disambiguated verb by following pronoun
+
+    #[test]
+    fn corrects_lets_mock_them() {
+        assert_suggestion_result(
+            "Then lets mock them using Module._load based mocker.",
+            NoContractionWithVerb::default(),
+            "Then let's mock them using Module._load based mocker.",
+        );
     }
 }

harper-core/src/linting/pronoun_contraction/should_contract.rs

@@ -6,6 +6,11 @@ use crate::{
 use crate::Lint;
 use crate::linting::{LintKind, PatternLinter, Suggestion};
 
+/// See also:
+/// harper-core/src/linting/compound_nouns/implied_ownership_compound_nouns.rs
+/// harper-core/src/linting/lets_confusion/mod.rs
+/// harper-core/src/linting/lets_confusion/let_us_redundancy.rs
+/// harper-core/src/linting/lets_confusion/no_contraction_with_verb.rs
 pub struct ShouldContract {
     pattern: Box<dyn Pattern>,
 }