[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie marked an inline comment as done.
dblaikie added a comment.

In D102122#3550590 <https://reviews.llvm.org/D102122#3550590>, @aaron.ballman wrote:

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

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



================
Comment at: clang/include/clang/Basic/Attr.td:2945
                    C2x<"", "nodiscard", 201904>,
                    CXX11<"clang", "warn_unused_result">,
                    GCC<"warn_unused_result">];
----------------
aaron.ballman wrote:
> 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
Oh, so we'd have to put `C2x<"clang", "warn_unused_result">` in here? Fair enough, yeah, separate issue.


================
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;
----------------
aaron.ballman wrote:
> 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.
> 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).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102122