Support warn_unused_result on typedefs

[dblaikie]

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.

[dblaikie]

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]

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?)

[aaron.ballman]

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.

In D102122#2746426, @dblaikie wrote:

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).

In D102122#2746424, @dblaikie wrote:

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.

[dblaikie]

In D102122#2748206, @aaron.ballman wrote:

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?)

In D102122#2746426, @dblaikie wrote:

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).

In D102122#2746424, @dblaikie wrote:

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?

[aaron.ballman]

In D102122#2748271, @dblaikie wrote:

In D102122#2748206, @aaron.ballman wrote:

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).

In D102122#2746426, @dblaikie wrote:

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).

In D102122#2746424, @dblaikie wrote:

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. :-/

[dblaikie]

I don't recall all the context, but did try discussing this with the committee folks and got a variety of strong opinions/wasn't sure whether there was a path forward: https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those with access to that). What's your take on the discussion there? Worth pushing forward?

& some minor questions I guess I wrote last year...

In D102122#2748529, @aaron.ballman wrote:

In D102122#2748271, @dblaikie wrote:

In D102122#2748206, @aaron.ballman wrote:

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.

Fair enough - does the spec say what happens if you use a completely unknown spelling like [[foobar]]? I guess the spec reserves that for future attributes, so implementations aren't meant to add support in there (implementations being expected to use some namespace to put their extensions in)

Just curiious.

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.

Sure enough - I'll add a warning.

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).

In D102122#2746426, @dblaikie wrote:

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).

In D102122#2746424, @dblaikie wrote:

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. :-/

Not sure I'm following here. If we enable the new behavior for all the spellings - I guess we could pick a new, higher value for __has_cpp_attribute to return, which would let us detect the newer functionality? Hoping that our high value is higher than a value GCC returns?

& the "silly" nonzero value is silly because it's year/month, rather than some kind of version? or year/month/day?

[dblaikie]

In D102122#3344317, @dblaikie wrote:

I don't recall all the context, but did try discussing this with the committee folks and got a variety of strong opinions/wasn't sure whether there was a path forward: https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those with access to that). What's your take on the discussion there? Worth pushing forward?

Ping on this

[aaron.ballman]

Thank you for the ping, this fell entirely off my radar!

In D102122#3344317, @dblaikie wrote:

I don't recall all the context, but did try discussing this with the committee folks and got a variety of strong opinions/wasn't sure whether there was a path forward: https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those with access to that). What's your take on the discussion there? Worth pushing forward?

The C++ side was definitely mixed review, so I'm on the fence about whether it's worth pushing forward in C++. However, I think the story in C is rather different because there are less reasonable alternatives there and the type system is far less complex. I think it's reasonable to experiment with support in C only under [[clang::warn_unused_result]], keeping in mind some of the WG21 feedback in mind regarding questions like behavior of typedefs of typedef types, etc, if that's of interest to you.

The story with [[nodiscard]] is a bit more complex -- I would expect this attribute to appear in header files shared between C and C++, so I think it would be bad for it to be applicable to typedefs in only one language but not the other. However, so long as we issue a pedantic warning that the [[nodiscard]] attribute on a typedef is a Clang extension, I think we could technically do it. But it might not be worth it given the lack of portability; at that point, the user can use [[clang::warn_unused_result]] to the same effect.

& some minor questions I guess I wrote last year...

In D102122#2748529, @aaron.ballman wrote:

In D102122#2748271, @dblaikie wrote:

In D102122#2748206, @aaron.ballman wrote:

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.

Fair enough - does the spec say what happens if you use a completely unknown spelling like [[foobar]]? I guess the spec reserves that for future attributes, so implementations aren't meant to add support in there (implementations being expected to use some namespace to put their extensions in)

Just curiious.

It's allowed with implementation-defined semantics (per https://eel.is/c++draft/dcl.attr#grammar-6.sentence-1) but we'd be evil, bad folks and I'd be opposed to it as attribute code owner due to it stepping on the WG21 and WG14 design space for attributes (per https://eel.is/c++draft/dcl.attr#grammar-6.sentence-3).

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.

Sure enough - I'll add a warning.

The advice here is somewhat different on the question of spellings, now that we have more information from WG21.

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).

In D102122#2746426, @dblaikie wrote:

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).

In D102122#2746424, @dblaikie wrote:

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. :-/

Not sure I'm following here. If we enable the new behavior for all the spellings - I guess we could pick a new, higher value for __has_cpp_attribute to return, which would let us detect the newer functionality?

Correct.

Hoping that our high value is higher than a value GCC returns?

We'd likely want to coordinate with GCC in some form or fashion rather than rely on hope, but yes, this part is a bit fragile.

& the "silly" nonzero value is silly because it's year/month, rather than some kind of version? or year/month/day?

Yeah, silly that we return the same year/month value for all spellings. The standard spelling requires that year/month format, but the vendor attributes could use a version number. Or a bitmask of supported entities you can apply the attribute to, or whatever.

[dblaikie]

Rebase

[dblaikie]

Separate out clang:warn_unused_result so typedef support can be added to only that spelling.

This has one lingering issue (because it still currently instantiates both the clang:warn_unused_result as the same *Attr class as the others - and so SemaCXX/cxx11-attr-print.cpp is failing because the AST printing for the clang spelling decides to use the C++ standard spelling, I guess because the non-clang Attr class doesn't realize it was spelled with the clang spelling)

[dblaikie]

Sorry, with all the layers in the previous messages I'm not sure what the summary is. Sounds like the summary is "this is OK to continue work in the direction of supporting [[clang::warn_unused_result]] (only that spelling) for typedefs in C and C++ (for compatibility with C/so that C headers, like LLVM C's APIs can still be parsed as C++), while considering some of the complications raised in the WG21 discussion"?

OK, so I updated this patch with a very first draft/attempt at that - I wasn't sure how to add typedef support to only the clang spelling - so I created a new attribute name in the .td file (is there a better/more suitable way?) and that seems to have created one follow-on issue (partly because of my other choice: Clang still only /instantiates WarnUnusedResultAttr class, not the new WarnUnusedResultClangAttr class - even for the clang spelling, it uses the non-clang attribute class, so /most/ of the code looking at the attribute class type continues to work) - that issue is in SemaCXX/cxx11-attr-print.cpp it doesn't print the attribute spelling correctly, it chooses the C++ standard spelling when trying to reproduce the Clang spelling, I guess because the ClangAttr class isn't used. (but of course if I change the code to actually instantiate the WarnUnusedResultClangAttr class - then all the existing code that handles the attribute class/implements the warning has to be updated to test both attribute classes... )

Is there another way I should be approaching this?

(oh, and there was something about a supported version number for the has attribute checking - where would that factor into all this?)

[rsmith]

clang/include/clang/Basic/Attr.td

2943

Should we allow this new behavior for the GNU attribute spelling too? (Not [[gnu::..]] but __attribute__((warn_unused_result))). We don't generally avoid extending the meaning of __attribute__((...)) compared to GCC.

clang/lib/AST/Expr.cpp

1525–1527

This should loop over TypedefType sugar; the attribute could be in a nested typedef.

[aaron.ballman]

In D102122#3538668, @dblaikie wrote:

Sorry, with all the layers in the previous messages I'm not sure what the summary is. Sounds like the summary is "this is OK to continue work in the direction of supporting [[clang::warn_unused_result]] (only that spelling) for typedefs in C and C++ (for compatibility with C/so that C headers, like LLVM C's APIs can still be parsed as C++), while considering some of the complications raised in the WG21 discussion"?

Yup! Though I agree with Richard's later comment that we could maybe also extend the __attribute__((warn_unused_result)) spelling as well.

OK, so I updated this patch with a very first draft/attempt at that - I wasn't sure how to add typedef support to only the clang spelling - so I created a new attribute name in the .td file (is there a better/more suitable way?)

I would not go that route. Instead, I'd add it as a subject to the existing attribute, and then add a spelling-based restriction in handleWarnUnusedResult() to give an appropriate diagnostic if the subject is wrong based on the spelling used. Something like:

if ((!AL.isGNUAttributeSyntax() || !(AL.isStandardAttributeSyntax() && AL.isClangScope())) && isa<TypedefNameDecl>(D))
  Diag(...);

(Note, you may need to extend ParsedAttr somewhat, I don't recall if we have all those helper functions explicitly.)

and that seems to have created one follow-on issue (partly because of my other choice: Clang still only /instantiates WarnUnusedResultAttr class, not the new WarnUnusedResultClangAttr class - even for the clang spelling, it uses the non-clang attribute class, so /most/ of the code looking at the attribute class type continues to work) - that issue is in SemaCXX/cxx11-attr-print.cpp it doesn't print the attribute spelling correctly, it chooses the C++ standard spelling when trying to reproduce the Clang spelling, I guess because the ClangAttr class isn't used. (but of course if I change the code to actually instantiate the WarnUnusedResultClangAttr class - then all the existing code that handles the attribute class/implements the warning has to be updated to test both attribute classes... )

Is there another way I should be approaching this?

I'd try to stick with just the one attribute definition in Attr.td, that should resolve a lot of these issues.

(oh, and there was something about a supported version number for the has attribute checking - where would that factor into all this?)

That's likely to get involved to address. Some of the Spelling objects have a version field you can set (like CXX11 and C2x spellings); that's the value returned by __has_cpp_attribute and __has_c_attribute. We don't have such a field for the GNU spelling and __has_attribute, or the GCC and Clang spellings. Adding one would involve going into ClangAttrEmitter.cpp and updating the tablegen code to support it.

I'd say we should prove that we like the feature to ourselves, and worry about the version number either later in this patch or in a follow-up.

clang/include/clang/Basic/Attr.td

2943

I'd be okay with that.

[dblaikie]

Use a single attribute, with a filter in the sema parsing code

[dblaikie]

In D102122#3539812, @aaron.ballman wrote:

In D102122#3538668, @dblaikie wrote:

Sorry, with all the layers in the previous messages I'm not sure what the summary is. Sounds like the summary is "this is OK to continue work in the direction of supporting [[clang::warn_unused_result]] (only that spelling) for typedefs in C and C++ (for compatibility with C/so that C headers, like LLVM C's APIs can still be parsed as C++), while considering some of the complications raised in the WG21 discussion"?

Yup! Though I agree with Richard's later comment that we could maybe also extend the __attribute__((warn_unused_result)) spelling as well.

OK, so I updated this patch with a very first draft/attempt at that - I wasn't sure how to add typedef support to only the clang spelling - so I created a new attribute name in the .td file (is there a better/more suitable way?)

I would not go that route. Instead, I'd add it as a subject to the existing attribute, and then add a spelling-based restriction in handleWarnUnusedResult() to give an appropriate diagnostic if the subject is wrong based on the spelling used. Something like:

if ((!AL.isGNUAttributeSyntax() || !(AL.isStandardAttributeSyntax() && AL.isClangScope())) && isa<TypedefNameDecl>(D))
  Diag(...);

(Note, you may need to extend ParsedAttr somewhat, I don't recall if we have all those helper functions explicitly.)

Ah, OK. Added those (& I think the first || was meant to be a && there, then it seems to do what's expected)

How's this look now, then?

One drawback is the built-in attribute warnings that mention which entities the attribute is valid on will mention typedefs even when the user uses a spelling that wouldn't be valid there - so the user might try it there, then get the follow-on warning (totally open to wordsmithing that warning, too - just placeholder words - also made it part of the same group as the warn_unused_result warning itself - if you disable that warning, then presumably you don't care about the warning being in the wrong place either, to some extent).

[aaron.ballman]

In D102122#3544655, @dblaikie wrote:

How's this look now, then?

I think it's heading in the right direction.

One drawback is the built-in attribute warnings that mention which entities the attribute is valid on will mention typedefs even when the user uses a spelling that wouldn't be valid there - so the user might try it there, then get the follow-on warning (totally open to wordsmithing that warning, too - just placeholder words - also made it part of the same group as the warn_unused_result warning itself - if you disable that warning, then presumably you don't care about the warning being in the wrong place either, to some extent).

Yeah, that is a drawback -- we don't have a spelling-specific set of subjects currently. I had a plan to address that at some point by introducing more data into the subject list so you could do something like:

def WarnUnusedResult : InheritableAttr {
  let Spellings = [CXX11<"", "nodiscard", 201907>,
                   C2x<"", "nodiscard", 201904>,
                   CXX11<"clang", "warn_unused_result">,
                   GCC<"warn_unused_result">];
  let Subjects = SubjectList<
                           SubjectsFor<[ObjCMethod, Enum, Record, FunctionLike, TypedefName], [GNU<"warn_unused_result">, CXX11<"clang", "warn_unused_result">]>,
                           SubjectsFor<[ObjCMethod, Enum, Record, FunctionLike], [CXX11<"", "nodiscard">, C2x<"", "nodiscard">, CXX11<"gnu", "warn_unused_result">]>,
                         >;
  ...
}

But.... that's complicated (as this attribute demonstrates!). This is a case where we want HALF of a spelling to apply to some subjects (the GNU spelling half of the GCC spelling) and the other half to apply to other subjects (the C++ spelling half of the GCC spelling).

I think for right now, I think it's okay for us to have the slightly degraded diagnostic behavior. If it turns into user confusion in practice, we can improve the diagnostics at that point. WDYT?

clang/include/clang/Basic/Attr.td

2945

Oops, it looks like we support this spelling in C++ but not in C (not your bug, just an observation): https://godbolt.org/z/1hPq6coba

clang/include/clang/Basic/DiagnosticSemaKinds.td

8738–8739

It's a bit more wordy, but I think it's more clear as well. Also, this makes it more clear that the attribute is being ignored (and groups it under that grouping as well).

(Definitely need to adjust for 80 col limits.)

clang/lib/AST/Expr.cpp

1525–1527

I agree with Richard here. I think this is needed for a case like:

typedef int foo __attribute__((warn_unused_result));
typedef foo foob;

foob func();

int main() {
  func(); // Still want to be barked at here
}

But this example brings up an interesting question of expectations. What's your intuition on how this should behave?

typedef int foo __attribute__((warn_unused_result));
typedef foo *foo_ptr;

foo_ptr func();

int main() {
  func(); // Bark? Silence? Other?
}

FWIW, my initial inclination was that this would diagnose the call to func() but now I'm second-guessing that, hence the question about type composition in a typedef.

clang/test/SemaCXX/warn-unused-result.cpp

259

I think you should also have a test for [[gnu::warn_unused_result]] to show that we didn't touch the behavior of that vendor-specific spelling.

[dblaikie]

Check through multiple typedef layers to find the warn_unused_result attribute
Rephrase unsupported syntax warning

[dblaikie]

Fix/update the unsupported syntax warning

[dblaikie]

In D102122#3550590, @aaron.ballman wrote:

In D102122#3544655, @dblaikie wrote:

How's this look now, then?

I think it's heading in the right direction.

One drawback is the built-in attribute warnings that mention which entities the attribute is valid on will mention typedefs even when the user uses a spelling that wouldn't be valid there - so the user might try it there, then get the follow-on warning (totally open to wordsmithing that warning, too - just placeholder words - also made it part of the same group as the warn_unused_result warning itself - if you disable that warning, then presumably you don't care about the warning being in the wrong place either, to some extent).

Yeah, that is a drawback -- we don't have a spelling-specific set of subjects currently. I had a plan to address that at some point by introducing more data into the subject list so you could do something like:

def WarnUnusedResult : InheritableAttr {
  let Spellings = [CXX11<"", "nodiscard", 201907>,
                   C2x<"", "nodiscard", 201904>,
                   CXX11<"clang", "warn_unused_result">,
                   GCC<"warn_unused_result">];
  let Subjects = SubjectList<
                           SubjectsFor<[ObjCMethod, Enum, Record, FunctionLike, TypedefName], [GNU<"warn_unused_result">, CXX11<"clang", "warn_unused_result">]>,
                           SubjectsFor<[ObjCMethod, Enum, Record, FunctionLike], [CXX11<"", "nodiscard">, C2x<"", "nodiscard">, CXX11<"gnu", "warn_unused_result">]>,
                         >;
  ...
}

But.... that's complicated (as this attribute demonstrates!). This is a case where we want HALF of a spelling to apply to some subjects (the GNU spelling half of the GCC spelling) and the other half to apply to other subjects (the C++ spelling half of the GCC spelling).

I think for right now, I think it's okay for us to have the slightly degraded diagnostic behavior. If it turns into user confusion in practice, we can improve the diagnostics at that point. WDYT?

Oh, yeah, should've said I'm OK with it if you are. Only meant to point it out to get confirmation there. Thanks!

clang/include/clang/Basic/Attr.td

2945

Oh, so we'd have to put C2x<"clang", "warn_unused_result"> in here? Fair enough, yeah, separate issue.

clang/lib/AST/Expr.cpp

1525–1527

I agree with Richard here. I think this is needed for a case like:

typedef int foo __attribute__((warn_unused_result));
typedef foo foob;

foob func();

int main() {
 func(); // Still want to be barked at here
}

Oh, sorry, I marked this as "done" because I'd done this - but hadn't updated the patch. Should be updated now, including a test case (warn-unused-result.cpp:unused_typedef_result::indirect).

But this example brings up an interesting question of expectations. What's your intuition on how this should behave?

typedef int foo __attribute__((warn_unused_result));
typedef foo *foo_ptr;

foo_ptr func();

int main() {
  func(); // Bark? Silence? Other?
}

FWIW, my initial inclination was that this would diagnose the call to func() but now I'm second-guessing that, hence the question about type composition in a typedef.

Yeah, I'm OK with/think that probably shouldn't warn - like instantiating a template with a warn_unused_result doesn't/shouldn't designate that template specialization as warn_unused_result - it's a related/derived type, but doesn't necessarily have the same properties (I guess another example in that direction: a function pointer, where one of the parameters/result type are warn_unused_result doesn't make the function pointer type itself warn_unused_result).

clang/test/SemaCXX/warn-unused-result.cpp

259

Ah, sure - done!

[aaron.ballman]

LGTM!

clang/include/clang/Basic/Attr.td

2945

Yeah, I can take care of that one pretty quickly once your changes land (so we don't step on each other's toes). Or if you want to tackle it, I'm fine with that as well.

clang/lib/AST/Expr.cpp

1525–1527

Yeah, I'm OK with/think that probably shouldn't warn - like instantiating a template with a warn_unused_result doesn't/shouldn't designate that template specialization as warn_unused_result - it's a related/derived type, but doesn't necessarily have the same properties (I guess another example in that direction: a function pointer, where one of the parameters/result type are warn_unused_result doesn't make the function pointer type itself warn_unused_result).

SGTM, thanks for thinking it through with me!

clang/lib/Sema/SemaDeclAttr.cpp

3159

This looks like it goes over the usual 80-col limit, so you should probably format this when landing.

clang/include/clang/Basic/Attr.td

 def WarnUnused : InheritableAttr {
   let Spellings = [GCC<"warn_unused">];
   let Subjects = SubjectList<[Record]>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
 def WarnUnusedResult : InheritableAttr {
   let Spellings = [CXX11<"", "nodiscard", 201907>,
                    C2x<"", "nodiscard", 201904>,
                    CXX11<"clang", "warn_unused_result">,
                    GCC<"warn_unused_result">];
-  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike]>;
+  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike, TypedefName]>;
   let Args = [StringArgument<"Message", 1>];
   let Documentation = [WarnUnusedResultsDocs];
   let AdditionalMembers = [{
     // Check whether this the C++11 nodiscard version, even in non C++11
     // spellings.
     bool IsCXX11NoDiscard() const {
       return this->getSemanticSpelling() == CXX11_nodiscard;
     }

clang/include/clang/Basic/AttributeCommonInfo.h

public:
   /// with surrounding underscores removed as appropriate (e.g.
   /// __gnu__::__attr__ will be normalized to gnu::attr).
   std::string getNormalizedFullName() const;
 
   bool isDeclspecAttribute() const { return SyntaxUsed == AS_Declspec; }
   bool isMicrosoftAttribute() const { return SyntaxUsed == AS_Microsoft; }
 
   bool isGNUScope() const;
+  bool isClangScope() const;
 
   bool isAlignasAttribute() const {
     // FIXME: Use a better mechanism to determine this.
     return getParsedKind() == AT_Aligned && isKeywordAttribute();
   }
 
   bool isCXX11Attribute() const {
     return SyntaxUsed == AS_CXX11 || isAlignasAttribute();
   }
 
   bool isC2xAttribute() const { return SyntaxUsed == AS_C2x; }
 
   /// The attribute is spelled [[]] in either C or C++ mode, including standard
   /// attributes spelled with a keyword, like alignas.
   bool isStandardAttributeSyntax() const {
     return isCXX11Attribute() || isC2xAttribute();
   }
 
+  bool isGNUAttribute() const { return SyntaxUsed == AS_GNU; }
+
   bool isKeywordAttribute() const {
     return SyntaxUsed == AS_Keyword || SyntaxUsed == AS_ContextSensitiveKeyword;
   }
 
   bool isContextSensitiveKeywordAttribute() const {
     return SyntaxUsed == AS_ContextSensitiveKeyword;
   }
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_side_effects_typeid : Warning<
   "expression with side effects will be evaluated despite being used as an "
   "operand to 'typeid'">, InGroup<PotentiallyEvaluatedExpression>;
 def warn_unused_result : Warning<
   "ignoring return value of function declared with %0 attribute">,
   InGroup<UnusedResult>;
 def warn_unused_result_msg : Warning<
   "ignoring return value of function declared with %0 attribute: %1">,
   InGroup<UnusedResult>;
+def warn_unused_result_typedef_unsupported_spelling : Warning<
+  "'[[%select{nodiscard|gnu::warn_unused_result}0]]' attribute ignored when "
+  "applied to a typedef; consider using '__attribute__((warn_unused_result))' "
+  "or '[[clang::warn_unused_result]]' instead">, InGroup<IgnoredAttributes>;
 def warn_unused_volatile : Warning<
   "expression result unused; assign into a variable to force a volatile load">,
   InGroup<DiagGroup<"unused-volatile-lvalue">>;
 
 def ext_cxx14_attr : Extension<
   "use of the %0 attribute is a C++14 extension">, InGroup<CXX14Attrs>;
 def ext_cxx17_attr : Extension<
   "use of the %0 attribute is a C++17 extension">, InGroup<CXX17Attrs>;

clang/lib/AST/Expr.cpp

 
 const Attr *CallExpr::getUnusedResultAttr(const ASTContext &Ctx) const {
   // If the return type is a struct, union, or enum that is marked nodiscard,
   // then return the return type attribute.
   if (const TagDecl *TD = getCallReturnType(Ctx)->getAsTagDecl())
     if (const auto *A = TD->getAttr<WarnUnusedResultAttr>())
       return A;
 
+  for (const auto *TD = getCallReturnType(Ctx)->getAs<TypedefType>(); TD;
+       TD = TD->desugar()->getAs<TypedefType>())
+    if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>())
+      return A;
+
   // Otherwise, see if the callee is marked nodiscard and return that attribute
   // instead.
   const Decl *D = getCalleeDecl();
   return D ? D->getAttr<WarnUnusedResultAttr>() : nullptr;
 }
 
 SourceLocation CallExpr::getBeginLoc() const {
   if (isa<CXXOperatorCallExpr>(this))

clang/lib/Basic/Attributes.cpp

static StringRef normalizeAttrName(const IdentifierInfo *Name,
 
   return AttrName;
 }
 
 bool AttributeCommonInfo::isGNUScope() const {
   return ScopeName && (ScopeName->isStr("gnu") || ScopeName->isStr("__gnu__"));
 }
 
+bool AttributeCommonInfo::isClangScope() const {
+  return ScopeName && (ScopeName->isStr("clang") || ScopeName->isStr("_Clang"));
+}
+
 #include "clang/Sema/AttrParsedAttrKinds.inc"
 
 static SmallString<64> normalizeName(const IdentifierInfo *Name,
                                      const IdentifierInfo *Scope,
                                      AttributeCommonInfo::Syntax SyntaxUsed) {
   StringRef ScopeName = normalizeAttrScopeName(Scope, SyntaxUsed);
   StringRef AttrName = normalizeAttrName(Name, ScopeName, SyntaxUsed);
 

clang/lib/Sema/SemaDeclAttr.cpp

if (AL.getNumArgs() == 1) {
       // extension.
       // FIXME: C2x should support this feature as well, even as an extension.
       if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, nullptr))
         return;
     } else if (LO.CPlusPlus && !LO.CPlusPlus17)
       S.Diag(AL.getLoc(), diag::ext_cxx17_attr) << AL;
   }
 
+  if ((!AL.isGNUAttribute() &&
+       !(AL.isStandardAttributeSyntax() && AL.isClangScope())) &&
+      isa<TypedefNameDecl>(D)) {
+    S.Diag(AL.getLoc(), diag::warn_unused_result_typedef_unsupported_spelling)
+        << AL.isGNUScope();
+    return;
+  }
+
   D->addAttr(::new (S.Context) WarnUnusedResultAttr(S.Context, AL, Str));
 }
 
 static void handleWeakImportAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   // weak_import only applies to variable & function declarations.
   bool isDef = false;
   if (!D->canBeWeakImported(isDef)) {
     if (isDef)

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp

 // RUN: %clang_cc1 -fsyntax-only -std=c++2a -verify %s
 
 struct [[nodiscard]] S1 {}; // ok
 struct [[nodiscard, nodiscard]] S2 {}; // ok
 struct [[nodiscard("Wrong")]] S3 {};
 
 [[nodiscard]] int f();
 enum [[nodiscard]] E {};
 
-namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: TargetClones (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
-// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportModule (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportName (SubjectMatchRule_function)
 // CHECK-NEXT: WorkGroupSizeHint (SubjectMatchRule_function)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: ZeroCallUsedRegs (SubjectMatchRule_function)
 // CHECK-NEXT: End of supported attributes.

clang/test/Sema/c2x-nodiscard.c

struct [[nodiscard("Wrong")]] S3 { // FIXME: may need an extension warning.
   int i;
 };
 
 struct S3 get_s3(void);
 
 [[nodiscard]] int f1(void);
 enum [[nodiscard]] E1 { One };
 
-[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 struct [[nodiscard]] S4 {
   int i;
 };
 struct S4 get_s(void);
 
 enum [[nodiscard]] E2 { Two };
 enum E2 get_e(void);

clang/test/Sema/unused-expr.c

int t6(void) {
   fn1();  // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
   fn2(92, 21);  // expected-warning {{ignoring return value of function declared with pure attribute}}
   fn3(42);  // expected-warning {{ignoring return value of function declared with const attribute}}
   __builtin_abs(0); // expected-warning {{ignoring return value of function declared with const attribute}}
   (void)0, fn1();  // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
   return 0;
 }
 
-int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 // PR4010
 int (*fn4)(void) __attribute__ ((warn_unused_result));
 void t8(void) {
   fn4(); // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
 }
 
 void t9(void) __attribute__((warn_unused_result)); // expected-warning {{attribute 'warn_unused_result' cannot be applied to functions without return value}}

clang/test/SemaCXX/warn-unused-result.cpp

 
 namespace PR45520 {
 [[nodiscard]] bool (*f)(); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 [[clang::warn_unused_result]] bool (*g)();
 __attribute__((warn_unused_result)) bool (*h)();
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *a;
+typedef a indirect;
+a af1();
+indirect indirectf1();
+void af2() {
+  af1(); // expected-warning {{ignoring return value}}
+  void *(*a1)();
+  a1(); // no warning
+  a (*a2)();
+  a2(); // expected-warning {{ignoring return value}}
+  indirectf1(); // expected-warning {{ignoring return value}}
+}
+[[nodiscard]] typedef void *b1; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
+[[gnu::warn_unused_result]] typedef void *b2; // expected-warning {{'[[gnu::warn_unused_result]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
+b1 b1f1();
+b2 b2f1();
+void bf2() {
+  b1f1(); // no warning
+  b2f1(); // no warning
+}
+__attribute__((warn_unused_result)) typedef void *c;
+c cf1();
+void cf2() {
+  cf1(); // expected-warning {{ignoring return value}}
+  void *(*c1)();
+  c1();
+  c (*c2)();
+  c2(); // expected-warning {{ignoring return value}}
+}
+}