Prompted by this rust-analyzer bug: apparently it is difficult for rust-analyzer to report certain classes of protocol errors in any way other by panic-ing, because the error is detected by String::replace_range, which panics on invalid offsets, and has no fallible version. Abstractly one could hand-validate offsets coming from outside the program up front, but, as pointed out near the bottom of the bug, this involves doing duplicate work, and I imagine it would be easy to get wrong.
What do you think of adding a fallible version, presumably with the signature
It’s actually would not be difficult to report for rust-analyzer: one can use get (String in std::string - Rust) to check if the range is valid, and then bubble the error up. In general, indexing APIs in Rust idiomatically panic on invalid indexes, so I don’t think any change is necessary here.
For some context, historically rust-analyzer used to deliberately and loudly crash for any kind of invalid input. I stand behind that design decision, as it allowed us to discover and fix numerous bugs across rust-analyzer, official VS Code LSP library and other clients. Roughly when we started the process of making rust-analyzer official, we downgraded this strategy to more classical “gracefully handle invalid inputs” for more or less every API except textDocument/didChange. This one doesn’t have a reasonable recovery strategy, and would completely break any other feature. Really, here the client and server just have to agree on the protocol. So in the current implementation we continue to deliberately crash. The best solution here is to send a PR to specific client libraries which send invalid offsets.
EDIT: this was worded in a rather unhelpful way, sorry about that, rephrased a bit!
Let's keep the question of whether rust-analyzer should panic, in this instance, over on the rust-analyzer issue tracker (I admit I'm being pretty dogmatic about it over there.)
Here, I'd like to focus discussion on (1) whether anyone besides rust-analyzer could potentially make use of a try_replace_range API, and (2) whether there are good alternatives in the stdlib today, and how error-prone they are in practice.
On that second note, it seems to me that recommending .get to validate offsets might lead people into a trap where they forget that both ends of the range need to be validated.
another function I noticed was missing is replace_range_from_within, ... essentially s.replace_range(target_range, &s[source_range].to_string()) but without the temporary string
A way to frame this question is to think if this API didn't exist, what's the better addition, the panicking version or the fallible version?
I think that the only way to justify the panicking version is if both inputs are known at compile time or somehow all instances of invalid offsets are known at compile time (probably because the offset was taken from processing the string itself). In that case, panics are definitely a programmer error and the panicking version is warranted. Otherwise, even if you want to bail on error, using the fallible version (and maybe unwrapping it) makes way more sense.
So if both existed I think that the fallible version would be way more useful
It's also possible that you have already established at runtime that the offsets are valid, but I agree such occasions are likely to be in the minority.
Well, my usual question is where would someone usually get the indexes from? My instinct is that you get the indexes from doing some kind of search on the string, in which case it's already a previous logic bug if the index isn't valid, at which point the panicking version is better than making almost everyone just .unwrap() it because of course the index they got from regex or whatever is correct.
In general, indexes aren't just summoned from the æther -- aka they have an implied expectation that they're valid -- so I lean towards the panicking versions. Indexes are very different that way from fundamentally-not-precheckable things like paths.
(I admit I'm being pretty dogmatic about it over there.)