This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Use of decltype(capture) in parameter-declaration-clause
ClosedPublic

Authored by cor3ntin on Apr 17 2022, 5:47 AM.

Details

Summary

Partially implement the proposed resolution to CWG2569.

D119136 broke some libstdc++ code, as P2036R3, implemented as a DR to
C++11 made ill-formed some previously valid and innocuous code.

We resolve this issue to allow decltype(x) - but not decltype((x)
to appear in the parameter list of a lambda that capture x by copy.

Unlike CWG2569, we do not extend that special treatment to
sizeof/noexcept yet, as the resolution has not been approved yet
and keeping the review small allows a quicker fix of impacted code.

Diff Detail

Event Timeline

cor3ntin created this revision.Apr 17 2022, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2022, 5:47 AM
cor3ntin requested review of this revision.Apr 17 2022, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2022, 5:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Apr 18 2022, 4:36 AM
clang/include/clang/Sema/Sema.h
5281

This parameter is always false, so it seems like it can be dropped.

clang/lib/Parse/ParseExprCXX.cpp
688–694
700

This feels surprisingly restrictive to me, but it's sufficient for getting libstdc++ working again.

713–715

In the case we get an error from parsing the unqualified id, I don't think we want to immediately try to parse an expression, right? Seems like a case to return an ExprError

aaron.ballman added reviewers: erichkeane, Restricted Project.Apr 18 2022, 4:37 AM
cor3ntin added inline comments.Apr 18 2022, 4:51 AM
clang/lib/Parse/ParseExprCXX.cpp
700

I've can't think of another kind of unqualified-id that could refer to a capturable entity here.
Do you know what would be missing?
Also, without being very restrictive, I'm not sure how this is implementable

cor3ntin added inline comments.Apr 18 2022, 5:05 AM
clang/include/clang/Sema/Sema.h
5281

Not if we extend to sizeof id - I guess i can drop it for now?

cor3ntin updated this revision to Diff 423363.Apr 18 2022, 5:25 AM
cor3ntin marked an inline comment as done.

Address Aaron's feedback

  • Typos
  • Remove unused parameters
  • return ExprError() if ParseUnqualifiedId fails.
aaron.ballman added inline comments.Apr 18 2022, 5:29 AM
clang/include/clang/Sema/Sema.h
5281

I'd drop for now -- we can add it in later when we get around to doing the full core issue once it's settled.

clang/lib/Parse/ParseExprCXX.cpp
700

What I find restrictive is that it requires an unqualified-id *only* to kick in as being maybe mutable agnostic. e.g., decltype(id) and decltype(+id) (where the unary + may be an overloaded operator that depends on the mutability of the this object).

(I'm not seeing the principle that guides this restriction proposed in the core issue yet, but those can be hashed out later.)

We should probably add a release note that says we partially implement CWG 2569 (just the decltype portion) and that the core issue has not yet been approved by WG21 but the changes are needed to keep libstdc++ working.

clang/lib/Parse/ParseExprCXX.cpp
710

Sorry, I forgot to mention this before, but there should be some test coverage for this case to make sure the behavior is still reasonable. Something like:

void whatever() {
  [=]<typename T = decltype(b)>(){};
}

(I'd expect we get some sort of lookup error for b.)

cor3ntin added inline comments.Apr 18 2022, 5:39 AM
clang/lib/Parse/ParseExprCXX.cpp
700

I do not think there is a general answer to "given these captures and this arbitrarily complex expression", would the mutability of the lambda affect the declaration of the parameters.
In this case we know it doesn't and that happen to fix existing code, which make the solution tractable.
If core was keen to add any more complexity here, I think they should consider mandating a look-ahead of the mutable keyword even if that sounds rather complex. I don't think it will come to that, the proposed resolution will be good enough to reduce the blast radius of the paper, and I don't see a strong motivation to add even more complexity here.

aaron.ballman added inline comments.Apr 18 2022, 5:48 AM
clang/lib/Parse/ParseExprCXX.cpp
700

Eh, I'm less convinced. The changes in P2036R3 were a band-aid to fix mistakes. The core issue "fixing" it is another band-aid over the top of P2036R3. My experience is that band-aids over band-aids rarely leads to a good feature that users can make sense of. The fact that I can't make heads or tails of what the actual language design rule is here also means this is painful for us supporting our own extensions (like alignof on an object instead of a type, __typeof__ instead of decltype, etc).

Personally, I'd prefer the core issue was resolved in a more clean fashion than carving out a few exceptions under the hope that it's "good enough". However, that's something for us to fight about in Core when this comes up. For the moment, I'm happy enough with this because it gets libstdc++ unstuck.

cor3ntin added inline comments.Apr 18 2022, 6:01 AM
clang/lib/Parse/ParseExprCXX.cpp
700

The changes in P2036R3 were a band-aid to fix mistakes. The core issue "fixing" it is another band-aid over the top of P2036R3.

Agreed, but, the question is how many people will run into scenario not covered by the band-aid in existing code?
And this is why I'm not a fan of generalizing to sizeof/noexcept because it' sounds arbitrary and unmotivated, and if we have more than one bandaid we should consider a general solution.

Basically either that or mutable look-ahead, i don't see an in-between solution as being viable.

1 nit, otherwise no comments.

clang/lib/Sema/SemaExpr.cpp
2703

I would probably prefer a RAII object for this bool. It makes it easier later on if we decide this is something that needs to be enabled/disabled in a stack.

cor3ntin updated this revision to Diff 423365.Apr 18 2022, 6:15 AM
  • Add a release note
  • Add tests for lookup errors in decltype expressions in lambda parameters clause
cor3ntin updated this revision to Diff 423367.Apr 18 2022, 6:33 AM
  • Use a RAII class to control InMutableAgnosticContext
  • Formatting
clang/lib/Sema/SemaExpr.cpp
2703

Agreed, it's better

LGTM aside from a nit (forgot to remove a line covered by RAII).

clang/lib/Sema/SemaExpr.cpp
2704–2712
This revision is now accepted and ready to land.Apr 18 2022, 6:36 AM
cor3ntin updated this revision to Diff 423368.Apr 18 2022, 6:40 AM

Thanks Aaron, let's pretend this did not happen :)

cor3ntin updated this revision to Diff 423370.Apr 18 2022, 6:42 AM

... and we don't need a local variable anymore either

This revision was landed with ongoing or failed builds.Apr 18 2022, 6:58 AM
This revision was automatically updated to reflect the committed changes.

@aaron.ballman @erichkeane Thanks for the review - I've landed it. Hopefully the remediation should be enough so that libstdc++ is no longer affected but we should still keep an eye out for further issues

hokein added a subscriber: hokein.Apr 20 2022, 5:49 AM

Hi, this patch seems to break the following code which was previously compiled:

#include <type_traits>
#include <algorithm>
#include <numeric>

template <typename It, typename MapFn>
auto MapJoin(It first, It last, MapFn map_fn) {
  return std::accumulate(
      first, last, map_fn(*first),
      [=](typename std::result_of<MapFn(decltype(*first))>::type result) {  }); // a new diagnostic: error: captured variable 'first' cannot appear here
}

Hi, this patch seems to break the following code which was previously compiled:

#include <type_traits>
#include <algorithm>
#include <numeric>

template <typename It, typename MapFn>
auto MapJoin(It first, It last, MapFn map_fn) {
  return std::accumulate(
      first, last, map_fn(*first),
      [=](typename std::result_of<MapFn(decltype(*first))>::type result) {  }); // a new diagnostic: error: captured variable 'first' cannot appear here
}

This looks like a true positive to me, at least for the moment (Core is still trying to decide what to do about CWG2569 which may relax some restrictions in this area).

@MaskRay -- your revert was incorrect, please un-revert.

rsmith added a subscriber: rsmith.Apr 20 2022, 12:13 PM

This looks like a true positive to me, at least for the moment (Core is still trying to decide what to do about CWG2569 which may relax some restrictions in this area).

@MaskRay -- your revert was incorrect, please un-revert.

It sounds to me like this change is causing a substantial amount of breakage for real code, which might suggest that applying it retroactively by default is not in the best interests of our users. I think we need more information here on the scope of the breakages, but it might make sense to suggest that CWG reconsiders treating this as a DR.

This looks like a true positive to me, at least for the moment (Core is still trying to decide what to do about CWG2569 which may relax some restrictions in this area).

@MaskRay -- your revert was incorrect, please un-revert.

It sounds to me like this change is causing a substantial amount of breakage for real code, which might suggest that applying it retroactively by default is not in the best interests of our users. I think we need more information here on the scope of the breakages, but it might make sense to suggest that CWG reconsiders treating this as a DR.

Based on the amount of code broken thus far, I tend to agree that P2036R3 should not be a DR (or perhaps retracted entirely for C++23 until these issues are addressed by the paper author). But I agree that we need more information on the scope of the breakages, which is why I was sad to see all of this reverted without prior discussion.