refactor(harper-comments): migrate Unit, Lua, and Solidity onto LineWise [WIP - Do Not Merge] - #3909
Draft
rodbegbie wants to merge 7 commits into
Draft
Conversation
Adds a new harper-yaml crate, following the same standalone pattern as harper-python and harper-html rather than folding into harper-comments. YamlParser lints `#` comments (with the usual spellchecker-ignore-family suppression) and prose-like scalar values (plain, quoted, or block scalars), while leaving structural YAML (keys, identifiers, enum-like values) untouched. The core pieces: - YamlMasker runs two independent tree-sitter passes over the parsed document -- one for comment nodes, one for scalar-value nodes -- and combines the surviving spans. Scalar nodes exclude mapping keys (unquoted keys are the same tree-sitter node kind as values, so a key-vs-value distinction has to be made explicitly by comparing a candidate node's start position against its enclosing mapping pair's start position). - heuristics::is_prose_scalar decides whether a scalar value looks like prose worth checking, as opposed to structural config data: it requires 3+ words and rejects values that are, in their entirety, a single URL/path/version-shaped token (so "v1.2.3" is skipped, but "upgrade to version 1.2.3" is still checked). - DedentLines wraps the inner PlainEnglish parser to parse each line of a scalar independently after trimming its whitespace, avoiding a spurious "double space" false positive that raw multi-line block scalar indentation would otherwise trigger. Closes Automattic#2700. Entire-Checkpoint: 8eae5ee14317
harper-ls now routes the "yaml" language ID directly to YamlParser, the same way it already routes "python" to PythonParser. harper-cli routes .yaml/.yml files the same way it routes .py/.pyi. Entire-Checkpoint: 173be144ecc2
- New fuzz_harper_yaml target, mirroring fuzz_harper_html. - VS Code plugin: register onLanguage:yaml activation and add a YAML case to the language integration test suite. - Document YAML in the language-server support table. Marked "Comments Only: no" (unlike TOML), since YamlParser also lints prose-like scalar values, not just comments. Entire-Checkpoint: 3d4124766057
Address review findings on the YAML support crate: - Remove unreachable dead code in `is_prose_scalar`: under the `word_count >= 3` gate, `looks_url_path_or_version` (bails at >1 word) and `is_snake_or_kebab_case` (bails on whitespace) could never fire. Behaviour is unchanged - the word-count gate already subsumes every single-word URL/path/version/identifier case - but the code and its helpers are gone, so tests no longer imply filtering that isn't there. - Strip YAML block-scalar indicators (`|`, `>`, `|-`, `>+2`, ...) before the word-count gate so a short block-scalar body isn't pushed over the prose threshold by the leading indicator token. - Replace `is_mapping_key`'s byte-position heuristic with tree-sitter's named `key` field, so explicit-key syntax (`? key` / `: value`) no longer misclassifies the key as a lintable value. - Coalesce overlapping spans before building the `Mask`, so adversarial input cannot trigger the `FromIterator` overlap panic (a DoS in harper-ls). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 6c181a01b5d3
…combinator DedentLines duplicated a split-by-line/strip/parse/stitch pattern already present three times in harper-comments (Unit, Lua, Solidity). Pull the shared mechanism into harper_core::parsers::LineWise, parameterized by a per-line strip function, so DedentLines becomes a thin wrapper supplying just the YAML-specific whitespace-trim policy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Entire-Checkpoint: 88a3dc81119c
`just check-rust` runs `cargo clippy -- -Dwarnings ...` against whatever Rust "stable" currently resolves to. A recent stable release added new lints (useless_borrows_in_formatting, question_mark) and promoted float_literal_f32_fallback from a future-incompat warning towards a hard error, which broke previously-clean code across harper-core, harper-cli, and harper-desktop with no code change on their part. This has been failing on master's own CI since at least 2026-07-05 and was blocking CI on this branch too (rebased onto upstream/master, which already picked up a separate fix for 3 more issues of the same kind in harper-desktop's mac_broker module - see 3f46f06). Also drops an unused `Config` import in harper-desktop's mac_broker module, left over from upstream's 607d0f8 (2026-07-20) which replaced its only two uses with `Integration` but didn't clean up the import. Same "unblock CI, not our diff" rationale as the rest of this commit. Unrelated to the YAML work in this branch; bundled here only because it was blocking this PR's CI and nothing else had fixed it yet upstream. Verified against the exact CI command plus `cargo hack check --each-feature` and the full test suite. Entire-Checkpoint: 902cc73d5bab
Follow-up to 93075ce. Unit, Lua, and Solidity each hand-rolled the same split-by-line/strip/parse/stitch loop that LineWise now centralizes, but none of them fit the plain trim-a-span contract: - Unit swallows lines inside fenced code blocks entirely (no tokens, no separator), tracked via a toggle that must persist across lines within one parse() call. - Lua turns an "@tag" line into a paragraph break (Newline(2)) instead of parsing it. - Solidity's SPDX-License-Identifier special case turned out to be unreachable dead code: it searches for a '\n' terminator within a span that has already been split on '\n', so it always falls through to swallowing the whole line. Preserved that observed behavior (line contributes no tokens) without carrying the dead branch forward. Extended LineWise's strip contract from a bare Span<char> to a small LineClass { span, newline: Option<usize> } struct so the per-line classifier can also control the separator: suppress it (skip_silently, for Unit's fences), reweight it (skip_with_newline, for Lua's @-lines), or leave it as the default single Newline (skip/parse, matching DedentLines' existing behavior unchanged). One intentional, test-uncovered normalization: Lua previously emitted a dangling Newline(2) after an "@"-line even when it was the last line in the source, with nothing following it. LineWise now gates all separators uniformly on "is there a next line", so that dangling token is no longer emitted. No fixture exercises this edge case either way. All 40 harper-comments language_support fixtures keep their exact lint counts; full workspace test suite and clippy (-D warnings) pass clean on the touched files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rodbegbie
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft / work in progress — depends on #3757 landing first.
Issues
Description
Follow-up to the
LineWisecombinator introduced in #3757 (see this review discussion), which deliberately scopedLineWisetoharper-yamlonly and leftUnit,Lua, andSolidityinharper-commentsas a follow-up, since they share the same split-by-line/strip/parse/stitch shape but each have quirks that don't fit the plain trim-a-span contract:parse()call.@tagline into a paragraph break (Newline(2)) instead of parsing it.\nterminator within a span that has already been split on\n, so it always falls through to swallowing the whole line. Preserved that observed behavior (line contributes no tokens) without carrying the dead branch forward.To support these cases,
LineWise's strip contract is extended from a bareSpan<char>to a smallLineClass { span, newline: Option<usize> }struct, so the per-line classifier can also control the separator: suppress it (skip_silently, for Unit's fences), reweight it (skip_with_newline, for Lua's@-lines), or leave it as the default singleNewline(matchingDedentLines' existing behavior unchanged).One intentional, test-uncovered normalization: Lua previously emitted a dangling
Newline(2)after an@-line even when it was the last line in the source, with nothing following it.LineWisenow gates all separators uniformly on "is there a next line", so that dangling token is no longer emitted. No fixture exercises this edge case either way.This branch is currently rebased on top of #3757 and will need re-rebasing onto
masteronce that PR lands (and shrink to just this one commit's diff at that point).Demo
N/A — internal parser refactor, no user-visible behavior change beyond the one noted normalization above.
How Has This Been Tested?
cargo test --workspace— full workspace suite passes; all 40harper-commentslanguage_supportfixtures keep their exact lint counts.cargo clippy -- -Dwarningspasses clean on the touched files.AI Disclosure
If Your PR Implements or Enhances a Linter
Checklist