[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D102122#3544655 <https://reviews.llvm.org/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?



================
Comment at: clang/include/clang/Basic/Attr.td:2945
                    C2x<"", "nodiscard", 201904>,
                    CXX11<"clang", "warn_unused_result">,
                    GCC<"warn_unused_result">];
----------------
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


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8733-8734
+def warn_unused_result_typedef_unsupported_spelling : Warning<
+  "warn_unused_result on typedefs only supported with gnu or clang scoped "
+  "standard spelling">, InGroup<UnusedResult>;
 def warn_unused_volatile : Warning<
----------------
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.)


================
Comment at: clang/lib/AST/Expr.cpp:1525-1527
+  if (const auto *TD = getCallReturnType(Ctx)->getAs<TypedefType>())
+    if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>())
+      return A;
----------------
rsmith wrote:
> This should loop over `TypedefType` sugar; the attribute could be in a nested typedef.
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.


================
Comment at: clang/test/SemaCXX/warn-unused-result.cpp:259
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *a;
+a af1();
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122