Automattic / Automattic/harper

`OrthographicConsistency` wrongly flags the `It's` at the start of sentences

Open
#4,375 1 comment 0 reactions 0 assignees View on GitHub
bug false-positive harper-core linting
Dominant language
Rust
Stars
15.4k
Forks
627
Avg merge
1d 15h
Merged PRs (30d)
106

Description

This one is pretty tricky. I noticed that "it" has noun and proper noun POS annotation flags.
This is due to the "It" and "IT" entries. Both are abbreviations and are flagged as nouns. But "It" also has the proper noun flag because it's used for "Italy" and "Italian". But unlike "IT", it's never used with this meaning in mid-sentence so I went to remove it and leaving those entries like this:
```
IT/Ng4 # removed `5`. nouns can qualify nouns. makes `it` an adjective which interferes with heuristics
```
```
It/4 # abbreviation for Italy/Italian. "biological force", "Id", and "desirable quality" senses too marginal
```

However, doing so caused two previously passing tests to fail:
- `its_not_perfect_keeps_apostrophe()` in `linting/lint_group/mod.rs`
- `lint_group::lint_descriptions_are_clean`when it got to `LetsConfusion`.

The `description` of `LetsConfusion` is
> 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.

And the test only checks the number of lints, which was 0 when "It" had the noun and proper noun annotations but is 1 without them.

Digging deeper it turned out to be `OrthographicConsistency` that began to be triggered after this seemingly unrelated change.

It turns out to be in this code in `match_to_lint_with_context()`
```rs
let canonical_flags = metadata.orth_info;
let flags_to_check = [
OrthFlags::LOWER_CAMEL,
OrthFlags::UPPER_CAMEL,
OrthFlags::APOSTROPHE,
OrthFlags::HYPHENATED,
];

if flags_to_check
.into_iter()
.filter(|flag| canonical_flags.contains(*flag) != cur_flags.contains(*flag))
.count()
== 1
&& let Some(canonical) = self.dict.get_correct_capitalization_of(chars)
&& alphabetic_differs(canonical, chars)
{
return Some(Lint {
span: word.span,
lint_kind: LintKind::Capitalization,
suggestions: vec![Suggestion::ReplaceWith(canonical.to_vec())],
message: format!(
"The canonical dictionary spelling is `{}`.",
canonical.iter().collect::()
),
priority: 31,
});
}
```
For some reason it could accept `It's` as the first word in the sentence when `It` was a proper noun, but not when it's a common noun.

I'm still trying to understand the code well enough to work out the proper solution. In the meantime I document it here for catharsis and in case anyone else takes an interest. Maybe @elijah-potter remembers the code well enough?

One problem is that since the dictionary is case-folded, the orth info is a superposition of the entries. This is why a word in the dictionary more than once after taking into account case folding doesn't really have a canonical orthography. If you look up the orth info for any of "it's", "It's", or "IT's" after the dictionary changes mentioned above, you will find `"orth_info": "LOWERCASE | UPPER_CAMEL | APOSTROPHE"`.

Contributor guide

Open the contributing guide

Research direction

Start in match_to_lint_with_context() at the OrthographicConsistency capitalization branch and inspect how case-folded orth_info is combined for `it's`, `It's`, and `IT's`. Run its_not_perfect_keeps_apostrophe() and lint_group::lint_descriptions_are_clean, especially the LetsConfusion description. Done means the sentence-initial `It's` no longer gets a false capitalization lint while both existing tests continue to pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
devtools
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.