jgorbe created this revision. 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. https://reviews.llvm.org/D30963 Files: lib/Sema/SemaLookup.cpp test/Modules/Inputs/crash-typo-correction-visibility/module.h test/Modules/Inputs/crash-typo-correction-visibility/module.modulemap test/Modules/crash-typo-correction-visibility.cpp Index: test/Modules/crash-typo-correction-visibility.cpp =================================================================== --- /dev/null +++ test/Modules/crash-typo-correction-visibility.cpp @@ -0,0 +1,10 @@ +// 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'}} Index: test/Modules/Inputs/crash-typo-correction-visibility/module.modulemap =================================================================== --- /dev/null +++ test/Modules/Inputs/crash-typo-correction-visibility/module.modulemap @@ -0,0 +1,3 @@ +module "module" { + header "module.h" +} Index: test/Modules/Inputs/crash-typo-correction-visibility/module.h =================================================================== --- /dev/null +++ test/Modules/Inputs/crash-typo-correction-visibility/module.h @@ -0,0 +1 @@ +struct member; Index: lib/Sema/SemaLookup.cpp =================================================================== --- lib/Sema/SemaLookup.cpp +++ lib/Sema/SemaLookup.cpp @@ -3786,8 +3786,8 @@ 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; @@ -3797,9 +3797,11 @@ 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();
Index: test/Modules/crash-typo-correction-visibility.cpp =================================================================== --- /dev/null +++ test/Modules/crash-typo-correction-visibility.cpp @@ -0,0 +1,10 @@ +// 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'}} Index: test/Modules/Inputs/crash-typo-correction-visibility/module.modulemap =================================================================== --- /dev/null +++ test/Modules/Inputs/crash-typo-correction-visibility/module.modulemap @@ -0,0 +1,3 @@ +module "module" { + header "module.h" +} Index: test/Modules/Inputs/crash-typo-correction-visibility/module.h =================================================================== --- /dev/null +++ test/Modules/Inputs/crash-typo-correction-visibility/module.h @@ -0,0 +1 @@ +struct member; Index: lib/Sema/SemaLookup.cpp =================================================================== --- lib/Sema/SemaLookup.cpp +++ lib/Sema/SemaLookup.cpp @@ -3786,8 +3786,8 @@ 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; @@ -3797,9 +3797,11 @@ 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();
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits