[PATCH v3 6/9] Add simple way for a CorrectionCandidateCallback to reject exact matches of the typo.

Fri Jul 25 16:36:07 PDT 2014

On Fri, Jul 25, 2014 at 2:52 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On Mon, Jul 14, 2014 at 4:55 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>>
>> Also be more proactive about checking a correction's visibility so that
>> a correction requiring a module import can be distinguished from the
>> original typo even if it looks identical. Otherwise the correction will
>> be excluded and the diagnostic about needing the module import won't be
>> emitted.
>>
>> Note that no change was made to checkCorrectionVisibility other than
>> moving where it is at in SemaLookup.cpp.
>>
>
> Perhaps I'm missing the trick here (since there are no tests as part of
> this change, it's not easy to see exactly how this pans out), but I don't
> see how we're producing the missing import diagnostic along the delayed
> typo correction codepath in a way that doesn't cause typo correction to
> fail.
>

> That said, I don't think we need to do so (and perhaps this is how this
> already works out): if we can resolve the "typo" with a module import, we
> can correct it on the fly and never build a TypoExpr. If we actually need
> to correct the typo, then we never need to add a module import (because we
> never consider typo-correcting to a non-visible name).
>

Well this does affect both the immediate and delayed typo correction
codepaths. I think the only "trick" that you are missing is that if there
is an identifier that is a typo being corrected and there is a
textually-identical identifier that is being imported from a module, they
are different just as if there is a textually-identical identifier that has
a valid decl (i.e. the resolved/imported version of the identifier doesn't
work in the current context and so led to typo correction being invoked).
Also, once I started hooking up CorrectTypoDelayed to DiagnoseEmptyLookup,
not checking candidate.requiresImport() in
CorrectionCandidateCallback::MatchesTypo breaks three of the modules tests:

********************
FAIL: Clang :: Modules/auto-module-import.m (3675 of 7523)
******************** TEST 'Clang :: Modules/auto-module-import.m' FAILED
********************
Script:
--
rm -rf
/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/auto-module-import.m.tmp
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/auto-module-import.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m
-verify -DERRORS
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/auto-module-import.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m
-verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
30: declaration of 'sub_framework_other' must be imported from module
'DependsOnModule.SubFramework.Other'
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
74: declaration of 'no_umbrella_B_private' must be imported from module
'NoUmbrella.Private.B_Private'
error: 'error' diagnostics seen but not expected:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
30: use of undeclared identifier 'sub_framework_other'
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
49: use of undeclared identifier 'sub_framework_other'
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
74: use of undeclared identifier 'no_umbrella_B_private'; did you mean
'no_umbrella_A_private'?
error: 'note' diagnostics expected but not seen:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks/SubFramework.framework/Headers/Other.h
Line 1 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m:31):
previous
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/NoUmbrella.framework/PrivateHeaders/B_Private.h
Line 1 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m:75):
previous
error: 'note' diagnostics seen but not expected:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/NoUmbrella.framework/PrivateHeaders/A_Private.h
Line 1: 'no_umbrella_A_private' declared here
8 errors generated.

--

********************
FAIL: Clang :: Modules/normal-module-map.cpp (3725 of 7523)
******************** TEST 'Clang :: Modules/normal-module-map.cpp' FAILED
********************
Script:
--
rm -rf
/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/normal-module-map.cpp.tmp
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -x objective-c
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/normal-module-map.cpp.tmp
-fmodules -I
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/normal-module-map
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp
-verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp Line
26 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp:27):
declaration of 'nested_umbrella_b' must be imported from module
'nested_umbrella.b' before it is required
error: 'error' diagnostics seen but not expected:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp Line
26: use of undeclared identifier 'nested_umbrella_b'; did you mean
'nested_umbrella_a'?
error: 'note' diagnostics expected but not seen:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/normal-module-map/nested_umbrella/b.h
Line 1 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp:28):
previous
error: 'note' diagnostics seen but not expected:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/normal-module-map/nested_umbrella/a.h
Line 1: 'nested_umbrella_a' declared here
4 errors generated.

--

********************
FAIL: Clang :: Modules/subframeworks.m (3759 of 7523)
******************** TEST 'Clang :: Modules/subframeworks.m' FAILED
********************
Script:
--
rm -rf
/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/subframeworks.m.tmp
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/subframeworks.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs -F
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks
/mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m -verify
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -x
objective-c++ -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/subframeworks.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs -F
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks
/mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m -verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen:
  File /mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m
Line 8: declaration of 'sub_framework' must be imported from module
'DependsOnModule.SubFramework' before it is required
error: 'error' diagnostics seen but not expected:
  File /mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m
Line 8: use of undeclared identifier 'sub_framework'
error: 'note' diagnostics expected but not seen:
  File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks/SubFramework.framework/Headers/SubFramework.h
Line 2 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m:9):
previous
3 errors generated.

--

>

>
>
> +  if (MatchesTypo(candidate))
> +    return false;
>
> When does this happen?
>

It doesn't; I forgot to delete this (or it snuck back in while I was
merging and shuffling around patches to prep them for review) after moving
the check into CorrectionCandidateCallback:ValidateCandidate so that
classes which override ValidateCandidate don't accidentally lose this check
(as it prevents suggesting the original typo as a correction in cases like
"int foobar = foobar;").
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140725/0286a9da/attachment.html>