Skip to content

fix(core): don't flag MissingTo after a possessive nominal - #4089

Open
mauropereiira wants to merge 1 commit into
Automattic:masterfrom
mauropereiira:fix/missing-to-possessive-noun
Open

fix(core): don't flag MissingTo after a possessive nominal#4089
mauropereiira wants to merge 1 commit into
Automattic:masterfrom
mauropereiira:fix/missing-to-possessive-noun

Conversation

@mauropereiira

Copy link
Copy Markdown
Contributor

Issues

Fixes #3951

Description

MissingTo flagged "This Article's aims are both theoretical and historical.", wanting an infinitive after "aims".

The cause is a chain of defaults rather than one bug:

  • "aims" is in the controller list.
  • Its dictionary entry supports both noun and verb readings.
  • The tagger defaults the ambiguous token to UPOS::VERB.
  • "are" carries verb metadata without explicit verb-form flags, so the lemma check passes.

So the pattern accepts "aims are", and nothing in the rule looked left. As the reporter put it, the rule pattern-matches the token without resolving its part of speech from the frame it sits in: X's aims are, not X aims to.

The guard is structural. A noun-capable controller immediately preceded by a possessive nominal is the head of a noun phrase, not a predicate missing an infinitive:

if controller.kind.is_noun()
    && preceded_by_word(context, |tok| tok.kind.is_possessive_nominal())
{
    return None;
}

This covers possessive nouns like "Article's" as well as possessive determiners, without keying on any specific word.

I also considered guarding on the following token being a finite verb, and rejected it: Harper cannot generally separate a finite base form from an infinitive using UPOS alone, so it would have suppressed legitimate missing-to cases before infinitive auxiliaries such as "be".

Demo

This Article's aims are both theoretical and historical.   clean     (was MissingTo)

She wants finish early.                                    flagged   (unchanged)
We need talk about pricing.                                flagged   (unchanged)

How Has This Been Tested?

cargo test -p harper-core

test result: ok. 6130 passed; 0 failed; 290 ignored; 0 measured; 0 filtered out

Each existing positive in missing_to.rs was checked against the guard; none has a preceding possessive nominal, so all remain eligible. No files under harper-core/tests/text/ change.

AI Disclosure

  • I used an AI agent interactively.

If Your PR Implements or Enhances a Linter

  • I'm using examples from the bug report / feature request.

Checklist

  • I have performed a self-review of my own code
  • I have added tests to cover my changes
  • I have considered splitting this into smaller pull requests.

`MissingTo` read "aims" in "This Article's aims are ..." as a verb wanting
an infinitive, because the token is noun/verb ambiguous and the tagger
defaults it to VERB. A controller preceded by a possessive is a noun in a
noun phrase, not a predicate.

Suppress noun-capable controllers immediately preceded by a possessive
nominal. The existing positives have no preceding possessive and are
unaffected.

Fixes Automattic#3951

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hippietrail

hippietrail commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

The cause is a chain of defaults rather than one bug:

  • "aims" is in the controller list.
  • Its dictionary entry supports both noun and verb readings.
  • The tagger defaults the ambiguous token to UPOS::VERB.

Oh this is the kind of thing that made me never comfortable with using UPOS. In the linters I write I always try to work out which POS is likely, or document when I can't. This is of course also not ideal.
I think at one point I wanted so sus out how it worked and it failed on the "the cat sat on the mat" test. I was also never sure whether it used the POSes from the dictionary or was completely external. I also never looked at its code to try to grok either its exact intent or logic.

But perhaps we should start a new issue specifically gathering places like this where it fails so that it can be improved at some point?

  • "are" carries verb metadata without explicit verb-form flags, so the lemma check passes.

Yes "is" is unique in that it has more forms than all other English verbs and its agreement rules are more involved. At some point we might want to have a good think about whether we want a special Be or ToBe module along the lines of IrregularVerbs or such as a centralized place for logic applying to it and its forms that can be used in any linter. Alternatively we could add methods on the low-level DictWordMetadata or higher level TokenKind modules to add new queryable properties that don't rely on annotation flags that would only be applied to a tiny handful of entries...

Apologies for not diving deep into the rest of this PR. Just adding some thoughts for now as there's a ton of new issues and PRs to look at...

@mauropereiira

Copy link
Copy Markdown
Contributor Author

No worries on the review, and thanks for the thoughts. Three of these I could answer from the code since I had it open.

Does it still fail "the cat sat on the mat"? Yes, on the last word:

the cat sat on the mat     DET NOUN VERB ADP DET DET
the cat sat on the rug     DET NOUN VERB ADP DET NOUN

It looks lexical rather than contextual. mat comes back DET in every frame I tried, including with a trailing period and with a cat or the dog in front. harper-cli metadata mat reports NVJ with determiner: null, so the tagger is producing a tag the word's own dictionary entry does not license.

Dictionary POS or external? External, and it wins. From Document::parse:

let mut found_meta = dictionary
    .get_word_metadata(word_source)
    .map(|c| c.into_owned());

if let Some(inner) = &mut found_meta {
    inner.pos_tag = token_tags[ti].or_else(|| inner.infer_pos_tag());
    inner.np_member = Some(np_flags[ti]);
}

The dictionary lookup runs first, then token_tags from BrillTagger<FreqDict> overwrites pos_tag. infer_pos_tag() only gets a say when the tagger returns None. The tagger is trained separately and loaded from trained_tagger_model.json, and does not consult the curated dictionary. So mat keeps its noun metadata while its pos_tag says DET, and nothing reconciles the two.

On a Be or ToBe module: patterns::InflectionOfBe already exists and matches be, am, is, are, was, were, been, being. 13 files under harper-core/src/linting reference it. It is a plain WordSet with no agreement or form logic behind it, so it answers "is this a form of be" and nothing more. If the goal is the agreement rules you mentioned, extending that looks cheaper than a new module.

On opening an issue to gather UPOS failures: worth doing, and I think part of it can be mechanized. Tagger versus dictionary disagreement is checkable in bulk. I ran 400 random dictionary words through the fixed frame The cat sat on the ___.. 142 came back untagged, and 11 got a tag their own entry does not license:

assigned   tagger=NOUN   dictionary: verb+adjective
pegged     tagger=NOUN   dictionary: verb+adjective
capped     tagger=NOUN   dictionary: verb+adjective
said       tagger=NOUN   dictionary: verb+adjective
exclude    tagger=NOUN   dictionary: verb
ideate     tagger=NOUN   dictionary: verb
impart     tagger=NOUN   dictionary: verb
simulate   tagger=NOUN   dictionary: verb
shoo       tagger=NOUN   dictionary: verb+pronoun
nat        tagger=ADJ    dictionary: noun
uptown     tagger=ADV    dictionary: noun+adjective

The disagreement cuts both ways, though. In a the ___ slot NOUN is the syntactically expected tag, so most of those rows probably mean the dictionary is missing a noun reading rather than the tagger being wrong. The ones that look like real tagger errors to me are nat, uptown, and mat. Either way the signal is cheap to collect, and it would give that issue real entries rather than ad hoc ones.

Happy to open it with this as the starting batch if you want it.

@hippietrail

Copy link
Copy Markdown
Collaborator

Thanks for the summary on how the Brill tagger works. Interesting.

On a Be or ToBe module: patterns::InflectionOfBe already exists and matches be, am, is, are, was, were, been, being. 13 files under harper-core/src/linting reference it. It is a plain WordSet with no agreement or form logic behind it, so it answers "is this a form of be" and nothing more. If the goal is the agreement rules you mentioned, extending that looks cheaper than a new module.

Yeah that's a Pattern/Expr module, I was talking about something that knows about morphology, agreement, tense, mood, number, transformations for "to be" rather than about pattern matching inflections. Like a much-expanded equivalent of the irregular verbs and plural modules. More like "do stuff with be" than "recognize forms of be".

On opening an issue to gather UPOS failures: worth doing, and I think part of it can be mechanized. Tagger versus dictionary disagreement is checkable in bulk. I ran 400 random dictionary words through the fixed frame The cat sat on the ___.. 142 came back untagged, and 11 got a tag their own entry does not license:

assigned   tagger=NOUN   dictionary: verb+adjective
pegged     tagger=NOUN   dictionary: verb+adjective
capped     tagger=NOUN   dictionary: verb+adjective
said       tagger=NOUN   dictionary: verb+adjective
exclude    tagger=NOUN   dictionary: verb
ideate     tagger=NOUN   dictionary: verb
impart     tagger=NOUN   dictionary: verb
simulate   tagger=NOUN   dictionary: verb
shoo       tagger=NOUN   dictionary: verb+pronoun
nat        tagger=ADJ    dictionary: noun
uptown     tagger=ADV    dictionary: noun+adjective

The disagreement cuts both ways, though. In a the ___ slot NOUN is the syntactically expected tag, so most of those rows probably mean the dictionary is missing a noun reading rather than the tagger being wrong. The ones that look like real tagger errors to me are nat, uptown, and mat. Either way the signal is cheap to collect, and it would give that issue real entries rather than ad hoc ones.

Interesting. I also did the "cat sat on the mat" test with .is_np_member() - I can't remember if it was around the same time or not. I was working on an Expr to match noun phrases structurally. We did have one, and maybe still do, that matched any sequence of adjective/noun/determiner in any order, I think. The hard parts about making a structural one came with words having both a noun and adjective POS and noun qualifiers being singular always but heads singular or plural, and the fact that this rule is often broken in the kind of text that needs grammar checking.

@mauropereiira

Copy link
Copy Markdown
Contributor Author

You're right, I answered the wrong question. InflectionOfBe recognises the forms and you asked for something that does things with them. My mistake.

The model you named is already sitting there in the shape you'd want, though. IrregularVerbs is JSON-backed with get_pasts_for_lemma, get_lemma_for_preterite and get_past_participle_for_preterite, and IrregularNouns is its sibling. Neither of them matches anything, they answer questions.

The gap a Be module would fill is measurable. Thirteen linters carry be-specific logic today:

be_adjective_confusions, be_allowed, far_be_it, i_am_agreement, it_would_be, modal_be_adjective, ought_to_be, progressive_needs_be, pronoun_inflection_be, pronoun_verb_agreement, single_be, soon_to_be, there_is_agreement

5,380 lines including their tests. pronoun_inflection_be alone holds nine expressions in an ExprMap, and six of them exist to encode that a third person singular subject takes "is" and a plural one takes "are". The contracted forms get their own copies, and sentence-initial position gets copies again, so the same agreement fact is written out four times.

On NominalPhrase. Still there, harper-core/src/patterns/nominal_phrase.rs, and it's the one you remember: a loop over determiners, adjectives and progressive verbs that stops at the first nominal. Ten linters still use it. Its own doc comment says "it is not recommended for new code. Please prefer DictWordMetadata::np_member."

Both hard parts you named are already recorded there, as skipped tests in take_serious.rs:

#[ignore = "'This' and 'that', which can be determiners and pronouns, are not handled properly by `NominalPhrase`"]
#[ignore = "'No one' is not handled properly by `NominalPhrase`"]

On the mat test with is_np_member. It fails, and on the same word:

the cat sat on the mat    the=true  cat=true  sat=false  on=false  the=true  mat=false
the cat sat on the rug    the=true  cat=true  sat=false  on=false  the=true  rug=true

So "the mat" comes out as a determiner with nothing attached to it.

I don't think that's a second bug, though. In Document::parse the chunker is handed the tagger's output:

let token_tags = tagger.tag_sentence(&token_strings);
let np_flags = chunker.chunk_sentence(&token_strings, &token_tags);

burn_chunker() is a separate model from BrillTagger, but it reads the tags. So mat arrives at the chunker already tagged DET, and refusing to put it in a noun phrase is the consistent thing to do with that input. Which also puts np_member downstream of the tagger and dictionary disagreements I listed last time, rather than beside them.

The structural pattern doesn't reproduce the split. NominalPhrase gives the same spans for both:

the cat sat on the mat  →  ["the cat sat", "on the mat"]
the cat sat on the rug  →  ["the cat sat", "on the rug"]

Stable across the pair, but it reads "cat" as a modifier and "sat" as the head, so I don't think it would serve as a fallback here. ("A red apple" still comes out right, for what it's worth.)

Opened the tagger issue as #4150. I regenerated the sweep against current master rather than reusing last week's batch, and the mat row and this chunker note are both in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WordChoice rule (MissingTo) fires without part-of-speech context

2 participants