Page MenuHomePhabricator

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

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



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

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


Event Timeline

jgorbe created this revision.Mar 14 2017, 6:00 PM
rsmith added inline comments.Jun 2 2017, 5:42 PM
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
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.
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
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.