<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 25, 2014 at 2:52 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class=""><div class="gmail_extra"><div class="gmail_quote">
On Mon, Jul 14, 2014 at 4:55 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
Also be more proactive about checking a correction's visibility so that<br>
a correction requiring a module import can be distinguished from the<br>
original typo even if it looks identical. Otherwise the correction will<br>
be excluded and the diagnostic about needing the module import won't be<br>
emitted.<br>
<br>
Note that no change was made to checkCorrectionVisibility other than<br>
moving where it is at in SemaLookup.cpp. <br></blockquote></div><br></div></div><div class="gmail_extra">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.<br>
</div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<br></div><div class="gmail_extra">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).</div>
</div></blockquote><div><br></div><div>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:</div>
<div><br></div><div><div>******************** </div><div>FAIL: Clang :: Modules/auto-module-import.m (3675 of 7523)</div><div>******************** TEST 'Clang :: Modules/auto-module-import.m' FAILED ********************</div>
<div>Script: </div><div>-- </div><div>rm -rf /mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/auto-module-import.m.tmp</div><div>/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</div>
<div>/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</div>
<div>-- </div><div>Exit Code: 1 </div><div> </div><div>Command Output (stderr): </div><div>-- </div><div>error: 'error' diagnostics expected but not seen:</div>
<div> 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'</div><div> 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'</div>
<div>error: 'error' diagnostics seen but not expected:</div></div><div><div> File /mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line 30: use of undeclared identifier 'sub_framework_other'</div>
<div> File /mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line 49: use of undeclared identifier 'sub_framework_other'</div><div> 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'?</div>
<div>error: 'note' diagnostics expected but not seen:</div><div> 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</div>
<div> 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 </div>
<div>error: 'note' diagnostics seen but not expected:</div><div> File /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/NoUmbrella.framework/PrivateHeaders/A_Private.h Line 1: 'no_umbrella_A_private' declared here</div>
<div>8 errors generated. </div><div> </div><div>-- </div><div> </div><div>******************** </div><div>FAIL: Clang :: Modules/normal-module-map.cpp (3725 of 7523)</div>
<div>******************** TEST 'Clang :: Modules/normal-module-map.cpp' FAILED ********************</div><div>Script: </div><div>-- </div></div><div><div>rm -rf /mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/normal-module-map.cpp.tmp</div>
<div>/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 </div>
<div>-- </div><div>Exit Code: 1 </div><div> </div><div>Command Output (stderr): </div><div>-- </div><div>error: 'error' diagnostics expected but not seen:</div>
<div> 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</div>
<div>error: 'error' diagnostics seen but not expected:</div><div> 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'?</div>
<div>error: 'note' diagnostics expected but not seen:</div><div> 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 </div>
<div>error: 'note' diagnostics seen but not expected:</div><div> File /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/normal-module-map/nested_umbrella/a.h Line 1: 'nested_umbrella_a' declared here</div>
<div>4 errors generated. </div></div><div><div> </div><div>-- </div><div> </div><div>******************** </div><div>FAIL: Clang :: Modules/subframeworks.m (3759 of 7523)</div>
<div>******************** TEST 'Clang :: Modules/subframeworks.m' FAILED ********************</div><div>Script: </div><div>-- </div><div>rm -rf /mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/subframeworks.m.tmp</div>
<div>/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</div>
<div>/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</div>
<div>-- </div><div>Exit Code: 1 </div><div> </div><div>Command Output (stderr): </div><div>-- </div><div>error: 'error' diagnostics expected but not seen:</div>
<div> 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</div><div>
error: 'error' diagnostics seen but not expected:</div></div><div><div> File /mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m Line 8: use of undeclared identifier 'sub_framework' </div>
<div>error: 'note' diagnostics expected but not seen:</div><div> 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</div>
<div>3 errors generated. </div><div> </div><div>-- </div></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"></div></div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+ if (MatchesTypo(candidate))</div><div class="gmail_extra">+ return false;</div><div><br></div>
<div>
When does this happen?</div></div></div>
</blockquote></div><br></div><div class="gmail_extra">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;").</div>
</div>