feat: enrich no-unsafe-enum-comparison diagnostics#867
Conversation
How to use the Graphite Merge QueueAdd the label 0-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR aims to enrich no-unsafe-enum-comparison diagnostics so users get more actionable guidance and clearer context when enum comparisons are unsafe.
Changes:
- Added
Helptext to the rule’s diagnostics to provide remediation guidance. - Introduced labeled ranges for comparisons/case clauses to highlight operand types alongside the main diagnostic.
- Updated rule-tester and E2E snapshots to reflect the new diagnostic output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/rules/no_unsafe_enum_comparison/no_unsafe_enum_comparison.go | Adds help text and constructs diagnostics with labeled operand ranges. |
| internal/rule_tester/snapshots/no-unsafe-enum-comparison.snap | Updates expected diagnostics output for the rule (help text + labeled ranges). |
| e2e/snapshots/snapshot.test.ts.snap | Updates E2E expected headless diagnostic output (includes changes beyond this rule). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func buildOperandRange(label string, node *ast.Node, typeText string) rule.RuleLabeledRange { | ||
| return rule.RuleLabeledRange{ | ||
| Label: label + ": " + typeText, | ||
| Range: node.Loc, | ||
| } | ||
| } | ||
|
|
||
| func buildComparisonDiagnostic( | ||
| typeChecker *checker.Checker, | ||
| node *ast.Node, | ||
| message rule.RuleMessage, | ||
| leftLabel string, | ||
| leftNode *ast.Node, | ||
| leftType *checker.Type, | ||
| rightLabel string, | ||
| rightNode *ast.Node, | ||
| rightType *checker.Type, | ||
| ) rule.RuleDiagnostic { | ||
| return rule.RuleDiagnostic{ | ||
| Range: node.Loc, | ||
| Message: message, | ||
| LabeledRanges: []rule.RuleLabeledRange{ | ||
| buildOperandRange(leftLabel, leftNode, typeChecker.TypeToString(leftType)), | ||
| buildOperandRange(rightLabel, rightNode, typeChecker.TypeToString(rightType)), | ||
| }, |
There was a problem hiding this comment.
buildComparisonDiagnostic/buildOperandRange use node.Loc for diagnostic and labeled-range spans. In this codebase, diagnostic ranges are typically trimmed to the first/last token (excluding leading trivia/comments) via utils.TrimNodeTextRange (see internal/linter/linter.go:214-218). Using Loc can cause diagnostics to highlight preceding comments/whitespace (as reflected by the updated snapshots where a comparison’s range starts on the comment line). Consider constructing the diagnostic Range and each operand Range with utils.TrimNodeTextRange(ctx.SourceFile, ...) (or equivalent token-trimmed ranges) to keep highlights stable and focused on the actual expression tokens.
There was a problem hiding this comment.
Updated this rule to use token-trimmed ranges via utils.TrimNodeTextRange for both the primary diagnostic span and operand labels, then refreshed the rule/e2e snapshots to match the trimmed highlights.
| "labeled_ranges": [ | ||
| { | ||
| "label": "Thrown value", | ||
| "range": { | ||
| "end": 70, | ||
| "pos": 63, | ||
| }, | ||
| }, | ||
| ], | ||
| "message": { | ||
| "description": "Expected an error object to be thrown.", | ||
| "help": "Throw an \`Error\` instance or subclass instead of a primitive, object literal, or other non-Error value.", | ||
| "id": "object", |
There was a problem hiding this comment.
This snapshot update adds message.help and labeled_ranges for only-throw-error diagnostics, but the current rule implementation (internal/rules/only_throw_error/only_throw_error.go) reports via ReportNode with messages that have no Help and does not populate LabeledRanges. This looks like the snapshot may be out of sync with the actual produced diagnostics (or the PR is missing the corresponding code changes). Please either include the code changes that add these fields, or revert/limit this snapshot update to only the intended rule(s).
There was a problem hiding this comment.
Confirmed. The original e2e snapshot was stale. I refreshed it from a branch-local rebuild so it now includes the intended no-unsafe-enum-comparison rich diagnostic output and removes the stale only-throw-error help/labeled-range entries.
b582df1 to
aee83d5
Compare
Merge activity
|
aee83d5 to
ff64618
Compare
No description provided.