Fix crash when an 'import a module' TypoCorrection has its CorrectionDecls replaced by visible decls.

[jgorbe]

Summary

The TypoCorrection::RequiresImport flag is managed by
checkCorrectionVisibility() in SemaLookup.cpp. Currently, when none of the
declarations in the typo correction are visible RequiresImport is set to true,
and if there are no declarations, it's implicitly set to false by assigning a
default-constructed TypoCorrection. If all declarations are visible, nothing is
done.

The current logic assumes a TypoCorrection starts as a 'normal' correction and
gets converted into an 'import a module' correction when suggested fixes are not
visible. This is not always the case. The crash in the included test case is
caused by performQualifiedLookups() copying a TypoCorrection with
RequiresImport == true, clearing its CorrectionDecls, then finding another
visible declaration in a lookup. This results in an 'import a module' correction
for a visible declaration.

The fix makes checkCorrectionVisibility() set RequiresImport in all cases, so
the relationship between RequiresImport and suggestion visibility always holds
after returning.

[jgorbe]

Ping?

[rsmith]

lib/Sema/SemaLookup.cpp

3792–3793

Do we need to clear the flag on this path too? (This might happen if the old correction required an import and after clearing we set this up as a keyword correction.)

[jgorbe]

Also clear the 'requires import' flag when the TypoCorrection has no decls at all.

[jgorbe]

lib/Sema/SemaLookup.cpp

3792–3793

If this might happen, then yes. I have updated the patch to clear the flag here as well but, on second thought, is this path necessary at all? It seems to me that 'no declarations' is a special case of 'all declarations are visible', which we check right after this.

[rsmith]

lib/Sema/SemaLookup.cpp

3792–3793

I agree, this check is redundant. Just go ahead and remove it :)

[jgorbe]

Removed unneeded branch.

[jgorbe]

lib/Sema/SemaLookup.cpp

3792–3793

Done! :D

I don't have commit access yet, can you please commit it?

lib/Sema/SemaLookup.cpp

static void LookupPotentialTypoResult(Sema &SemaRef,
                                       IdentifierInfo *Name,
                                       Scope *S, CXXScopeSpec *SS,
                                       DeclContext *MemberContext,
                                       bool EnteringContext,
                                       bool isObjCIvarLookup,
                                       bool FindHidden);
 
 /// \brief Check whether the declarations found for a typo correction are
-/// visible, and if none of them are, convert the correction to an 'import
-/// a module' correction.
+/// visible. Set the correction's RequiresImport flag to true if none of the
+/// declarations are visible, false otherwise.
 static void checkCorrectionVisibility(Sema &SemaRef, TypoCorrection &TC) {
   if (TC.begin() == TC.end())
     return;
 
   TypoCorrection::decl_iterator DI = TC.begin(), DE = TC.end();
 
   for (/**/; DI != DE; ++DI)
     if (!LookupResult::isVisible(SemaRef, *DI))
       break;
-  // Nothing to do if all decls are visible.
-  if (DI == DE)
+  // No filtering needed if all decls are visible.
+  if (DI == DE) {
+    TC.setRequiresImport(false);
     return;
+  }
 
   llvm::SmallVector<NamedDecl*, 4> NewDecls(TC.begin(), DI);
   bool AnyVisibleDecls = !NewDecls.empty();
 
   for (/**/; DI != DE; ++DI) {
     NamedDecl *VisibleDecl = *DI;
     if (!LookupResult::isVisible(SemaRef, *DI))
       VisibleDecl = findAcceptableDecl(SemaRef, *DI);

test/Modules/Inputs/crash-typo-correction-visibility/module.h

+struct member;

test/Modules/Inputs/crash-typo-correction-visibility/module.modulemap

+module "module" {
+  header "module.h"
+}

test/Modules/crash-typo-correction-visibility.cpp

+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -fmodule-name=module -o %T/module.pcm -emit-module %S/Inputs/crash-typo-correction-visibility/module.modulemap
+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -fmodule-file=%T/module.pcm %s -verify
+
+struct S {
+  int member; // expected-note {{declared here}}
+};
+
+int f(...);
+
+int b = sizeof(f(member)); // expected-error {{undeclared identifier 'member'}}