[clang][RecoveryExpr] Clarify the dependence-bits documentation.

[hokein]

Summary

The expr dependent-bits described that the expression somehow depends on a template paramter.

With RecoveryExpr, we have generalized it to "the expression depends on a template parameter, or an error". This patch updates/cleanups all related comments of dependence-bits.

[sammccall]

What's the rationale/motivation for this? I'd expect this to break a lot of subtle assumptions, because the invariant "nothing can be dependent without being instantiation-dependent" is a useful one.

It also doesn't seem correct from first principles. My mental model is:

• dependence describes code where some properties are unknown because a template wasn't instantiated yet
• we've generalized this to "some properties are unknown because we don't know what this piece of code is supposed to say"
• we can never know the value at compile-time even in contexts where we should, so RecoveryExpr is value-dependent
• we only know the type at compile-time if the RecoveryExpr's type is known and not itself dependent, so RecoveryExpr is usually value-dependent
• the code may intend to refer to entities valid only in certain instantiations (consider (void)f(T()) where f is an overloaded function). So RecoveryExpr is at least sometimes instantiation-dependent

At best I think we could say that the expr is only instantiation-dependent if it occurs in a template scope - otherwise there's no template we might fail to instantiate.

[hokein]

I think this depends on how we interpret the instantiation-dependent bit. In clang, it currently has two semantics:

1. an expression (somehow) depends on a template parameter;
2. an expression is dependent;

Prior to RecoveryExpression being added, 1 and 2 are equal, so we use isInstantiationDependent to check whether the code is dependent. This is fine.

However since we now generalize the "dependent" concept to mean "depends on a template parameter, or an error", checking isInstantiationDependent() is incomplete to determine the code is dependent.

what we could do?

Option 1 (what this patch does):

instantiation-dependence still implies the expression involves a template parameter -- if a recovery expression doesn't involves a template parameter, we don't set this flag

pros:

• avoid unnecessary instantiations if an AST node doesn't depend on template parameters but contain errorss;
• more precise semantics (maybe for better error-tracking, diagnostics), we can express a recovery-expr involves a template parameter (contains-errors + instantiation-dep), or not (contains-erros + no instantiation-dep)

cons:

• as you mentioned above, we violate the "nothing can be dependent if it is not instantiation dependent" assumption, which may leads us to fix a long tail of bugs (high effort), and the fix is basically adding containsErrors to where isInstantiaionDependent is used.

This makes me feel like we're going back to "use containsErrors everywhere" solution. Not sure what pros would buy us, maybe it is not worth the effort.

Option 2:

Keep it as it-is now, always set it, then the instantiation-dependent would mean "an expression depends on a template parameter or an error".

pros:

• everything would work without changing anything!

cons:

• keep using the instantiation name is confusing; we could do clarify the document of isInstantiation

While writing this, I'm starting to like the option2, any other thoughts?

[sammccall]

In D83213#2141387, @hokein wrote:

I think this depends on how we interpret the instantiation-dependent bit. In clang, it currently has two semantics:

1. an expression (somehow) depends on a template parameter;
2. an expression is dependent;

Not trying to be difficult, but I'm not sure what you mean by "dependent" in #2. Informally, it means the same thing as #1. Formally, C++ defines type-dependence and value-dependence for expressions, and the ABI defines instantiation-dependence for expressions - AFAIK "dependent" doesn't have a formal meaning.

So I'd rather say it has two definitions:

• informally, it (somehow) depends on a template parameter.
• formally (from the C++ABI), an expression is instantiation-dependent if it is type-dependent or value-dependent, or it has a subexpression that is type-dependent or value-dependent.

To preserve the linkage between these two, IMO we need to extend the formal definition to errors if and only if we extend the informal definition to errors. Accidentally unlinking the two is almost certain to result in subtle bugs as either the implementation subtly differs from the spec, or it subtly differs from the mental model.
That is, we need to decide whether to make all of these changes:

• (informal) instantiation-dependent means it (somehow) depends on a template parameter or error
• (formal) value-dependent means that (informally) an expressions's value depends on a tparam or err
• (formal) type-dependent means that (informally) an expression's type depends on a tparam or err

The approach so far has been YES. This looks like:

• all RecoveryExprs are value-dependent, instantiation-dependent, and contains-errors
• RecoveryExprs are type-dependent if we didn't guess the type, or we guessed it was a dependent type

Here we make use of existing codepaths that turn off checking in the presence of dependent code. We mostly need to check contains-errors to prevent a bad-recovery cascade.

The alternate approach is NO. This looks like:

• all RecoveryExpr are contains-errors
• a RecoveryExpr are instantiation-dependent if a subexpression is (i.e. refers to a template param)
• a RecoveryExpr is value-dependent if one of its direct subexpressions is, or if the broken code is likely to yield different values in different template instantiations due to the context
• a RecoveryExpr is type-dependent if its type is known to be dependent, or if the broken code is likely to yield different types in different template instantiations due to the context

Here codepaths that turn off checking need to turn it off for errors explicitly.
There's subtlety in the value-depnedent and type-dependent bits, which probably needs to be resolved on a case-by-case basis.
And there's more machinery needed: when an expression's type is not known but isn't believed to be dependent, what is it? I think we end up needing to define RecoveryType here (the fundamental contains-errors type).

So this is doable and maybe even a good idea (not obvious to me what the practical differences are). But it's a large change, not a small one like this patch.

Option 1 (what this patch does):

instantiation-dependence still implies the expression involves a template parameter -- if a recovery expression doesn't involves a template parameter, we don't set this flag

It doesn't make sense IMO to make this change without also saying "value-dependence implies the value dependds on a template parameter". It's too surprising and hard to reason about, and I don't see a principled reason for this distinction.

Not sure what pros would buy us

That's the thing that surprises me, it's a richer model, it's principled and correct... but I have no intuition what we can use it for.

Option 2:
cons:

• keep using the instantiation name is confusing; we could do clarify the document of isInstantiation

Yeah. I don't think actually renaming it would be appropriate, but we should be really explicit about its non-template meaning, and also give a generalized intuitive definition that makes sense. ("Depends in any way on a template parameter or error. In a template context, this implies that the resolution of this expr may cause instantiation to fail")

While writing this, I'm starting to like the option2, any other thoughts?

Yeah, I think we'd have to have some concrete benefits to pursue option 1. Doing it right is a lot of work.

[hokein]

Not trying to be difficult, but I'm not sure what you mean by "dependent" in #2. Informally, it means the same thing as #1. Formally, C++ defines type-dependence and value-dependence for expressions, and the ABI defines instantiation-dependence for expressions - AFAIK "dependent" doesn't have a formal meaning.

So I'd rather say it has two definitions:

informally, it (somehow) depends on a template parameter.
formally (from the C++ABI), an expression is instantiation-dependent if it is type-dependent or value-dependent, or it has a subexpression that is type-dependent or value-dependent.
To preserve the linkage between these two, IMO we need to extend the formal definition to errors if and only if we extend the informal definition to errors. Accidentally unlinking the two is almost certain to result in subtle bugs as either the implementation subtly differs from the spec, or it subtly differs from the mental model.

agree! I think the mental model you described makes more sense. yeah, it is *important* to keep the linkage between the informal and formal definition. It also explains why we should set instantiation-bit. Thanks!

ok, we have an agreement on the current approach. The only remaining thing is to clarify comments in clang. I will do it here (rather than D83215), so that this discussion thread can be found via the commit message.

[hokein]

Clarify the documentation for dependence-bits, and extend it to error.

[hokein]

more tweaks.

[sammccall]

clang/include/clang/AST/DependenceFlags.h

22

nit: I'd weaken usually->often

25

Maybe add ", and extend it to errors"

56

evenif > even if

63

nit: i'd just say "e.g. decltype(expr-with-errors)"

111–112

I'd rephrase as "Depends on a template parameter or error in some way. Validity depends on how the template is instantiated or the error is resolved."

clang/include/clang/AST/Expr.h

162

maybe "or an error, whose resolution is unknown"

This hints at the connection between template and error dependency, and also is more accurate for type-dependence (where sometimes we're not type dependent because the type depends on an error but we've guessed at what the resolution is)

6233

I'm not sure why this "in addition" is part of the same paragraph, it seems unrelated.

I'd move to a separate paragraph and drop "in addition".

6236

maybe "In this case, the expression is not type-dependent (unless the known type is itself dependent)"

clang/lib/AST/ComputeDependence.cpp

499

nit: I'd say "always value-dependent, and therefore instantiation dependent"
and make "contains-errors" a separate bullet at the end like "- contains errors (ExprDependence::Error), by definition"

500

opaque

502

hmm, I'd missed the type-dependent subexpressions question.
If there are type-dependent subexpressions, but a non-dependent type was specified for the RecoveryExpr, is the expr type-dependent?

This is the part that I think we have discretion over.
The definition of type-dependent does say "any type-dependent subexpression" but then lays out a list of exceptions such as casts, which are not type-dependent even if their argument is. What these have in common is that the type is known.

So I think this comes down to whether it's the caller's job to work this out, or we want to conservatively call these expressions dependent.

I think the former is probably better - marking the expression as type-dependent but not having its actual type be dependent doesn't serve any purpose I'm aware of. It's also inconsistent with the informal definition of type-dependence described earlier in this patch.

So the comment should describe the current state, but maybe a FIXME to remove the type-dependent subexpressions clause?

[hokein]

address comments.

[hokein]

clang/lib/AST/ComputeDependence.cpp

502

yeah, I think we should respect to actual type of RecoveryExpr, added a fixme.

clang/include/clang/AST/DependenceFlags.h

 #include "clang/Basic/BitmaskEnum.h"
 #include "llvm/ADT/BitmaskEnum.h"
 #include <cstdint>
 
 namespace clang {
 struct ExprDependenceScope {
   enum ExprDependence : uint8_t {
     UnexpandedPack = 1,
+    // This expr depends in any way on
+    //   - a template parameter, it implies that the resolution of this expr may
+    //     cause instantiation to fail
+    //   - or an error (often in a non-template context)
+    //
+    // Note that C++ standard doesn't define the instantiation-dependent term,
+    // we follow the formal definition coming from the Itanium C++ ABI, and
+    // extend it to errors.
     Instantiation = 2,
+    // The type of this expr depends on a template parameter, or an error.
     Type = 4,
+    // The value of this expr depends on a template parameter, or an error.
     Value = 8,
 
     // clang extension: this expr contains or references an error, and is
     // considered dependent on how that error is resolved.
     Error = 16,
 
     None = 0,
     All = 31,
 
     TypeValue = Type | Value,
     TypeInstantiation = Type | Instantiation,
     ValueInstantiation = Value | Instantiation,
     TypeValueInstantiation = Type | Value | Instantiation,
 
     LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Error)
   };
 };
 using ExprDependence = ExprDependenceScope::ExprDependence;
 
 struct TypeDependenceScope {
   enum TypeDependence : uint8_t {
     /// Whether this type contains an unexpanded parameter pack
     /// (for C++11 variadic templates)
     UnexpandedPack = 1,
-    /// Whether this type somehow involves a template parameter, even
-    /// if the resolution of the type does not depend on a template parameter.
+    /// Whether this type somehow involves
+    ///   - a template parameter, even if the resolution of the type does not
+    ///     depend on a template parameter.
+    ///   - or an error.
     Instantiation = 2,
-    /// Whether this type is a dependent type (C++ [temp.dep.type]).
+    /// Whether this type
+    ///   - is a dependent type (C++ [temp.dep.type])
+    ///   - or it somehow involves an error, e.g. decltype(recovery-expr)
     Dependent = 4,
     /// Whether this type is a variably-modified type (C99 6.7.5).
     VariablyModified = 8,
 
     /// Whether this type references an error, e.g. decltype(err-expression)
     /// yields an error type.
     Error = 16,
 
     None = 0,

 // A combined space of all dependence concepts for all node types.
 // Used when aggregating dependence of nodes of different types.
 class Dependence {
 public:
   enum Bits : uint8_t {
     None = 0,
 
     // Contains a template parameter pack that wasn't expanded.
     UnexpandedPack = 1,
-    // Uses a template parameter, even if it doesn't affect the result.
-    // Validity depends on the template parameter.
+    // Depends on a template parameter or an error in some way.
+    // Validity depends on how the template is instantiated or the error is
+    // resolved.
     Instantiation = 2,
-    // Expression type depends on template context.
+    // Expression type depends on template context, or an error.
     // Value and Instantiation should also be set.
     Type = 4,
-    // Expression value depends on template context.
+    // Expression value depends on template context, or an error.
     // Instantiation should also be set.
     Value = 8,
-    // Depends on template context.
+    // Depends on template context, or an error.
     // The type/value distinction is only meaningful for expressions.
     Dependent = Type | Value,
     // Includes an error, and depends on how it is resolved.
     Error = 16,
     // Type depends on a runtime value (variable-length array).
     VariablyModified = 32,
 
     LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/VariablyModified)

clang/include/clang/AST/Expr.h

void setType(QualType t) {
 
     TR = t;
   }
 
   ExprDependence getDependence() const {
     return static_cast<ExprDependence>(ExprBits.Dependent);
   }
 
-  /// isValueDependent - Determines whether this expression is
-  /// value-dependent (C++ [temp.dep.constexpr]). For example, the
-  /// array bound of "Chars" in the following example is
+  /// Determines whether the value of this expression depends on
+  ///   - a template parameter (C++ [temp.dep.constexpr])
+  ///   - or an error, whose resolution is unknown
+  ///
+  /// For example, the array bound of "Chars" in the following example is
   /// value-dependent.
   /// @code
   /// template<int Size, char (&Chars)[Size]> struct meta_string;
   /// @endcode
   bool isValueDependent() const {
     return static_cast<bool>(getDependence() & ExprDependence::Value);
   }
 
-  /// isTypeDependent - Determines whether this expression is
-  /// type-dependent (C++ [temp.dep.expr]), which means that its type
-  /// could change from one template instantiation to the next. For
-  /// example, the expressions "x" and "x + y" are type-dependent in
+  /// Determines whether the type of this expression depends on
+  ///   - a template paramter (C++ [temp.dep.expr], which means that its type
+  ///     could change from one template instantiation to the next)
+  ///   - or an error
+  ///
+  /// For example, the expressions "x" and "x + y" are type-dependent in
   /// the following code, but "y" is not type-dependent:
   /// @code
   /// template<typename T>
   /// void add(T x, int y) {
   ///   x + y;
   /// }
   /// @endcode
   bool isTypeDependent() const {
     return static_cast<bool>(getDependence() & ExprDependence::Type);
   }
 
   /// Whether this expression is instantiation-dependent, meaning that
-  /// it depends in some way on a template parameter, even if neither its type
-  /// nor (constant) value can change due to the template instantiation.
+  /// it depends in some way on
+  ///    - a template parameter (even if neither its type nor (constant) value
+  ///      can change due to the template instantiation)
+  ///    - or an error
   ///
   /// In the following example, the expression \c sizeof(sizeof(T() + T())) is
   /// instantiation-dependent (since it involves a template parameter \c T), but
   /// is neither type- nor value-dependent, since the type of the inner
   /// \c sizeof is known (\c std::size_t) and therefore the size of the outer
   /// \c sizeof is known.
   ///
   /// \code
   /// template<typename T>
   /// void f(T x, T y) {
   ///   sizeof(sizeof(T() + T());
   /// }
   /// \endcode
   ///
+  /// \code
+  /// void func(int) {
+  ///   func(); // the expression is instantiation-dependent, because it depends
+  ///           // on an error.
+  /// }
+  /// \endcode
   bool isInstantiationDependent() const {
     return static_cast<bool>(getDependence() & ExprDependence::Instantiation);
   }
 
   /// Whether this expression contains an unexpanded parameter
   /// pack (for C++11 variadic templates).
   ///
   /// Given the following function template:

 /// fails, we cannot produce a BinaryOperator expression. Instead, we can choose
 /// to produce a recovery expression storing left and right operands.
 ///
 /// RecoveryExpr does not have any semantic meaning in C++, it is only useful to
 /// preserve expressions in AST that would otherwise be dropped. It captures
 /// subexpressions of some expression that we could not construct and source
 /// range covered by the expression.
 ///
-/// By default, RecoveryExpr is type-, value- and instantiation-dependent to
-/// take advantage of existing machinery to deal with dependent code in C++,
-/// e.g. RecoveryExpr is preserved in `decltype(<broken-expr>)` as part of the
-/// `DependentDecltypeType`. In addition to that, clang does not report most
-/// errors on dependent expressions, so we get rid of bogus errors for free.
-/// However, note that unlike other dependent expressions, RecoveryExpr can be
-/// produced in non-template contexts.
-/// In addition, we will preserve the type in RecoveryExpr when the type is
-/// known, e.g. preserving the return type for a broken non-overloaded function
-/// call, a overloaded call where all candidates have the same return type.
+/// By default, RecoveryExpr uses dependence-bits to take advantage of existing
+/// machinery to deal with dependent code in C++, e.g. RecoveryExpr is preserved
+/// in `decltype(<broken-expr>)` as part of the `DependentDecltypeType`. In
+/// addition to that, clang does not report most errors on dependent
+/// expressions, so we get rid of bogus errors for free. However, note that
+/// unlike other dependent expressions, RecoveryExpr can be produced in
+/// non-template contexts.
+///
+/// We will preserve the type in RecoveryExpr when the type is known, e.g.
+/// preserving the return type for a broken non-overloaded function call, a
+/// overloaded call where all candidates have the same return type. In this
+/// case, the expression is not type-dependent (unless the known type is itself
+/// dependent)
 ///
 /// One can also reliably suppress all bogus errors on expressions containing
 /// recovery expressions by examining results of Expr::containsErrors().
+///
+/// FIXME: RecoveryExpr is currently generated by default in C++ mode only, as
+/// dependence isn't handled properly on several C-only codepaths.
 class RecoveryExpr final : public Expr,
                            private llvm::TrailingObjects<RecoveryExpr, Expr *> {
 public:
   static RecoveryExpr *Create(ASTContext &Ctx, QualType T,
                               SourceLocation BeginLoc, SourceLocation EndLoc,
                               ArrayRef<Expr *> SubExprs);
   static RecoveryExpr *CreateEmpty(ASTContext &Ctx, unsigned NumSubExprs);
 

clang/lib/AST/ComputeDependence.cpp

ExprDependence clang::computeDependence(DeclRefExpr *E, const ASTContext &Ctx) {
   //      -  a member function or a static data member of the current
   //         instantiation
   if (isa<CXXMethodDecl>(Decl) && Decl->getDeclContext()->isDependentContext())
     Deps |= ExprDependence::ValueInstantiation;
   return Deps;
 }
 
 ExprDependence clang::computeDependence(RecoveryExpr *E) {
-  // Mark the expression as value- and instantiation- dependent to reuse
-  // existing suppressions for dependent code, e.g. avoiding
-  // constant-evaluation.
-  // FIXME: drop type+value+instantiation once Error is sufficient to suppress
-  // bogus dianostics.
+  // RecoveryExpr is
+  //   - always value-dependent, and therefore instantiation dependent
+  //   - contains errors (ExprDependence::Error), by definition
+  //   - type-dependent if we don't know the type (fallback to an opaque
+  //     dependent type), or the type is known and dependent, or it has
+  //     type-dependent subexpressions.
   auto D = toExprDependence(E->getType()->getDependence()) |
            ExprDependence::ValueInstantiation | ExprDependence::Error;
+  // FIXME: remove the type-dependent bit from subexpressions, if the
+  // RecoveryExpr has a non-dependent type.
   for (auto *S : E->subExpressions())
     D |= S->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(PredefinedExpr *E) {
   return toExprDependence(E->getType()->getDependence()) &
          ~ExprDependence::UnexpandedPack;

clang/lib/Sema/SemaExpr.cpp

else if (getCurBlock() || getCurLambda())
     getCurFunction()->HasPotentialAvailabilityViolations = true;
 
   return new (Context)
       ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy);
 }
 
 ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
                                     ArrayRef<Expr *> SubExprs, QualType T) {
-  // FIXME: enable it for C++, RecoveryExpr is type-dependent to suppress
-  // bogus diagnostics and this trick does not work in C.
-  // FIXME: use containsErrors() to suppress unwanted diags in C.
   if (!Context.getLangOpts().RecoveryAST)
     return ExprError();
 
   if (isSFINAEContext())
     return ExprError();
 
   if (T.isNull() || !Context.getLangOpts().RecoveryASTType)
     // We don't know the concrete type, fallback to dependent type.
     T = Context.DependentTy;
   return RecoveryExpr::Create(Context, T, Begin, End, SubExprs);
 }