Skip to content

sed: add modern diagnostic like coreutils - #574

Closed
sylvestre wants to merge 2 commits into
uutils:mainfrom
sylvestre:sed-diagnostics
Closed

sylvestre wants to merge 2 commits into
uutils:mainfrom
sylvestre:sed-diagnostics

Conversation

@sylvestre

Copy link
Copy Markdown
Contributor

No description provided.

Start an mdBook under docs/, laid out as in coreutils, and move the list of
extensions and incompatibilities out of the README into it. The README keeps
a pointer.
sed already knows exactly where a script error is -- every message carries
input:line:column -- but a one-line message has nowhere to show it. Quote the
offending script line back and underline the character at fault, gated on
uucore::diagnostics::enabled() so that UUTILS_DIAG behaves as in every other
utility, and drawn with ariadne, the renderer uucore uses. uucore's own
Snapshot renders argument lists, so it would label the excerpt sed:1:N and
could name neither a -f script nor the line within it.

The excerpt is drawn only when stderr is a terminal, so a pipe, a script or
the test suite still sees the single line it parses: the ~21 exact stderr
assertions in the suite needed no change, and UUTILS_DIAG=always is how the new tests
ask for a report down a pipe.

Everything flows through the two existing constructors, so no call site
changes: compilation_error covers the ~66 sites in compiler.rs and
delimited_parser.rs, and location_error covers semantic errors.

Semantic errors are raised once the whole script is compiled, long after the
line was consumed -- the line provider streams and keeps no history -- so
ScriptLocation now carries the line text along with the position. It is only
captured when diagnostics are on, otherwise every compiled command would pay
for a copy nobody reads, and even then the commands compiled from one line
share a single copy of it -- a generated one-line script of 20,000 commands
would otherwise hold 20,000 copies of itself.

Only the offending line is still in hand, so it is padded with the newlines
that came before it; that way the gutter shows the line number the message
quotes rather than always 1. Errors that run off the end of the line report
the column just past the last character, so the line gets a trailing space
for the caret to sit on and the drawn column matches the one the message
names.

Anything that cannot be drawn -- a script that is not valid UTF-8, an empty
line, a location with no line recorded, an error hit once the whole script
has been read (whose message names line 0) -- leaves the message alone
rather than guessing.
Copilot AI lite review requested due to automatic review settings September 26, 2026 17:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sylvestre

Copy link
Copy Markdown
Contributor Author

Superseded by the stack #575 (mdBook docs) → #576 (modern diagnostics).

@sylvestre sylvestre closed this Sep 26, 2026
@codecov

codecov Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 38.00000% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.20%. Comparing base (76086c8) to head (46b0c68).

Files with missing lines Patch % Lines
src/sed/error_handling.rs 36.95% 57 Missing and 1 partial ⚠️
src/sed/script_char_provider.rs 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
- Coverage   83.88%   83.20%   -0.68%     
==========================================
  Files          14       14              
  Lines        6974     7062      +88     
  Branches      406      412       +6     
==========================================
+ Hits         5850     5876      +26     
- Misses       1121     1182      +61     
- Partials        3        4       +1     
Flag Coverage Δ
macos_latest 84.31% <38.00%> (-0.72%) ⬇️
ubuntu_latest 84.53% <38.00%> (-0.71%) ⬇️
windows_latest 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed

codspeed Bot commented Sep 26, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 4.27%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 9 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ number_fix 1.1 s 1.3 s -11.32%
⚡ access_log_subst 2.4 s 2.3 s +3.33%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing sylvestre:sed-diagnostics (46b0c68) with main (76086c8)

Open in CodSpeed

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.

2 participants