Skip to content

Rust: avoid panic from line_index crate #18759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

redsun82
Copy link
Contributor

We found out we can stumble upon a line_index.line_col panic, specifically when reporting a macro parse error.

This replaces line_col with safer try_line_col, and deals more gracefully with the error.

We found out we can stumble upon a `line_index.line_col` panic,
specifically when reporting a macro parse error.

This replaces `line_col` with safer `try_line_col`, and deals more
gracefully with the error.
@redsun82 redsun82 requested a review from aibaars February 12, 2025 15:59
@Copilot Copilot AI review requested due to automatic review settings February 12, 2025 15:59
@github-actions github-actions bot added the Rust Pull requests that update Rust code label Feb 12, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This pull request addresses a potential panic scenario when using the line_index crate by replacing calls to line_index.line_col with the safer try_line_col. Key changes include adding a constant for a fallback unknown location, updating the location() function to return an Option with proper error propagation, and adjusting its callers to handle the new behavior gracefully.

Changes

File Description
rust/extractor/src/translate/base.rs Replaces direct line_col calls with try_line_col; updates error handling and control flow to avoid panics in diagnostic reporting

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

rust/extractor/src/translate/base.rs:84

  • [nitpick] Consider adding a comment explaining the rationale behind using (0, 0) for UNKNOWN_LOCATION to aid future maintenance and clarify its intended fallback behavior.
const UNKNOWN_LOCATION: (LineCol, LineCol) =

rust/extractor/src/translate/base.rs:215

  • Using unwrap_or with UNKNOWN_LOCATION may hide issues with location resolution; consider logging a warning or error when try_line_col fails to improve diagnostic traceability.
location.unwrap_or(UNKNOWN_LOCATION)

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

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

Looks great!

@redsun82 redsun82 merged commit 98c755d into main Feb 12, 2025
13 checks passed
@redsun82 redsun82 deleted the redsun82/rust-avoid-linecol-panic branch February 12, 2025 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants