[clang] WIP: Improved Context Declaration tracking #107942

mizvekov · 2024-09-10T00:38:16Z

This patch aims to improve how parenting relationships are represented in the AST.

Currently regular declarations can only be children of special AST nodes which inherit from DeclContext, except for a few special cases which are required by itanium mangling.

Having this parenting relationship well described helps in tracking which entities are dependent, and an improvement here would allow us to drop a lot of workarounds and to get some difficult cases right, such as lambdas and requires expressions appearing within return types and function parameter types of templated functions.

This patch extends the ContextDecl tracking which currently is used for mangling, in order to cover almost needed cases.

Template type aliases represent a specially difficult case which is addressed by this patch.
They can be contexts for other declarations, but besides not being DeclContexts, they also lack a declaration which represents their specialization.

This patch addresses the type alias context problem by storing the specialization arguments along with the ContextDecl for entities declared in their context.

TODO:

Move away from using ExpressionEvaluationContexts, and use the same stack used for tracking the CurContext. Remove the separate ContextDecl field, and have that functionally built in to the base Decl class, so that we don't separatelly track both a parent Decl and a parent DeclContext.
A function type has ParmVarDecls, and these can appear inside aliases and template type aliases in particular, so a ParmVarDecl needs a ContextDecl too.

This patch aims to improve how parenting relationships are represented in the AST. Currently regular declarations can only be children of special AST nodes which inherit from DeclContext, except for a few special cases which are required by itanium mangling. Having this parenting relationship well described helps in tracking which entities are dependent, and an improvement here would allow us to drop a lot of workarounds and to get some difficult cases right. This patch extends the ContextDecl tracking which currently is used for mangling, in order to cover almost needed cases. Template type aliases represent a specially difficult case which is addressed by this patch. They can be contexts for other declarations, but besides not being DeclContexts, they also lack a declaration which represents their specialization. This patch addresses the type alias context problem by storing the specialization arguments along with the ContextDecl for entities declared in their context. TODO: * Move away from using ExpressionEvaluationContexts, and use the same stack used for tracking the CurContext. Remove the separate ContextDecl field, and have that functionally built in to the base Decl class, so that we don't separatelly treack both a parent Decl and a parent DeclContext. * A function type has ParmVarDecls, and these can appear inside aliases and template type aliases in particular, so a ParmvarDecl needs a ContextDecl too.

zyn0217 · 2024-09-10T05:37:46Z

clang/lib/Sema/TreeTransform.h

-  // so early, e.g. because we want to see if any of the *substituted*
-  // parameters are dependent.
-  DependencyKind = getDerived().ComputeLambdaDependency(&LSICopy);
-  Class->setLambdaDependencyKind(DependencyKind);


So after the removal of such, lines 14911 - 14914 would also be useless.

zyn0217

Thanks. Just some thoughts/questions in my mind, and I may need more time to understand the PR.

clang/include/clang/AST/DeclCXX.h

zyn0217 · 2024-09-10T06:09:01Z

clang/lib/Parse/ParseDecl.cpp

@@ -6291,7 +6307,8 @@ bool Parser::isDeclarationSpecifier(
 bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide,
                                     DeclSpec::FriendSpecified IsFriend,
                                     const ParsedTemplateInfo *TemplateInfo) {
-  RevertingTentativeParsingAction TPA(*this);
+  RevertingTentativeParsingAction TPA(*this, /*Unannotated=*/true);
+  Sema::SFINAETrap Trap(Actions);


Why do we need a SFINAETrap here?

To suppress a bunch of diagnostics that would otherwise be duplicated when we parse the thing again for real.

zyn0217 · 2024-09-10T14:46:38Z

clang/lib/Sema/SemaConcept.cpp

@@ -961,6 +961,7 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(

  if (MLTAL.getNumSubstitutedLevels() == 0)
    return ConstrExpr;
+  MLTAL.setKind(TemplateSubstitutionKind::Rewrite);


I presume this avoids creating Subst* nodes for the substitution, right?
Could you explain what we would gain from / the difference it makes this way?

I remember I had tried using RewriteKind while fixing the mismatched constraint issue. But that turned out to cause some regressions/crashes in the end. Don't know what kind of situation it would be with this patch.

The new difference introduced in this patch is that a Rewrite is depth preserving, while a specialization is not.

In this case, we want to rewrite a requires clause from another potential redeclaration to see if it matches, and confirm it really is a redeclaration.

If we don't preserve the depth here, this would trip a new assert, where we would be instantiating a requires clause at template depth 0, but then parenting it to a declaration at depth 1, which is incorrect.

zyn0217 · 2024-09-10T14:49:21Z

clang/lib/Sema/SemaDecl.cpp

+    if (auto *TD = dyn_cast<FunctionTemplateDecl>(D))
+      D = TD->getTemplatedDecl();
+    else
+      assert(isa<FunctionDecl>(D));


Maybe

assert((isa<FunctionDecl, FunctionTemplateDecl>(D))); if (auto *FTD = dyn_cast<>) ...

?

zyn0217 · 2024-09-10T14:53:52Z

clang/lib/Sema/SemaExpr.cpp

+static auto skipRequiresBody(DeclContext *DC) {
+  while (DC->isRequiresExprBody())
+    DC = DC->getParent();
+  return DC;
+}
+


We could move it to tryCaptureVariable, inlining it as a lambda to avoid such small functions.

zyn0217 · 2024-09-10T14:59:44Z

clang/lib/Sema/SemaExpr.cpp

-  // Exception: Function parameters are not tied to the function's DeclContext
-  // until we enter the function definition. Capturing them anyway would result
-  // in an out-of-bounds error while traversing DC and its parents.
-  if (isa<ParmVarDecl>(Var) && !VarDC->isFunctionOrMethod())


I think the rationale behind it is that every ParmVarDecl now has a context decl pointing to e.g. its function? Do we still need to set the ParmVarDecl's context separately (by calling setOwningFunction) now?
Sorry I haven't gotten around to the changes on VarDecls yet.

We still do need to reparent them the old way, but this is going to change once I give them a ContextDecl too.

The difference right now is that we don't create the ParmVarDecls always at the translation unit context anymore, we just create them at the CurContext.

zyn0217 · 2024-09-10T15:01:22Z

clang/lib/Sema/SemaExpr.cpp

@@ -19000,7 +19056,7 @@ bool Sema::tryCaptureVariable(
    Explicit = false;
    FunctionScopesIndex--;
    if (IsInScopeDeclarationContext)
-      DC = ParentDC;
+      DC = skipRequiresBody(ParentDC);


Hmm, do you have a test for it? e.g. the requires expression contains a lambda that contains another requires expression which would cause a crash for now.

That is one of your tests actually:

clang/test/SemaCXX/lambda-unevaluated.cpp

// GH88081: Test if we evaluate the requires expression with lambda captures properly. namespace GH88081 { // Test that ActOnLambdaClosureQualifiers() is called only once. void foo(auto value) requires requires { [&] -> decltype(value) {}; } // expected-error@-1 {{non-local lambda expression cannot have a capture-default}} {}

The difference is that now the requires expression is properly reparented to foo, where as before it would just sit at the namespace.

zyn0217 · 2024-09-10T15:02:13Z

clang/lib/Sema/SemaExpr.cpp

-          ExprEvalContexts.back().ManglingContextDecl)) {
+  if (auto ContextDecl = currentEvaluationContext().ContextDecl;
+      auto *VD =
+          dyn_cast_or_null<VarDecl>(ContextDecl ? *ContextDecl : nullptr)) {


dyn_cast_if_present, if we have to touch it anyway :)

zyn0217 · 2024-10-15T09:12:55Z

clang/lib/Sema/SemaExpr.cpp

+  const auto &PrevRec = ExprEvalContexts.back();
+  PushExpressionEvaluationContext(NewContext, PrevRec.ContextDecl,
+                                  PrevRec.ContextArgs, ExprContext);
+  ExprEvalContexts.back().HasReusedDeclContext = true;
+  ExprEvalContexts.back().LazyContextDeclPos = PrevRec.LazyContextDeclPos;


FYI, PrevRec would be invalidated after PushExpressionEvaluationContext(), so you probably want to save the value before the call.

This caused clang/test/SemaTemplate/stack-exhaustion.cpp to fail on my machine.

Yeah, I saw this failure on CI, but not on my machine (MacOS).

Thanks for taking a look.

zyn0217

I took another pass through the code, and it looks good overall.

Given the scale of this patch, input from others would be valuable. It would be nice if you could move the patch out of draft status to get more visibility :)

zyn0217 · 2024-12-06T06:54:07Z

clang/include/clang/Sema/Sema.h

@@ -7946,6 +7980,8 @@ class Sema final : public SemaBase {
  /// A stack of expression evaluation contexts.
  SmallVector<ExpressionEvaluationContextRecord, 8> ExprEvalContexts;

+  SmallVector<Decl *, 8> PendingLazyContextDecls;


So from a quick reading, PendingLazyContextDecls is used for lambdas created as part of other declarations, such as being part of a default argument for a template parameter. Their context declarations are updated only at the end of the evaluation context or right after the outer declaration is created.

I think we'd better document its intent, wdyt?

The intention is that they are used for any declarations where its parent is created later. Sure will document this.

zyn0217 · 2024-12-06T07:37:04Z

clang/lib/Sema/SemaLambda.cpp

+      ContextDecl.hasValue() ? *ContextDecl
+                             : ContextDeclOrSentinel(TemplateDepth),
+      currentEvaluationContext().ContextArgs);
+  if (!ContextDecl.hasValue())


I find it more likely for one to conflate the semantics of hasValue() with those of operator bool whereas the latter actually indicates whether this is in a null state (i.e. neither the pointer nor the depth is assigned).

For clarity, could we rename it to something more descriptive, like hasDeclPointer()?

zyn0217 · 2024-12-06T07:40:29Z

clang/lib/Sema/SemaTemplate.cpp

+    else if (!(isa<FunctionDecl>(D) || isa<VarDecl>(D) ||
+               isa<VarTemplateSpecializationDecl>(D))) {
+      D->dumpColor();
+      llvm_unreachable("FU");


Debugging leftover, will remove.

zyn0217 · 2024-12-06T07:59:57Z

clang/lib/Sema/SemaExpr.cpp

+  assert(Rec.LazyContextDeclPos <= PendingLazyContextDecls.size());
+  if (!Rec.HasReusedDeclContext) {
+    if (Rec.ContextDecl.hasValue()) {
+      assert(Rec.LazyContextDeclPos == PendingLazyContextDecls.size());


Is this an assumption for that

if the lambda (or something else we want to later add context decl for) is aware of its parent context decl at the point of its creation, then there should not be any pointer in PendingLazyContextDecls tracking the lambda.

the subsequent on-stack evaluation contexts should have handled PendingLazyContextDecls properly

Yeah that's basically it. If we have already created the parent, we shouldn't be using the Pending list at all.
If this originally started as a lazy context, then we are in a state where we know the parent, so we should have already handled and emptied the lazy list, and shouldn't be adding new stuff to it anymore.

zyn0217 · 2024-12-06T08:30:58Z

clang/lib/Sema/SemaExpr.cpp

+    } else {
+      while (PendingLazyContextDecls.size() > Rec.LazyContextDeclPos) {
+        Decl *D = PendingLazyContextDecls.pop_back_val();
+        setContextDecl(*this, D, nullptr);


Is there anything special for the third argument being null? Both setLambdaContextDecl() and handleLambdaNumbering() would bail out for such cases, right?

This is the error path. Basically we had a bunch of declarations waiting for their parents to be created, but for some reason we bailed out and we are never going to create their parents anymore.

Before this patch, these declarations would have stayed parented to their parent's parent.

These declarations would never make it to codegen, so don't matter much except for analysis.
The upside is that by staying as lazycontext entities, we don't lose track of their template depth.

zyn0217 · 2025-02-12T12:14:15Z

clang/lib/Parse/ParseExprCXX.cpp

+  LocalInstantiationScope InstScope(getActions());
+  for (ParmVarDecl *D : LocalParameterDecls)
+    InstScope.InstantiatedLocal(D, D);


Where would we use that?

zyn0217 · 2025-03-14T07:42:27Z

A quick note that we want to test with this patch

Default non-type template parameters with immediately invoked lambdas fail with deduction guides #51315
Clang: Failed assertion in nested instantiation #127421
[Clang] Crash when lambda requires clause uses a parameter declared in an outer requires expression #126550
[Clang] constexpr variables wrongfully treated as captures in certain lambdas #120503
[Clang] Crash when generic lambda is used as argument to a concept in type constraint for template parameter pack #131481
clang++ 19.1.7 segfault in isPackExpansion() #124471

mizvekov · 2025-03-14T19:40:23Z

Noted. Sorry I haven't responded yet, I have other priorities and it might take a while before I get back to this patch.

mizvekov requested a review from zygoloid September 10, 2024 00:38

mizvekov self-assigned this Sep 10, 2024

zyn0217 reviewed Sep 10, 2024

View reviewed changes

mizvekov mentioned this pull request Oct 7, 2024

Crash when passing a lambda to function in decltype, or no viable conversion from lambda to itself #111283

Open

zyn0217 mentioned this pull request Oct 8, 2024

Wrong error about not capturing an already local variable #61029

Open

zyn0217 linked an issue Oct 8, 2024 that may be closed by this pull request

Wrong error about not capturing an already local variable #61029

Open

mizvekov mentioned this pull request Oct 10, 2024

Error on use of parameter in lambda outside function body #111840

Closed

zyn0217 reviewed Oct 15, 2024

View reviewed changes

zyn0217 mentioned this pull request Dec 6, 2024

[clang][regression] lambda call in unevaluated context in generic lambda in template function fails from clang 15 onwards #114755

Open

zyn0217 linked an issue Dec 6, 2024 that may be closed by this pull request

[clang][regression] lambda call in unevaluated context in generic lambda in template function fails from clang 15 onwards #114755

Open

zyn0217 reviewed Dec 6, 2024

View reviewed changes

This was linked to issues Dec 6, 2024

[clang] crash on valid in isPackExpansion #116036

Open

ICE on function with decltype(lambda()) argument type #95669

Open

[clang] lambda with template parameter crashed in unevaluated context #88104

Open

cor3ntin mentioned this pull request Jan 9, 2025

[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857

Merged

cor3ntin self-requested a review January 14, 2025 20:28

cor3ntin mentioned this pull request Jan 18, 2025

[Clang] constexpr variables wrongfully treated as captures in certain lambdas #120503

Open

zyn0217 reviewed Feb 12, 2025

View reviewed changes

zyn0217 mentioned this pull request Feb 12, 2025

[Clang] Crash when lambda requires clause uses a parameter declared in an outer requires expression #126550

Open

mizvekov mentioned this pull request Feb 19, 2025

Clang: Failed assertion in nested instantiation #127421

Open

shafik mentioned this pull request Mar 20, 2025

[Clang] Crash when generic lambda is used as argument to a concept in type constraint for template parameter pack #131481

Open

mizvekov mentioned this pull request Mar 31, 2025

Assertion failure in LocalInstantiationScope::findInstantiationOf: `isa<LabelDecl>(D) && "declaration not instantiated in this scope" #133792

Open

[clang] WIP: Improved Context Declaration tracking #107942

Are you sure you want to change the base?

[clang] WIP: Improved Context Declaration tracking #107942

Conversation

mizvekov commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyn0217 commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mizvekov commented Mar 14, 2025

Uh oh!

Uh oh!

mizvekov commented Sep 10, 2024 •

edited

Loading

zyn0217 commented Mar 14, 2025 •

edited

Loading