Complete: Shared user-editable span recovery helpers (8.1.2)#197
Open
Complete: Shared user-editable span recovery helpers (8.1.2)#197
Conversation
This adds a detailed living document ExecPlan for roadmap item 8.1.2, which defines the scope, goals, constraints, and steps to implement shared helpers for recovering user-editable spans in macro-heavy `rstest` code paths. The plan outlines a pure policy in `whitaker-common`, a `rustc` adapter in `whitaker`, and adoption in an existing lint consumer, along with documentation updates and validation steps. This groundwork facilitates improved lint diagnostics in macro-generated test code. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
|
This PR implements roadmap item 8.1.2 by adding shared, macro-aware user-editable span recovery helpers: a pure policy layer in whitaker-common, a rustc_span adapter in whitaker (feature-gated), and a first-adopter integration in function_attrs_follow_docs. The ExecPlan for this work is included at docs/execplans/8-1-2-shared-user-editable-span-recovery-helpers.md and the roadmap entry 8.1.2 is marked COMPLETE. Key changes Core policy layer (whitaker-common)
Rustc adapter layer (whitaker) — feature-gated
Consumer integration (function_attrs_follow_docs)
Tests and QA-level changes
Documentation & process
Review comments & fixes addressed
Notes for reviewers
WalkthroughIntroduce shared, macro-aware span-recovery helpers: a pure policy module in Changes
Sequence Diagram(s)sequenceDiagram
participant CA as Consumer (function_attrs_follow_docs)
participant HA as HIR Adapter (whitaker::hir)
participant RS as Rustc Span (source_callsite)
participant RP as Recovery Policy (whitaker_common::rstest)
CA->>HA: recover_user_editable_hir_span(span)
HA->>HA: span_recovery_frames(span)
loop while from_expansion and progressing
HA->>RS: source_callsite()
RS-->>HA: next span
HA->>HA: push SpanRecoveryFrame(next_span, from_expansion)
end
HA->>RP: recover_user_editable_span(frames)
RP->>RP: scan frames for first non-expansion
alt first frame non-expansion
RP-->>HA: UserEditableSpan::Direct
else later non-expansion found
RP-->>HA: UserEditableSpan::Recovered
else no user frame
RP-->>HA: UserEditableSpan::MacroOnly
end
HA-->>CA: Option<Span> (Recovered or None)
CA->>CA: use for attribute ordering & containment checks
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideImplements shared, pure span-recovery policy for user-editable spans in whitaker-common, adds a rustc_span::Span adapter in whitaker, and wires a first real adopter (function_attrs_follow_docs) to use these helpers, along with unit/BDD tests and documentation/roadmap updates marking roadmap item 8.1.2 complete. Sequence diagram for function attribute checking using shared span recoverysequenceDiagram
participant Driver as LintDriver
participant Check as FunctionAttributeCheck
participant HirAdapter as HirSpanAdapter
participant AttrInfo as AttrInfo
participant Policy as rstest_span_module
participant function_attrs_driver_module as function_attrs_driver_module
Driver->>Check: construct FunctionAttributeCheck(item_span, attrs)
Driver->>HirAdapter: recover_user_editable_hir_span(Check.item_span)
HirAdapter->>HirAdapter: span_recovery_frames(item_span)
HirAdapter->>Policy: recover_user_editable_span(frames)
Policy-->>HirAdapter: UserEditableSpan~Span~
HirAdapter-->>Driver: item_user_editable_span: Option~Span~
loop for each attr in Check.attrs
Driver->>AttrInfo: try_from_hir(attr)
AttrInfo->>HirAdapter: recover_user_editable_hir_span(attr.span)
HirAdapter->>HirAdapter: span_recovery_frames(attr.span)
HirAdapter->>Policy: recover_user_editable_span(frames)
Policy-->>HirAdapter: UserEditableSpan~Span~
HirAdapter-->>AttrInfo: user_editable_span: Option~Span~
AttrInfo-->>Driver: AttrInfo
end
Driver->>Driver: retain infos where
Driver->>function_attrs_driver_module: attribute_within_item(info.user_editable_span(), item_user_editable_span, Check.item_span)
function_attrs_driver_module-->>Driver: bool (discard if false or MacroOnly)
Driver->>AttrInfo: source_order_key()
AttrInfo-->>Driver: (lo, hi) from user_editable_span or span
Driver->>Driver: sort infos by source_order_key
Class diagram for span recovery policy, adapter, and adopterclassDiagram
class SpanRecoveryFrame~T~ {
- value: T
- from_expansion: bool
+ new(value: T, from_expansion: bool) SpanRecoveryFrame~T~
+ value() &T
+ into_value() T
+ from_expansion() bool
}
class UserEditableSpan~T~ {
<<enum>>
+ Direct(T)
+ Recovered(T)
+ MacroOnly
+ into_option() Option~T~
}
class rstest_span_module {
+ recover_user_editable_span~T~(frames: &[SpanRecoveryFrame~T~]) UserEditableSpan~T~
}
rstest_span_module --> SpanRecoveryFrame
rstest_span_module --> UserEditableSpan
class HirSpanAdapter {
+ span_recovery_frames(span: Span) Vec~SpanRecoveryFrame~Span~~
+ recover_user_editable_hir_span(span: Span) Option~Span~
}
HirSpanAdapter --> SpanRecoveryFrame
HirSpanAdapter --> UserEditableSpan
class AttrInfo {
- span: Span
- user_editable_span: Option~Span~
- is_doc: bool
- is_outer: bool
+ try_from_hir(attr: Attribute) Option~AttrInfo~
+ span() Span
+ source_order_key() (BytePos, BytePos)
+ user_editable_span() Option~Span~
}
class FunctionAttributeCheck {
- item_span: Span
- attrs: Vec~Attribute~
+ new(item_span: Span, attrs: Vec~Attribute~) FunctionAttributeCheck
}
class function_attrs_driver_module {
+ check_function_attributes(check: FunctionAttributeCheck) void
+ attribute_within_item(attribute_span: Option~Span~, item_span: Option~Span~, raw_item_span: Span) bool
}
function_attrs_driver_module --> HirSpanAdapter
function_attrs_driver_module --> AttrInfo
function_attrs_driver_module --> FunctionAttributeCheck
AttrInfo --> HirSpanAdapter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
- Introduce SpanRecoveryFrame, UserEditableSpan, and recover_user_editable_span in common::rstest. - Implement pure policy to locate first user-editable span from macro expansion chains. - Expose rustc_span::Span adapter and re-exports in whitaker crate. - Adopt shared recovery in function_attrs_follow_docs to improve attribute ordering and filtering. - Add comprehensive unit tests and rstest-bdd scenarios for recovery behavior. - Update documentation and roadmap to reflect implementation progress and rationale. This enables consistent handling of macro-expanded spans and suppresses diagnostics on macro-only glue, improving lint accuracy and source diagnostics across crates. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on file fn treats_all_expansion_frames_as_macro_only() {
let first = source_span(4, 1, 4);
let second = source_span(5, 1, 6);
assert_eq!(
recover_user_editable_span(&[
SpanRecoveryFrame::new(first, true),
SpanRecoveryFrame::new(second, true),
]),
UserEditableSpan::MacroOnly
);
}❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Introduced `assert_span_recovery` helper to simplify and unify span recovery assertions in tests. Replaced repetitive assertion code with concise calls to the helper, enhancing readability and maintainability of span recovery test cases. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rtions Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/tests/rstest_span_recovery_behaviour.rs`:
- Around line 55-60: The panic message in function source_span does not include
which line failed; update the panic in the Err arm of SourceSpan::new (in
function source_span) to include the line parameter and the error (e.g.,
"behaviour test span invalid for line {line}: {error:?}") so test failures
report the offending line number and the underlying error.
In `@crates/function_attrs_follow_docs/src/tests/order_detection.rs`:
- Around line 113-130: Add a new test in
crates/function_attrs_follow_docs/src/tests/order_detection.rs that asserts
attribute_within_item returns false when the item span is None: create an
attribute_span (e.g., test_span(12,20)), pass Some(attribute_span) for the
attribute, None for the item_span, and the item_bound span (e.g.,
test_span(10,40)) as the third argument, then
assert!(!attribute_within_item(...)); reference the existing tests and the
attribute_within_item function to place the new test alongside
macro_only_attributes_are_dropped_from_item_comparison and
recovered_attribute_spans_stay_in_item_bounds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1200fa2d-a1f2-45c4-ace6-65c26327b08e
📒 Files selected for processing (13)
common/src/lib.rscommon/src/rstest/mod.rscommon/src/rstest/span.rscommon/src/rstest/tests.rscommon/tests/features/rstest_span_recovery.featurecommon/tests/rstest_span_recovery_behaviour.rscrates/function_attrs_follow_docs/src/driver.rscrates/function_attrs_follow_docs/src/tests/order_detection.rsdocs/execplans/8-1-2-shared-user-editable-span-recovery-helpers.mddocs/lints-for-rstest-fixtures-and-test-hygiene.mddocs/roadmap.mdsrc/hir.rssrc/lib.rs
Owner
Author
|
@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix: ❌ Failed checks (3 warnings)
|
This comment was marked as resolved.
This comment was marked as resolved.
Replaced the usage of recover_user_editable_span with an explicit loop to recover the first user-editable HIR span from a macro expansion chain. This improves clarity by avoiding unnecessary dependencies and provides a straightforward iterative approach. Additional improvements: - Changed function attribute_within_item to unwrap item span or fallback to raw item span for recovery. - Added tests for attribute_within_item to verify behavior with recovered attributes and raw spans. - Minor fixes to error messages and documentation clarity in ExecPlan. These changes enhance maintainability and correctness of span recovery mechanisms and related tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22822
Overall Comment 1 — allocation in Overall Comment 2 — Individual Comment 1 — tests for
Individual Comment 2 — "ExecPlan" acronym undefined on first use Annulment check: None of the requirements violated en-GB-oxendict spelling conventions or increased cyclomatic complexity, so no annulments are needed. |
…ntation - Introduce a policy layer with reusable span recovery utilities in whitaker-common. - Add adapter layer in whitaker crate for rustc_span integration. - Provide functions to recover user-editable spans, excluding macro-only glue spans. - Add detailed documentation on usage, extension, and test coverage. - Include integration tests in function_attrs_follow_docs for attribute span handling. - Improve lint handling by ignoring macro-generated attribute spans, avoiding spurious diagnostics. - Update docs (developers-guide, users-guide, documentation-style-guide) to reflect the new span recovery approach and usage guidelines. - Enhance tests in hir.rs and order_detection.rs to validate span recovery behavior. This feature enables lints and tools to correctly handle and recover source locations when procedural macros reorder or inject attributes, improving the accuracy of diagnostics and user experience. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation-style-guide.md`:
- Line 187: Replace the incorrect fenced-block language identifier `text` with
`plaintext` in the documentation (change the opening fence that currently reads
```text to ```plaintext) so the non-code text blocks comply with the project's
documentation-style-guide; update any other occurrences of the `text` fenced
code marker in the file to `plaintext` to ensure consistency.
- Line 269: Replace any fenced-code block language identifier of ```text with
```plaintext in the documentation (i.e., change the literal token ```text to
```plaintext for non-code text fences); search for occurrences of the string
"```text" (e.g., the top of the non-code blocks) and update them to
"```plaintext", then run markdown lint/format checks to ensure the file remains
valid.
In `@docs/users-guide.md`:
- Around line 290-291: Add an Oxford comma to the sentence "With `rstest`, place
the doc comment before all attributes including the test annotation:" by
inserting a comma after "attributes" so it reads "With `rstest`, place the doc
comment before all attributes, including the test annotation:" to improve
clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d944f0f-4849-49fc-9ee4-e1c59635f94e
📒 Files selected for processing (7)
common/tests/rstest_span_recovery_behaviour.rscrates/function_attrs_follow_docs/src/driver.rscrates/function_attrs_follow_docs/src/tests/order_detection.rsdocs/developers-guide.mddocs/documentation-style-guide.mddocs/users-guide.mdsrc/hir.rs
| ### Roadmap example | ||
|
|
||
| ```markdown | ||
| ```text |
There was a problem hiding this comment.
Use plaintext instead of text for non-code fenced blocks.
The coding guidelines explicitly require plaintext as the language identifier for non-code text blocks. Change text to plaintext to comply with the project's documentation standards.
Triage: [type:syntax/md]
📝 Proposed fix
-```text
+```plaintextAs per coding guidelines: "Always provide a language identifier for fenced code blocks; use plaintext for non-code text."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/documentation-style-guide.md` at line 187, Replace the incorrect
fenced-block language identifier `text` with `plaintext` in the documentation
(change the opening fence that currently reads ```text to ```plaintext) so the
non-code text blocks comply with the project's documentation-style-guide; update
any other occurrences of the `text` fenced code marker in the file to
`plaintext` to ensure consistency.
| ### ADR template | ||
|
|
||
| ```markdown | ||
| ```text |
There was a problem hiding this comment.
Use plaintext instead of text for non-code fenced blocks.
The coding guidelines explicitly require plaintext as the language identifier for non-code text blocks. Change text to plaintext to comply with the project's documentation standards.
Triage: [type:syntax/md]
📝 Proposed fix
-```text
+```plaintext🤖 AI coding-agent prompt to apply both fixes
Apply the following changes to docs/documentation-style-guide.md:
1. On line 187, replace ```text with ```plaintext
2. On line 269, replace ```text with ```plaintext
After applying the changes, validate by running:
- make markdownlint to verify Markdown syntax
- make fmt to format the file
- Visually inspect lines 187 and 269 to confirm the changes
As per coding guidelines: "Always provide a language identifier for fenced code blocks; use plaintext for non-code text."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/documentation-style-guide.md` at line 269, Replace any fenced-code block
language identifier of ```text with ```plaintext in the documentation (i.e.,
change the literal token ```text to ```plaintext for non-code text fences);
search for occurrences of the string "```text" (e.g., the top of the non-code
blocks) and update them to "```plaintext", then run markdown lint/format checks
to ensure the file remains valid.
Comment on lines
+290
to
+291
| With `rstest`, place the doc comment before all attributes including the test | ||
| annotation: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add Oxford comma for clarity.
Insert a comma after "attributes" to improve readability.
Triage: [type:grammar]
📝 Proposed fix
-With `rstest`, place the doc comment before all attributes including the test
+With `rstest`, place the doc comment before all attributes, including the test
annotation:🧰 Tools
🪛 LanguageTool
[uncategorized] ~290-~290: Possible missing comma found.
Context: ...test`, place the doc comment before all attributes including the test annotation: ```rust...
(AI_HYDRA_LEO_MISSING_COMMA)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/users-guide.md` around lines 290 - 291, Add an Oxford comma to the
sentence "With `rstest`, place the doc comment before all attributes including
the test annotation:" by inserting a comma after "attributes" so it reads "With
`rstest`, place the doc comment before all attributes, including the test
annotation:" to improve clarity.
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.
Summary
rstestcode paths. This turns the prior planning material into an implemented foundation with a pure policy inwhitaker-common, a thinrustc_span::Spanadapter inwhitaker, and an adopter infunction_attrs_follow_docsdemonstrating the approach. Includes tests, docs, and execution-plan updates.Rationale
whitaker-common, adapter inwhitaker, and a single adopter to demonstrate correctness and reduce crate-local logic in macro-heavy paths.Changes
whitakerfacade when the dylint-driver feature is enabled.Goals and implementation outline (updated)
whitaker-commonfor determining if a frame in a span chain is user-editable and whether recovery is possible.rustc_span::Spanadapter inwhitaker(viasrc/hir.rs) to walk the source-callsite chain and produce an ordered frame list consumable by the policy.function_attrs_follow_docs) adopting the shared helper to reduce crate-local logic and properly handle macro-generated glue.Implementation stages (summary)
rustc_span::Spanadapter (surface and re-exports behinddylint-driver).function_attrs_follow_docsand regression tests).make fmt,make markdownlint,make nixie,make check-fmt,make lint,make test).Validation and evidence (planning gates)
Documentation impact
docs/execplans/8-1-2-shared-user-editable-span-recovery-helpers.mdand marked COMPLETE.docs/lints-for-rstest-fixtures-and-test-hygiene.md.Notes for reviewers
whitaker-commonandwhitakerbehind a feature gate. One adopter demonstrates viability and helps validate policy correctness and integration points.Task link
◳ Generated by DevBoxer ◰
Summary by Sourcery
Introduce shared user-editable span recovery helpers and adopt them in an existing rstest-related lint path to avoid diagnostics on macro-only macro expansion glue.
New Features:
Enhancements:
Documentation:
Tests: