Skip to content

Conversation

@madelynnblue
Copy link
Contributor

Fixes #5933

@madelynnblue
Copy link
Contributor Author

My (probably buggy?) acme client causes rust-anayzer to crash a few times per day on this. While clients should do a good job, it is currently trivial to panic rust-analyzer if you send it a ChangeText notification on some random path.

@SomeoneToIgnore
Copy link
Contributor

Thank you!

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 24, 2020

@bors bors bot merged commit 2ff78cd into rust-lang:master Nov 24, 2020
@madelynnblue madelynnblue deleted the change-unwrap branch November 24, 2020 02:33
@kjeremy
Copy link
Contributor

kjeremy commented Nov 24, 2020

Is this correct behavior? Why doesn't the path exist? The client needs to gain ownership of the fire first by sending DidOpen which should have a valid URI.

@madelynnblue
Copy link
Contributor Author

I agree the client should do that. I thought I programmed mine to always send an open first, but apparently it has a bug. However, it should never just panic rust-analyzer.

@matklad
Copy link
Contributor

matklad commented Nov 30, 2020

However, it should never just panic rust-analyzer.

For some historical context, we used to deliberately panic in such cases (to force the us/clients fix all the bugs), but we've changed our stance on this.

That said, I think we need to abstract this away into some kind of log_protocol_violation error, such that we can show an error message to the client or something like that. But that, to be done nicely, requires re-engineering of our event loop I think....

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.

panic: called Option::unwrap() on a None value

4 participants