[clang] Fix a null-NSS-access crash in DependentNameType.

[hokein]

Summary

The DependentNameType must have a non-null NSS. This property could be
violated during typo correction.

[sammccall]

clang/lib/Sema/SemaTemplate.cpp

4869

two concerns here:

• it's not clear to me whether it's best to recover from this failed invariant or make a change elsewhere to ensure it holds
• IIUC we should only return true if an error has been emitted. We've got some suggestion that it already has, but no smoking gun - maybe there are cases we've missed. (The failure mode here is clang exits with an error status but prints no errors)

clang/test/Parser/cxx-template-decl.cpp

296

If I understand right, AddObservationFn gets typo-corrected to AddObservation (the function). When we go to check whether it's valid, we look up the name and get the type instead (injected-class-name). so we figure "missing typename" but actually everything's just totally confused.

The "best" solution I guess would be not allowing typo-correction to the shadowed name.
But I think the next best thing is just not to allow "insert typename and recover" if the expression we're inserting around already has errors - this seems like we have low confidence at this point.

So does requiring !Arg.getAsExpr().containsErrors() in the check for inserting typename work?
My hope is this would result in "undeclared identifier AddObservationFn, did you mean AddObservation" ... "template argument must be a type".
(Which is "wrong" in the sense that AddObservation is a type, but that bug is in the typo-correction logic, not here)

[hokein]

refine the fix.

[hokein]

clang/lib/Sema/SemaTemplate.cpp

4869

yeah, the current fix is not quite ideal.

thinking more about this, I think we should not recover if the arg is DeclRefExpr, reasons:

• looks like most important cases are covered by DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr already (no failing tests, though the test coverage is weak)
• if the arg is a DeclRefExpr (no typo correction), we'll never into this branch - the lookup should find the responding ValueDecl, which is not a TypeDecl
• if the arg is a DeclRefExpr (from typo correction), we might run into this branch, but we're less certain, and it might violate the assumption (NSS must be non-null). dropping it doesn't seem bad.

clang/test/Parser/cxx-template-decl.cpp

296

If I understand right, AddObservationFn gets typo-corrected to AddObservation (the function). When we go to check whether it's valid, we look up the name and get the type instead (injected-class-name). so we figure "missing typename" but actually everything's just totally confused.

Yes, exactly. The inconsistence between different decls resolved by typo correction (function) and the name lookup (class) leads to the crash.

The "best" solution I guess would be not allowing typo-correction to the shadowed name.

it might be possible, but I'm not sure this would fit the design of typo-correction, or make it worse

• in particular, TypoCorrection core emits all *visible* decls for a typo (in this case, both the function and the class)
• TypoCorrection API provides flexibility (e.g. CorrectionCandidateCallback ) to allow the client to do customization of all candidates, e.g. filtering, ranking (in this case, the class decl gets dropped somehow).

so, yeah it seems reasonable to fix the typo correction client side, to make it suggest a type decl (rather than a non-type decl) for this particular case.

Maybe a better way to fix the issue:

1. improve the typo correction (https://reviews.llvm.org/D83025)
2. make SemaTemplate.cpp robust, see my another comment

either 1 or 2 can fix this particular crash case (1 will improve the diagnostic), but I think we need both -- as we can't guarantee there is no typo correction running into that code path.

But I think the next best thing is just not to allow "insert typename and recover" if the expression we're inserting around already has errors - this seems like we have low confidence at this point.
So does requiring !Arg.getAsExpr().containsErrors() in the check for inserting typename work?

unfortunately, this is not possible at the moment -- the error-bit isn't propagated from TypoExpr to the corrected Expr, this is because the transformation of TypoExpr -> Expr is not straightforward,
a particular code path (in parsing a template argument):

1. we see an identifier token AddObservationFn, and we clarify (in Sema::ClassifyName) it to determine it is a type, an expr, etc
2. during the classify section, we perform a name lookup, but don't find anything. ok, let's try the typo correction.
3. we find a typo-corrected candidate (AddObservation function), so we annotate the current token is non-type (by setting the function decl)
4. in the later parsing step, we build a DeclRefExpr which points to the function decl

if we want to propagate the error-bit from typo-expr to the built DeclRefExpr, we need to carry the error-bit through the above code path somehow, which seems non-trivial.

[sammccall]

Yeah, I think you're right, and this small version of the patch seems good.

if the arg is a DeclRefExpr (no typo correction), we'll never into this branch - the lookup should find the responding ValueDecl, which is not a TypeDecl

I think this is true but I don't feel certain. @rsmith could think of an exception if anyone can. (Only important if the exception is common enough we really need to offer the typename recovery.

Agree that if we've already done typo correction then offering further recovery isn't necessary and in fact often does more harm than good.

clang/lib/Sema/SemaTemplate.cpp

4869

Assertion message doesn't really say anything more than the assertion itself.
Instead of saying what, can we say why?

(DependentScopeDeclRefExpr must be relative to some specified scope!)

clang/lib/Sema/SemaTemplate.cpp

case TemplateArgument::Expression: {
 
     if (auto *II = NameInfo.getName().getAsIdentifierInfo()) {
       LookupResult Result(*this, NameInfo, LookupOrdinaryName);
       LookupParsedName(Result, CurScope, &SS);
 
       if (Result.getAsSingle<TypeDecl>() ||
           Result.getResultKind() ==
               LookupResult::NotFoundInCurrentInstantiation) {
+        if (SS.isEmpty())
+          // bail out if we don't have a NNS, this could be happened during
+          // typo correction in error recovery.
+          // A dependent name type should have a non-null NNS.
+          return true;
         // Suggest that the user add 'typename' before the NNS.
         SourceLocation Loc = AL.getSourceRange().getBegin();
         Diag(Loc, getLangOpts().MSVCCompat
                       ? diag::ext_ms_template_type_arg_missing_typename
                       : diag::err_template_arg_must_be_type_suggest)
             << FixItHint::CreateInsertion(Loc, "typename ");
         Diag(Param->getLocation(), diag::note_template_param_here);
 

clang/test/Parser/cxx-template-decl.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -fsyntax-only -verify %s -fdelayed-template-parsing -DDELAYED_TEMPLATE_PARSING
 // RUN: %clang_cc1 -fsyntax-only -verify -std=gnu++1z %s
 
 
 
 // Errors
 export class foo { };   // expected-error {{expected template}}
 template  x;            // expected-error {{C++ requires a type specifier for all declarations}} \

 }
 
 namespace PR45239 {
   // Ensure we don't crash here. We used to deallocate the TemplateIdAnnotation
   // before we'd parsed it.
   template<int> int b;
   template<int> auto f() -> b<0>; // expected-error +{{}}
 }
+
+namespace NoCrashOnNullNNSTypoCorrection {
+
+int AddObservation(); // expected-note {{declared here}}
+
+template <typename T, typename... Args>
+class UsingImpl {};
+class AddObservation {
+  using Using =
+    UsingImpl<AddObservationFn, const int>; // expected-error {{use of undeclared identifier 'AddObservationFn'; did you mean}}
+};
+
+}