Page MenuHomePhabricator

Support warn_unused_result on typedefs
Needs ReviewPublic

Authored by dblaikie on May 8 2021, 8:23 PM.

Details

Summary

While it's not as robust as using the attribute on enums/classes (the
type information may be lost through a function pointer, a declaration
or use of the underlying type without using the typedef, etc) but I
think there's still value in being able to attribute a typedef and have
all return types written with that typedef pick up the
warn_unused_result behavior.

Specifically I'd like to be able to annotate LLVMErrorRef (a wrapper for
llvm::Error used in the C API - the underlying type is a raw pointer, so
it can't be attributed itself) to reduce the chance of unhandled errors.

Diff Detail

Event Timeline

dblaikie requested review of this revision.May 8 2021, 8:23 PM
dblaikie created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2021, 8:23 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Oh, one question: If we do move forward with this patch, how would we detect that the compiler supports this new use of warn_unused_result? (so that the feature detection macros in LLVM properly make the attribute a no-op unless we have a version of clang that supports it)

dblaikie updated this revision to Diff 343885.May 8 2021, 9:09 PM

Fixes for a few other test cases (though I wonder if these tests are overconstrained - do we need to be testing the list of things this attribute can be applied to in so many places?)

Let me start off by saying: thanks, I think this is really useful functionality. As a ridiculously obvious example, Annex K has an integer type alias errno_t and it would incredibly handy to be able to mark that as [[nodiscard]] to strongly encourage checking for errors for functions that return that type (not that we support Annex K, but the general idea extends to any integer-based error type).

That said, there are multiple different ways to spell the same semantic attribute, and it's not clear to me that we want to change them all the same way.

[[clang::warn_unused_result]] is ours to do with as we please, so there's no issue changing that behavior.

__attribute__((warn_unused_result)) and [[gnu::warn_unused_result]] are governed by GCC and we try to be compatible with their behavior. GCC does not appear to support the attribute on typedefs from my testing, so we'd want to coordinate with the GCC folks to see if there's an appetite for the change.

[[nodiscard]] is governed by both the C++ and C standards and we should not be changing its behavior without some extra diagnostics about extensions (and, preferably, some sort of attempt to standardize the behavior with WG14/WG21).

Do you have an appetite to talk to the GCC and standards folks? If not, then I think the safest bet is to only change the behavior for the [[clang::warn_unused_result]] and to leave the other forms alone for now.

Fixes for a few other test cases (though I wonder if these tests are overconstrained - do we need to be testing the list of things this attribute can be applied to in so many places?)

If the semantics of the attribute are identical regardless of how it's spelled, then we probably don't need the tests all spread out in multiple files. However, I don't think there's an intention to keep all of the spellings with identical semantics, so the different tests might still make sense (but could perhaps be cleaned up).

Oh, one question: If we do move forward with this patch, how would we detect that the compiler supports this new use of warn_unused_result? (so that the feature detection macros in LLVM properly make the attribute a no-op unless we have a version of clang that supports it)

__has_[c|cpp]_attribute returns a value, so we'd likely wind up using that return value to distinguish between versions.

Let me start off by saying: thanks, I think this is really useful functionality. As a ridiculously obvious example, Annex K has an integer type alias errno_t and it would incredibly handy to be able to mark that as [[nodiscard]] to strongly encourage checking for errors for functions that return that type (not that we support Annex K, but the general idea extends to any integer-based error type).

That said, there are multiple different ways to spell the same semantic attribute, and it's not clear to me that we want to change them all the same way.

[[clang::warn_unused_result]] is ours to do with as we please, so there's no issue changing that behavior.

__attribute__((warn_unused_result)) and [[gnu::warn_unused_result]] are governed by GCC and we try to be compatible with their behavior. GCC does not appear to support the attribute on typedefs from my testing, so we'd want to coordinate with the GCC folks to see if there's an appetite for the change.

FWIW, looks like we already diverge from GCC's behavior - GCC doesn't seem to support this attribute on types at all: https://godbolt.org/z/8YjqnE4cv (but does support [[nodiscard]] in that place)

[[nodiscard]] is governed by both the C++ and C standards and we should not be changing its behavior without some extra diagnostics about extensions (and, preferably, some sort of attempt to standardize the behavior with WG14/WG21).

Might be a compatible extension, though - to use a standard attribute in a non-standard context? (at least clang and gcc only warn on putting [[nodiscard]] in non-standard places, they don't error)

Do you have an appetite to talk to the GCC and standards folks?

Medium interest.

If not, then I think the safest bet is to only change the behavior for the [[clang::warn_unused_result]] and to leave the other forms alone for now.

Happy to go either way.

Is there an easy/recommended way to split these things up? Duplicate the records in the td/come up with separate names, etc?)

Fixes for a few other test cases (though I wonder if these tests are overconstrained - do we need to be testing the list of things this attribute can be applied to in so many places?)

If the semantics of the attribute are identical regardless of how it's spelled, then we probably don't need the tests all spread out in multiple files. However, I don't think there's an intention to keep all of the spellings with identical semantics, so the different tests might still make sense (but could perhaps be cleaned up).

Oh, one question: If we do move forward with this patch, how would we detect that the compiler supports this new use of warn_unused_result? (so that the feature detection macros in LLVM properly make the attribute a no-op unless we have a version of clang that supports it)

__has_[c|cpp]_attribute returns a value, so we'd likely wind up using that return value to distinguish between versions.

Hmm - what if we end up with different behavior for the different spellings of the attribute (as GCC does)? Can we check them separately?

Let me start off by saying: thanks, I think this is really useful functionality. As a ridiculously obvious example, Annex K has an integer type alias errno_t and it would incredibly handy to be able to mark that as [[nodiscard]] to strongly encourage checking for errors for functions that return that type (not that we support Annex K, but the general idea extends to any integer-based error type).

That said, there are multiple different ways to spell the same semantic attribute, and it's not clear to me that we want to change them all the same way.

[[clang::warn_unused_result]] is ours to do with as we please, so there's no issue changing that behavior.

__attribute__((warn_unused_result)) and [[gnu::warn_unused_result]] are governed by GCC and we try to be compatible with their behavior. GCC does not appear to support the attribute on typedefs from my testing, so we'd want to coordinate with the GCC folks to see if there's an appetite for the change.

FWIW, looks like we already diverge from GCC's behavior - GCC doesn't seem to support this attribute on types at all: https://godbolt.org/z/8YjqnE4cv (but does support [[nodiscard]] in that place)

Whomp whomp! :-(

[[nodiscard]] is governed by both the C++ and C standards and we should not be changing its behavior without some extra diagnostics about extensions (and, preferably, some sort of attempt to standardize the behavior with WG14/WG21).

Might be a compatible extension, though - to use a standard attribute in a non-standard context? (at least clang and gcc only warn on putting [[nodiscard]] in non-standard places, they don't error)

It's a bit unclear -- there's a list of things the attribute applies to, and typedef is not on the list, so it would be reasonable to think that means the attribute can't be written on anything else. But because the standard doesn't say what happens if you DO apply it to a typedef, perhaps that's sufficiently undefined to allow us to call it a conforming extension. Given that you're fine trying to get this standardized and it seems like it shouldn't be contentious, I think we aren't behaving badly if we accept [[nodiscard]] on a typedef so long as we give an extension warning.

Do you have an appetite to talk to the GCC and standards folks?

Medium interest.

If not, then I think the safest bet is to only change the behavior for the [[clang::warn_unused_result]] and to leave the other forms alone for now.

Happy to go either way.

Is there an easy/recommended way to split these things up? Duplicate the records in the td/come up with separate names, etc?)

Given that we already diverge from GCC for warn_unused_result, and because you're willing to give the standardization bit a shot and that seems highly likely to succeed, I say let's try to keep the same semantic effects for all of the spellings in terms of what the attribute does. If that's reasonable to everyone, then in SemaDeclAttr.cpp, when we see the standard spelling on a typedef declaration (using as well as typedef), we can issue a Clang extension warning on that particular use so that it's clear this is not yet standardized behavior for that spelling.

Btw, if you ever find yourself needing to distinguish between various spellings for the semantics of an attribute, you can use an Accessors list in the .td file (e.g., https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L665).

Fixes for a few other test cases (though I wonder if these tests are overconstrained - do we need to be testing the list of things this attribute can be applied to in so many places?)

If the semantics of the attribute are identical regardless of how it's spelled, then we probably don't need the tests all spread out in multiple files. However, I don't think there's an intention to keep all of the spellings with identical semantics, so the different tests might still make sense (but could perhaps be cleaned up).

Oh, one question: If we do move forward with this patch, how would we detect that the compiler supports this new use of warn_unused_result? (so that the feature detection macros in LLVM properly make the attribute a no-op unless we have a version of clang that supports it)

__has_[c|cpp]_attribute returns a value, so we'd likely wind up using that return value to distinguish between versions.

Hmm - what if we end up with different behavior for the different spellings of the attribute (as GCC does)? Can we check them separately?

__has_[c|cpp]_attribute() has you specify the full name of the attribute (including vendor namespace), so we can check them separately depending on the spelling. So we are able to make __has_cpp_attribute(nodiscard) return a different value from __has_cpp_attribute(clang::warn_unused_result) that's different from __has_cpp_attribute(gnu::warn_unused_result) and vary the return value depending on which one has what features. One potential issue is that it seems we return a rather silly nonzero value for the clang and gnu spellings: https://godbolt.org/z/3ajxdeWr1 so feature testing this could be a bit awkward. :-/