Skip to content

Add [[nodiscard]] to overloads of static events that return winrt::event_token#1559

Open
YexuanXiao wants to merge 1 commit into
microsoft:masterfrom
YexuanXiao:nodiscard-static-event-upstream
Open

Add [[nodiscard]] to overloads of static events that return winrt::event_token#1559
YexuanXiao wants to merge 1 commit into
microsoft:masterfrom
YexuanXiao:nodiscard-static-event-upstream

Conversation

@YexuanXiao
Copy link
Copy Markdown
Contributor

@YexuanXiao YexuanXiao commented Apr 5, 2026

Some events in WinRT are static, such as Clipboard.ContentChanged.For these events, the only way to release a delegate is through winrt::event_token, because the delegate is not stored in any user-controllable object. Therefore, if the winrt::event_token returned by these events is discarded, the delegate will leak. This PR adds the [[nodiscard]] attribute to the overloads that return winrt::event_token for static events to indicate that their return value should not be ignored, just like the overloads that return a revoker.

struct Clipboard
{
    [[nodiscard]] static auto ContentChanged(EventHandler<IInspectable> const& handler);
};

@sylveon
Copy link
Copy Markdown
Contributor

sylveon commented Apr 5, 2026

Users may want to permanently subscribe to an event for the lifetime of their program, in which case discarding the event token seems intentional.

@YexuanXiao
Copy link
Copy Markdown
Contributor Author

YexuanXiao commented Apr 5, 2026

It does not affect non-static events, and their overloads that return winrt::event_token still do not have this attribute. Non-static events can be released through the object they are associated with, so discarding the return value does not cause a leak, whereas static events do not have such an object. The impact of this patch is very small; if users wish to discard the return value, they can use (void)expr.

@sylveon
Copy link
Copy Markdown
Contributor

sylveon commented Apr 5, 2026

I think we should only add [[nodiscard]] if dropping the value is always considered an error

@YexuanXiao
Copy link
Copy Markdown
Contributor Author

I'm not entirely sure which static events might need to be subscribed to permanently, but at least the Clipboard example performs proper cleanup and I don't believe that well-written code should allow such a leak.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the C++/WinRT code generator to annotate static event “add” overloads (the ones that return winrt::event_token) with [[nodiscard]], helping prevent accidental loss of the token needed to later unsubscribe from static events (where losing the token effectively leaks the handler).

Changes:

  • Treat add_ overloads in static factory method generation as “events” and apply [[nodiscard]] to their generated static declarations.
  • Minor refactor to compute is_event once and reuse it for both declaration emission and revoker-type emission logic.

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.

3 participants