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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4869
               LookupResult::NotFoundInCurrentInstantiation) {
+        if (SS.isEmpty())
+          // bail out if we don't have a NNS, this could be happened during
----------------
sammccall wrote:
> 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)
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. 
 


================
Comment at: clang/test/Parser/cxx-template-decl.cpp:296
+class UsingImpl {};
+class AddObservation {
+  using Using =
----------------
sammccall wrote:
> 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)
> 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. 





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82738