sminez / penrose

A library for writing an X11 tiling window manager
https://sminez.github.io/penrose/
MIT License
1.26k stars 88 forks source link

penrose crashes due to unknown client and using expect. prefer if let #279

Closed griccardos closed 1 year ago

griccardos commented 1 year ago

Use of .expect causes crash when client is unknown in stack_set.rs. Probably better to use if let

 pub(crate) fn float_unchecked<R: RelativeTo>(&mut self, client: C, r: R) {
        let screen = self.screen_for_client(&client).expect("client to be known");
        let r = r.relative_to(&screen.r);
        self.floating.insert(client, r);

to

pub(crate) fn float_unchecked<R: RelativeTo>(&mut self, client: C, r: R) {
        if let Some(screen) = self.screen_for_client(&client) {
            let r = r.relative_to(&screen.r);
            self.floating.insert(client, r);
        }

Libraries generally shouldn't use expect/unwrap as a rule unless absolutely necessary. This one bit me because i have penrose running in a while true loop like you suggest, and it just crashed and restarted forever.

I have already created fix, let me know if you want the PR.

sminez commented 1 year ago

Libraries generally shouldn't use expect/unwrap as a rule unless absolutely necessary.

The _unchecked suffix of the method in question here is by design. This method is an internal API and is only called in situations where we know for sure that the client is present, with the expect marking the invariant that is being relied upon. If that invariant isn't holding then there is a bug in the program and crashing so that it is easier to locate is a deliberate design choice. I'd also just point out the large number of standard library code (and other library code on crates.io) that panics when invariants are violated. This is a common practice in for asserting such invariants, not an oversight.

The only place in the penrose code base that this is currently called (other than tests) is inside of the float method where it is guarded by a check for the client being present in the stackset. The issue is more likely that in order to call float_unchecked the client in question must be present on a screen so we can determine it's relative position to the screen, so I assume that you are trying to call float on a hidden client somehow? That's not really going to work as there's no way to determine the required RelativeRect for the client in question.

sminez commented 1 year ago

I've added an additional error case to the float method around attempting to call it for clients that are not currently mapped to a screen.