Skip to content
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

Remove unwrap on line option, preventing DAP crash #9632

Merged
merged 2 commits into from Feb 16, 2024

Conversation

mattriverm
Copy link
Contributor

@mattriverm mattriverm commented Feb 15, 2024

Fix #4683.
xdebug.php-debug-1.34.0 does not provide a line value for this event, this change fixes the issue for me

helix_dap::transport [INFO] <- DAP event Breakpoint(Breakpoint { reason: "changed", breakpoint: Breakpoint { id: Some(6), verified: false, message: None, source: None, line: None, column: None, end_line: None, end_column: None, instruction_reference: None, offset: None } })

@mattriverm mattriverm changed the title Remove unwrap on line option, preventing DAP crash, ref #4683 Remove unwrap on line option, preventing DAP crash Feb 15, 2024
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-debug-adapter Area: Debug adapter client labels Feb 15, 2024
the-mikedavis
the-mikedavis previously approved these changes Feb 15, 2024
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

This seems like a fine fix to me but in the long-run I believe we will want to make Breakpoint.line an Option<usize> (in helix-view/src/editor.rs)

@gabydd
Copy link
Member

gabydd commented Feb 15, 2024

I think if it is none we shouldn't be changing the line, instead we should check if it is some and then set the line, as from what I remember change doesn't necessarily have to update everything/send back the old values

@pascalkuthe
Copy link
Member

yeah I need to look at the DAP stnardard again but I also think that if line number is null it does not mean 0

@gabydd
Copy link
Member

gabydd commented Feb 15, 2024

Looked into this for #8625 which this partially fixes though we should fix the resetting of everything else as well, what I found is at least lldb sends the change event just to change verified to true and only sends along the id of the breakpoint nothing else so by setting the info without checking if it's None we are losing some of the breakpoint information

@mattriverm
Copy link
Contributor Author

if let Some(line) = breakpoint.line {
    breakpoints[i].line = line.saturating_sub(1);    
}

I'm starting to feel like this would be a better solution at the moment

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Yeah, we should fall back to the existing values for these Option fields:

breakpoints[i].message = breakpoint
    .message
    .clone()
    .or_else(|| breakpoints[i].message.take());
breakpoints[i].line = breakpoint
    .line
    .map_or(breakpoints[i].line, |line| line.saturating_sub(1));
breakpoints[i].column =
    breakpoint.column.or(breakpoints[i].column);

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 15, 2024
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2024
@pascalkuthe pascalkuthe merged commit 76e512f into helix-editor:master Feb 16, 2024
6 checks passed
@mattriverm mattriverm deleted the mf/issue-4683 branch February 16, 2024 15:44
@gabydd
Copy link
Member

gabydd commented Feb 16, 2024

Thanks for fixing this!

uek-1 pushed a commit to uek-1/helix that referenced this pull request Feb 24, 2024
* Remove unwrap on line option, preventing DAP crash, ref helix-editor#4683

* Update to fall back to existing values for option fields
cosmikwolf pushed a commit to cosmikwolf/helix that referenced this pull request Feb 26, 2024
* Remove unwrap on line option, preventing DAP crash, ref helix-editor#4683

* Update to fall back to existing values for option fields
shortc pushed a commit to shortc/helix that referenced this pull request Mar 31, 2024
* Remove unwrap on line option, preventing DAP crash, ref helix-editor#4683

* Update to fall back to existing values for option fields
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
* Remove unwrap on line option, preventing DAP crash, ref helix-editor#4683

* Update to fall back to existing values for option fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debug-adapter Area: Debug adapter client S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashed when hit breakpoint
4 participants