This is an archive of the discontinued LLVM Phabricator instance.

[clang] adds a discardable attribute
AbandonedPublic

Authored by cjdb on Jul 14 2022, 11:24 PM.

Details

Summary

Although the default behaviour for C and C++ is to not diagnose ignored function
calls, we can make this the default by using `#pragma clang attribute
push([[nodiscard]], apply_to = function)`. When we have a function that can
have its value discarded, we can use [[clang::discardable]] to indicate that
the `[[nodiscard]]` attribute should be ignored.

[[clang::discardable]] can be placed anywhere [[nodiscard]] is allowed,
but the presence of either is prioritised when it's applied to a callable.

Diff Detail

Event Timeline

cjdb created this revision.Jul 14 2022, 11:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 11:24 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
cjdb requested review of this revision.Jul 14 2022, 11:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 11:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cjdb updated this revision to Diff 444890.Jul 14 2022, 11:29 PM

undoes editor autotrimming of spaces

When we have a function that can have its value discarded, we can use [[clang::discardable]] to indicate that the [[nodiscard]] attribute should be ignored.

Alternatively, you can scope the pragma and declarations appropriately and then we don't need to add a new attribute. Is there a reason that wouldn't work for you?

(This attribute would be interesting to consider if we added a feature flag like -fdiagnose-discarded-results where the default is as-if everything was declared nodiscard and then you could mark the APIs with [[clang::discardable]] for the few whose results don't matter. However, that's also a much bigger feature because system headers are a challenge.)

cjdb added a subscriber: ldionne.Jul 15 2022, 9:44 AM

When we have a function that can have its value discarded, we can use [[clang::discardable]] to indicate that the [[nodiscard]] attribute should be ignored.

Alternatively, you can scope the pragma and declarations appropriately and then we don't need to add a new attribute. Is there a reason that wouldn't work for you?

A key problem here (IMO) is that this requires manually pushing and popping the pragma as it is required. With [[discardable]] one just needs to push/pop at the extremes of a file (and if a future version of module maps supports global pragmas for a module, there too, but that's a discussion that requires a design doc).

It's also good for documenting "hey you can discard this thing in my sea of nodiscard interfaces" on the interface itself. That coupling is also good for correctness because it means something can't accidentally slip in/out of the pragma's scope.

(This attribute would be interesting to consider if we added a feature flag like -fdiagnose-discarded-results where the default is as-if everything was declared nodiscard and then you could mark the APIs with [[clang::discardable]] for the few whose results don't matter. However, that's also a much bigger feature because system headers are a challenge.)

The primary reason for me wanting to add this attribute comes from my libc++ days, where I wanted to mark whatever made sense as nodiscard. @ldionne was in favour in principle, but against in practice; noting that nodiscard would clutter the interface too much, and it'd be better if the situation were reversed (which is what this patch does). I think adding this is still a good step for getting us to a world where -fdiagnose-discarded-results can be a real consideration, especially if libc++ and llvm-libc adopt it.

When we have a function that can have its value discarded, we can use [[clang::discardable]] to indicate that the [[nodiscard]] attribute should be ignored.

Alternatively, you can scope the pragma and declarations appropriately and then we don't need to add a new attribute. Is there a reason that wouldn't work for you?

A key problem here (IMO) is that this requires manually pushing and popping the pragma as it is required.

That's a feature to me, not a bug. It means you have to actually think about what you're doing instead of being surprised. This is the same reason our community standards want you to keep anonymous namespaces as small as possible -- it's too easy to have absolutely no idea about the behavior when you're in the middle of a block of code and the relevant part is off-screen somewhere.

With [[discardable]] one just needs to push/pop at the extremes of a file (and if a future version of module maps supports global pragmas for a module, there too, but that's a discussion that requires a design doc).

I understood that, I just don't think that's a good thing. This is basically an attribute that says "I know we said we wanted everything here to be nodiscard, but JUST KIDDING not this one!" which is not a very clean approach to writing headers.

It's also good for documenting "hey you can discard this thing in my sea of nodiscard interfaces" on the interface itself. That coupling is also good for correctness because it means something can't accidentally slip in/out of the pragma's scope.

I'd like to see some examples of that. In my experience, attributes are most often hidden behind macros which are sometimes then combined with other macros, etc, so whether the attribute is effective or not is already not always obvious from the interface; I don't see how this will move the needle.

(This attribute would be interesting to consider if we added a feature flag like -fdiagnose-discarded-results where the default is as-if everything was declared nodiscard and then you could mark the APIs with [[clang::discardable]] for the few whose results don't matter. However, that's also a much bigger feature because system headers are a challenge.)

The primary reason for me wanting to add this attribute comes from my libc++ days, where I wanted to mark whatever made sense as nodiscard. @ldionne was in favour in principle, but against in practice; noting that nodiscard would clutter the interface too much, and it'd be better if the situation were reversed (which is what this patch does). I think adding this is still a good step for getting us to a world where -fdiagnose-discarded-results can be a real consideration, especially if libc++ and llvm-libc adopt it.

Yes, I wish the default were correct as well, but it's not and it won't ever be corrected for practicality reasons. I don't know what libc++'s goals are in terms of other compilers, but it seems like using a clang-specific pragma would be making it more difficult to use libc++ with any other compiler.

cjdb abandoned this revision.Jul 25 2022, 3:14 PM
cjdb added a comment.Apr 21 2023, 10:07 AM

With [[discardable]] one just needs to push/pop at the extremes of a file (and if a future version of module maps supports global pragmas for a module, there too, but that's a discussion that requires a design doc).

I understood that, I just don't think that's a good thing. This is basically an attribute that says "I know we said we wanted everything here to be nodiscard, but JUST KIDDING not this one!" which is not a very clean approach to writing headers.

@aaron.ballman Ugh, I've finally come up with a good use-case for [[discardable]].

class [[nodiscard]] iterator {
public:
  // usual iterator stuff

  [[clang::discardable]] iterator operator++(int);
  [[clang::discardable]] iterator operator--(int);
};

I hate it, but the alternative is to decorate everything else with [[nodiscard]] just to facilitate these two operators. It's a compelling use-case, but I'm not sure if it's compelling enough on its own. WDYT?
(I personally think that those two should be nodiscard, but i++ is pervasive enough that it might never be practical to correct.)

With [[discardable]] one just needs to push/pop at the extremes of a file (and if a future version of module maps supports global pragmas for a module, there too, but that's a discussion that requires a design doc).

I understood that, I just don't think that's a good thing. This is basically an attribute that says "I know we said we wanted everything here to be nodiscard, but JUST KIDDING not this one!" which is not a very clean approach to writing headers.

@aaron.ballman Ugh, I've finally come up with a good use-case for [[discardable]].

class [[nodiscard]] iterator {
public:
  // usual iterator stuff

  [[clang::discardable]] iterator operator++(int);
  [[clang::discardable]] iterator operator--(int);
};

I hate it, but the alternative is to decorate everything else with [[nodiscard]] just to facilitate these two operators. It's a compelling use-case, but I'm not sure if it's compelling enough on its own. WDYT?
(I personally think that those two should be nodiscard, but i++ is pervasive enough that it might never be practical to correct.)

I'm still not super motivated because I think this encourages a confused design, because marking a *type* as nodiscard is making a promise about the type as a whole. That asserts there's *never* a valid time to ignore a value of the type. If you have times when that type is reasonable to ignore, then the type isn't actually non-discardable, and you should mark the methods returning the type. (IOW, "nodiscard" is part of the contract for the type in some limited ways even if it's not reflected by the type system.)

What's more, I think iterator is a case where it'd be a mistake to mark the type nodiscard, because there are plenty times when it's valid to ignore the type (consider many of the functions in <algorithm>, like std::copy()). I think it's more appropriate to mark the functions returning an iterator rather than the type itself.

With [[discardable]] one just needs to push/pop at the extremes of a file (and if a future version of module maps supports global pragmas for a module, there too, but that's a discussion that requires a design doc).

I understood that, I just don't think that's a good thing. This is basically an attribute that says "I know we said we wanted everything here to be nodiscard, but JUST KIDDING not this one!" which is not a very clean approach to writing headers.

@aaron.ballman Ugh, I've finally come up with a good use-case for [[discardable]].

class [[nodiscard]] iterator {
public:
  // usual iterator stuff

  [[clang::discardable]] iterator operator++(int);
  [[clang::discardable]] iterator operator--(int);
};

I hate it, but the alternative is to decorate everything else with [[nodiscard]] just to facilitate these two operators. It's a compelling use-case, but I'm not sure if it's compelling enough on its own. WDYT?
(I personally think that those two should be nodiscard, but i++ is pervasive enough that it might never be practical to correct.)

I'm still not super motivated because I think this encourages a confused design, because marking a *type* as nodiscard is making a promise about the type as a whole. That asserts there's *never* a valid time to ignore a value of the type. If you have times when that type is reasonable to ignore, then the type isn't actually non-discardable, and you should mark the methods returning the type. (IOW, "nodiscard" is part of the contract for the type in some limited ways even if it's not reflected by the type system.)

What's more, I think iterator is a case where it'd be a mistake to mark the type nodiscard, because there are plenty times when it's valid to ignore the type (consider many of the functions in <algorithm>, like std::copy()). I think it's more appropriate to mark the functions returning an iterator rather than the type itself.

I'm a little more sympathetic to the use than Aaron here, but I'm still at 'unmotivated' here. As he said, iterator I think is a poor candidate for nodiscard, as the iterator type itself is not the 'costly to ignore' part here, just the begin/end methods (and perhaps some of algorithms?). I can see some logic to having this in very limited situations, but I just don't see a motivating use case yet.

cjdb added a comment.Apr 21 2023, 12:44 PM

Yeah, in retrospect, I agree (and may need to clean up a few iterator types now, grr).