[PATCH] D141580: [clang] Don't consider a nullptr returned from ActOnTag() as a valid result in ParseClassSpecifier.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141580#4052496 <https://reviews.llvm.org/D141580#4052496>, @sammccall wrote:

> To offer the opposing argument: if DeclResult is just a bad idea, then using it consistently/right might be worse than using it as little as possible.
>
> FWIW, I find every piece of code that produces/consumes the `*Result` to be extremely confusing (the semantics of the two different sentinel values are never obvious) and use of the type in more APIs means more callers have to deal with this tri-state logic and bugs like this one.
>
> So spreading DeclResult around where it's not strictly needed feels like the wrong direction to be, and i find the original version of the patch easier to understand. It may even be possible to switch entirely to pointers here.
>
> (Aaron, this is your call, just wanted to make sure you're aware of the cost)

FWIW, I also find the interface to be horribly confusing and would love to replace it with something better at some point. But we're horribly inconsistent with things at the moment (some return a pointer, some return a bool, some return a result, etc), so my thinking is that any amount of unification is probably a good thing if only because it makes it more likely we can (consistently) swap to something better in the future.



================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2069-2078
+    if (auto *TagDecl = Actions.ActOnTag(
+            getCurScope(), TagType, TUK, StartLoc, SS, Name, NameLoc, attrs, AS,
+            DS.getModulePrivateSpecLoc(), TParams, Owned, IsDependent,
+            SourceLocation(), false, clang::TypeResult(),
+            DSC == DeclSpecContext::DSC_type_specifier,
+            DSC == DeclSpecContext::DSC_template_param ||
+                DSC == DeclSpecContext::DSC_template_type_arg,
----------------
hokein wrote:
> aaron.ballman wrote:
> > hokein wrote:
> > > hokein wrote:
> > > > aaron.ballman wrote:
> > > > > sammccall wrote:
> > > > > > just so I understand:
> > > > > > 
> > > > > > initial state of TagOrTempResult is (null, invalid)
> > > > > > if ActOnTag returns ptr, we set it to (ptr, valid)
> > > > > > if ActOnTag returns null, then the old code would set it to (null, valid) and the new code would leave it as (null, invalid)
> > > > > > 
> > > > > > ActOnTag returns null in error cases (diagnostic has been emitted).
> > > > > > We check validity below when deciding whether to use the returned pointer (so we're passing null around at that point).
> > > > > > 
> > > > > > I can't really infer what the invariants here were originally meant to be, and obviously we're not crashing all over the place, and the diagnostics look like a wash to me. But the new logic seems much more sane.
> > > > > I think this is the wrong approach to fixing the issue. None of the other assignments to `TagOrTempResult` test for nullptr first, such as: https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037 and https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2029.
> > > > > 
> > > > > An `ActionResult` can be both valid/invalid *and* usable/unusable (and a `DeclResult` is a kind of `ActionResult`). When it's assigned *any pointer value* (including nullptr), it's a valid `ActionResult` but it's not a usable `ActionResult` (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Ownership.h#L166).
> > > > > 
> > > > > I think the correct fix is to find the places assuming a valid `ActionResult` means a nonnull pointer from `get()`, such as https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2121 and switch them over to looking for a usable `ActionResult` instead.
> > > > Thanks for the comment!
> > > > 
> > > > > None of the other assignments to TagOrTempResult test for nullptr first
> > > > 
> > > > I think the problem here is:
> > > > - most of the `ActOn*` methods return a `DeclResult` (and inside these method implementations, they return an Invalid `DeclResult` if the Decl is nullptr), so `TagOrTempResult = Actions.ActOn*` is a fine pattern.
> > > > - Only two exceptions `ActOnTagDecl` and `ActOnTemplatedFriendTag`, they return a `Decl*`, so the `TagOrTempResult = Action.ActOn*` is a very suspicious pattern. (IMO, they're not intentional, likely bugs).
> > > > 
> > > > > such as: https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037 and https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2029.
> > > > 
> > > > The first one is the other exception; the second one is not (ActOnExplicitInstantiation returns a `DeclResult`).
> > > > 
> > > > > I think the correct fix is to find the places assuming a valid ActionResult means a nonnull pointer from get(), such as https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2121 and switch them over to looking for a usable ActionResult instead.
> > > > 
> > > > This is one of the solutions. But instead of justifying every places of DeclResult, I think we should fix the two exception places (by adding the nullptr check in
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037 and https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2069), what do you think?
> > > I have updated the patch to check nullptr for ActOnTemplatedFriendTag as well. Let me know what you think (I'm also happy to do the change).
> > Actually, I think the best solution is to make those APIs return a correctly constructed (invalid) `DeclResult` rather than a `Decl *` that turns into a valid-but-unusable `DeclResult`. I tried the change out locally and it seems to have the same fallout as your changes with only a small amount of extra work involved.
> > 
> > I have this done locally, would you like me to go ahead and clean it up to commit it or hand a patch off to you to finish? Or do you think that's not a good approach?
> > Actually, I think the best solution is to make those APIs return a correctly constructed (invalid) DeclResult rather than a Decl * that turns into a valid-but-unusable DeclResult. I tried the change out locally and it seems to have the same fallout as your changes with only a small amount of extra work involved.
> 
> Yeah, I considered this approach, but I was not convinced myself is significantly better than the current approach, and it requires some more changes.
> 
> > I have this done locally, would you like me to go ahead and clean it up to commit it or hand a patch off to you to finish? Or do you think that's not a good approach?
> 
> Thanks, since you already have a patch, please go ahead (if you think it is better). I'm happy to review.
heh, yeah, it's hard to know what the right approach is -- I suspect anything other than what we're doing today is a good step forward. I'll go ahead and make this change; it's straightforward enough I think post-commit review should suffice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141580