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

[Haojian Wu via Phabricator via cfe-commits]

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

`Sema::Act*` have a mixed usage of returning {*Result`, `void`, a pointer, `bool`}, unclear the common practice there. Changing `ActOnTemplatedFriendTag` and `ActOnTag` return-type seems be more aligned with the other `Sema::Act*` methods used in the `ParseClassSpecifier` context.  And these two APIs are not widely used, it is probably fine.

> (Aaron, this is your call, just wanted to make sure you're aware of the cost)

To me, I don't have a strong preference, I'm fine with either solution.



================
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,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141580