This kind of PR is one of the main concerns I've had with the introduction of the non_exhaustive annotation. I feel like unless there's very clearly no reason why you'd ever want to exhaustively match against it, this annotation should never be used.

Alacritty itself is exhaustively matching against WindowEvent for example and it's not unusual at all for winit to make changes that actively require handling new events. So this makes it extremely easy to miss these.

As a downstream consumer, I really don't care if a change is breaking or not. I care about how much work it is for me to update to the new release. This makes it harder for me to update to new winit releases, while at the same time it's the same amount of effort to bump winit.

I don't think winit will go ahead and add a new event every second minor release which nobody downstream should handle. Winit also is pre-1.0, so there's basically no patch releases anyways. So I'm curious what upstream sees as a benefit to this?

Additionally, 1.40.0 isn't even close to being packaged in distros like Ubuntu (1.37.0), so this would block any winit bump in Alacritty for a long time.

I think I agree with @chrisduerr here.

Adding the non_exhaustive mark on these enums would be a convenience for winit maintainers, removing the need to worry about breaking changes by adding variants to them.

On the other hand, winit is not a "stability-sensitive" crate: it is extremely unlikely to appear several times in the dependency tree. As such, releasing breaking changes (especially small ones that would only add a few variants) is not really a big deal, even more so given we are pre 1.0.

My final view would basically be this: I believe the question of maybe adding the non_exhaustive mark to some enums should only be considered when winit will actively be working towards its 1.0, and there won't be any other API changes coming any more. And maybe at this point, the need to mark them as non_exhaustive will be gone anyway.

In any case, this is only me stating my opinion, don't take it as a "maintainer veto" or anything (see #972).

I feel like unless there's very clearly no reason why you'd ever want to exhaustively match against it, this annotation should never be used.

Alacritty itself is exhaustively matching against WindowEvent for example and it's not unusual at all for winit to make changes that actively require handling new events. So this makes it extremely easy to miss these.

It's worth noting that Winit is trending towards the point where any new events added to the API are optional: #1217 adds a ThemeChanged event that isn't necessary to handle universally, gamepad support is only really useful for games and home theater applications, and there are bits and bobs sprinkled throughout the API (such as the file hovering API) that really don't have to be handled everywhere. I'll agree that we shouldn't expose events that nobody handles, but events that only a portion of our downstream users handle are absolutely common occurrences. I'll also point out that that Alacritty does a decent amount of non-exhaustive matching even today, although it's admittedly not used everywhere.

That being said, I can accept that it's too early to add the annotations right now, and I'd be fine with letting this sit on the backburner until Winit gets closer to a 1.0 release. I still think we should absolutely add these annotations by the time 1.0 is out, but it's not too big of a deal to wait until Winit's API settles down more and Ubuntu catches up with the latest Rust version.

I think we do exhaustive matching on both WindowEvent and VirtualKeyCode at the moment. And as far as I'm concerned, just a single occurrence should already show that there clearly are downstream users that make use of this in a different fashion.

We're also talking about a sample size of 1 here and there are already problems. I'd imagine the more different usecases you look at, the more likely you are to run into unexpected behavior.

It's worth noting that Winit is trending towards the point where any new events added to the API are optional: #1217 adds a ThemeChanged event that isn't necessary to handle universally, gamepad support is only really useful for games and home theater applications, and there are bits and bobs sprinkled throughout the API (such as the file hovering API) that really don't have to be handled everywhere

Yeah and that's fine. You can have an API where things don't need to be exhaustively matched, that's perfectly sane. But this forces people not to exhaustively match them, even if they want to, which is a big difference.

Does Alacritty need to care about gamepad support at all? Probably not. But with a breaking change Alacritty can make the decision if it needs to care and it'll always know about new events just by virtue of nice compiler messages. Does Alacritty care if a gamepad event makes a release a breaking change, just to update to it without having to do anything? Not at all, I have to check for breaking changes constantly anyways, I love bumping a breaking change just to find out that nothing changes.

The only advantage would be the speed of propagation through libraries. If winit releases a breaking change, Alacritty needs to wait for glutin. That sucks and takes up time. But unless this is something that happens with like every release, I personally prefer that much over the alternatives.

With 1.0 it might make some more sense, simply because the time between releases is likely significantly bigger and there's going to be much more releases in-between. Semver allows adding new functionality in a minor version too, so that's perfectly fine. But even then I'm not sure if I would do it for all the enums in this PR (though it probably makes more sense to think about that specifically once it's time).

Just for another voice, as a user I don't want the non_exhaustive flag. If I'm matching everything then I want to know when there is a new event I should handle.