fix(linting): stop flagging the adjective "leftover" as a phrasal verb#3792
fix(linting): stop flagging the adjective "leftover" as a phrasal verb#3792AnayGarodia wants to merge 3 commits into
Conversation
hippietrail
left a comment
hippietrail
left a comment
There was a problem hiding this comment.
This fix does not fit the pattern where it was added.
| if noun_chars.eq_any_ignore_ascii_case_chars(&[ | ||
| &['g', 'a', 'l', 'l', 'o', 'n'], | ||
| &['d', 'r', 'a', 'g', 'o', 'n'], | ||
| &['l', 'e', 'f', 't', 'o', 'v', 'e', 'r'], |
There was a problem hiding this comment.
This is not appropriate. "Gallon" and "dragon" merely look like compound nouns that might be related to phrasal verbs but "leftover" is an actual compound noun that is related to an actual phrasal verb 'left over".
A different fix will have to be found. I highly recommend a data-based approach using as many actual mistakes and as many legit uses as you can find. AI coding agents so far seem to be terrible at this and only make up examples that fit their assumptions. I think they can't freely use Google, which is how I find examples and counterexamples when working on new linters.
|
you're right, that didn't fit. gallon/dragon are in the list because they're coincidental splits — "gall on"/"drag on" only look like verb+particle. "leftover" is a genuine one ("left over"), so hardcoding it was papering over the real reason. reworked it: dropped "leftover" from the list and added a contextual guard — when there's no usable previous word (punctuation before it) and the word is itself an adjective sitting right before a noun, it's attributive ("passive, leftover debris") rather than a phrasal verb written as a compound noun. scoped to the no-previous-word case so it doesn't touch "to backup individual apps" and friends, which the existing prev-word guards already handle. generalizes past the one word, no corpus snapshot changes. |
hippietrail
left a comment
hippietrail
left a comment
There was a problem hiding this comment.
More gotchas to look for I'm afraid. There are many subtleties in grammar checking due to words beloning in multiple POS categories.
| // is a previous word the adjective/determiner/preposition guards below decide. | ||
| if maybe_prev_tok.is_none() | ||
| && token.kind.is_adjective() | ||
| && maybe_next_tok.is_some_and(|t| t.kind.is_noun()) |
There was a problem hiding this comment.
Another common gotcha is that many words are both nouns and adjectives, so you would want to find examples where the first such is in both categories and where the second such is in both categories.
Then you'd want to test with the first token being an adjective and not also being a noun and the second maybe_next_tok being a noun and not also an adjective.
And then you want to test whether doing so doesn't rule out too many instances of the problem you were trying to fix.
This is the kind of lint that having a big set of real-world text files or markdown files works great with, by the way.
|
ok, redid this properly - dropped the earlier no-previous-word special case. the thing #3707 was actually missing: "passive, leftover debris" already has an adjective ("passive") sitting right before the word, which is exactly what the existing guard at line 168 checks for. but the comma made get_next_word_from_offset return None, so that guard never got to run. so rather than special-casing "leftover" i made it look back past the punctuation to find that preceding adjective/determiner. same guard you already had, just no longer blocked by a comma. kept it tight: the previous word (across punctuation) has to be an adjective or determiner, and the following word has to be a noun. so imperative errors still flag - "Backup your database before upgrading" -> "Back up ...", and "Run fast, backup often" stays flagged because "often" is an adverb, not a noun. on the multiple-POS point: this linter only iterates noun tokens, so the target word is always a noun - there is no adjective-that-is-not-a-noun case on that side. on the following-word side i checked both noun-only (debris, dinner rolls, database) and noun+adjective. for examples i went looking instead of inventing them. Writer's Digest and Longman for the attributive uses ("leftover paint", "three leftover dinner rolls"), and Linguix actually lists "back up" as a documented common mistake ("They backup the files" -> "back up"), plus grammar.com ("Please backup your work" -> "back up"). one thing i want to be upfront about: "Stay calm, backup files" gets suppressed now too. it is the same shape as "passive, leftover debris" - a coordinate adjective vs two imperative clauses split by a comma - and i do not think those can be told apart without a real parse. it is narrow (needs adjective, comma, the word, then a noun) and it trades against the more common attributive false positive, with no snapshot changes either way. if you would rather not make that trade i am happy to drop the approach. |
PhrasalVerbAsCompoundNoun split "leftover" into "left over" because it ends in the particle "over" and "left" is a verb. But "leftover" (as in "leftover debris") is an adjective, not a phrasal verb miswritten as a compound noun — the same false-positive shape already handled for "gallon" (gall on) and "dragon" (drag on). Add it to the known false-positive set and cover it with a test. Closes Automattic#3707.
… leftover
drop "leftover" from the known-false-positive list - unlike gallon/dragon
(coincidental verb+particle splits) it's a real adjective used attributively.
when there's no usable previous word (punctuation before it) and the word is
an adjective right before a noun, treat it as attributive ("passive, leftover
debris"). generalizes past the single word. refs Automattic#3707.
The Automattic#3707 case "passive, leftover debris" already has an adjective right before the word, which the existing attributive/determiner guard checks for. The comma made get_next_word_from_offset return None, so that guard never ran. Walk back across punctuation to find the modifier instead of special-casing the word. Kept narrow: the preceding word (past punctuation) must be an adjective or determiner and the following word must be a noun, so imperative errors still flag ("Backup your database ..." -> "Back up ...", "Run fast, backup often" stays flagged since "often" is not a noun). No corpus snapshot changes.
c1056bf to
3ed422c
Compare
What
Fixes #3707.
PhrasalVerbAsCompoundNounflagged the adjective "leftover" ("leftover debris") and suggested rewriting it to "left over".Why
"leftover" ends in the particle "over" and its prefix "left" is a verb, so it matches the rule's phrasal-verb shape. But used attributively it's an adjective, not a phrasal verb miswritten as a compound noun. This is the exact false-positive class the rule already handles for "gallon" ("gall on") and "dragon" ("drag on").
How
Add
leftoverto the existing known-false-positive set inanalyse, and update the accompanying comment. A blanket "skip adjectives" approach was rejected because "backup" is also tagged as an adjective, and skipping it would regress the rule's intendedbackup→back upcorrections (verified locally — 2 existing tests fail with that approach).Testing
Added
dont_flag_leftover_adjective_3707(reproduces the issue: 1 lint before, 0 after). Full rule suite green, no regressions:Compatibility
Purely narrows one false positive; no public API or output change for any other input.