This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jgorbe on Mar 14 2017, 6:00 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

jgorbe created this revision.Mar 14 2017, 6:00 PM
rsmith added inline comments.Jun 2 2017, 5:42 PM
lib/Sema/SemaLookup.cpp
3792–3793 ↗(On Diff #91803)

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 updated this revision to Diff 101302.Jun 2 2017, 6:03 PM

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

jgorbe added inline comments.Jun 2 2017, 6:07 PM
lib/Sema/SemaLookup.cpp
3792–3793 ↗(On Diff #91803)

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 accepted this revision.Jun 2 2017, 6:51 PM
rsmith added inline comments.
lib/Sema/SemaLookup.cpp
3792–3793 ↗(On Diff #91803)

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

This revision is now accepted and ready to land.Jun 2 2017, 6:51 PM
jgorbe updated this revision to Diff 101437.Jun 5 2017, 11:54 AM

Removed unneeded branch.

jgorbe added inline comments.Jun 5 2017, 11:56 AM
lib/Sema/SemaLookup.cpp
3792–3793 ↗(On Diff #91803)

Done! :D

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

This revision was automatically updated to reflect the committed changes.