patch: remove the unqualified name cache for typos

[Kaelyn Takata]

On Tue, Dec 16, 2014 at 12:13 AM, Nick Lewycky <nlewycky at google.com> wrote:

> On 15 December 2014 at 18:12, Kaelyn Takata <rikka at google.com> wrote:
>>
>> In general this patch looks good. And it is probably time to kill the
>> cache anyway now that so much of the typo correction bypasses it anyway
>> (correction callbacks that implement a meaningful ValidateCandidate, and
>> the delayed typo correction for the most part). A few things that should be
>> cleaned up though:
>>
>> 1) The declaration of EmptyTypoCorrection
>> in Sema::makeTypoCorrectionConsumer is unused,
>>
>> 2) The IsUnqualifiedLookup can be dropped from Sema::FailedCorrection now
>> that it is unused within the function,
>>
>> 3) The EmptyTypoCorrection and ValidatingCallback variables
>> in Sema::CorrectTypo can be dropped now that the third argument to
>> Sema::FailedCorrection is unused, and
>>
>> 4) IsUnqualifiedLookup can be made a local variable
>> within Sema::makeTypoCorrectionConsumer instead of a out-parameter and the
>> corresponding variables in CorrectTypo and CorrectTypoDelayed can be
>> removed, now that CorrectTypo doesn't need to know whether the lookup is
>> qualified or not (which was simply passed along to various calls to
>> FailedCorrection).
>>
>
> Done.
>
>
>> However, as this patch is already fairly large (removing the cache,
>> adding the new flag, and making use of it to reunify the two main typo
>> correction test files) I'm fine with 2-4 being done in a separate cleanup
>> commit.
>>
>
> I wrote the code to remove the cache first then the flag as an add on
> afterwards. After hitting send I realized that I could split them apart and
> land the new flag first. Let me know if you need me to do that.
>
> The only concern I have with this patch is actually incidental to it:
>> in test/CXX/class.access/class.friend/p11.cpp that the notes from the newly
>> exposed suggestions are wrong--they are pointing to previous friend
>> declarations (which had also been typo-corrected) instead of to the actual
>> declarations of ::test2::foo and ::test2::bar. I suspect this is a
>> relatively simple fix, probably a matter using the the
>> primary/canonical/first delcaration instead of simply the most recent one
>> for the NamedDecl descendants that are also Redeclarable (such as
>> FunctionDecl and RecordDecl), though it is also an important one from a QoI
>> perspective as that test demonstrates the "XYZ declared here" pointing to
>> incorrect and potentially confusing locations.
>>
>
> Yes! That is certainly a bug, I think it's a bug in name lookup actually.
> That's a problem for another day.
>
> I tried adding getCanonicalDecl(), but it breaks other cases. If the first
> declaration is a friend decl, then that one is now canonical and we will
> put the note on the friend decl instead of on the non-friend decl after it.
> For example:
>
> error: 'note' diagnostics expected but not seen:
>   File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2' declared
> here
>   File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2' declared
> here
>   File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2' declared
> here
>   File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2' declared
> here
> error: 'note' diagnostics seen but not expected:
>   File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2' declared
> here
>   File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2' declared
> here
>   File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2' declared
> here
>   File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2' declared
> here
> 8 errors generated.
>
> I spent a while looking at the innards of name lookup to see what was
> wrong, and the issue is that LookupDirect returns anything that was passed
> in to makeDeclVisibleInContext before looking at other decls. Passing
> friend functions to makeDeclVisibleIn the outer Context is the right thing
> to do, it might be an ordering issue inside LookupQualifiedName or maybe we
> shouldn't stop looking just because we found one, or maybe
> findAcceptableDecl ought to do some more sorting instead of just filtering
> based on visibility.
>

Ah, fair enough. Of course it can never be simple! I wonder if what's
needed is a way to distinguish between a friend decl and a non-friend
decl... or if we'd have to go so far as to flag decls created as a result
of some diagnostic like typo correction so that they can be avoided when
possible while generating notes for subsequent diagnostics. But like you
said, it's a problem for another day; just stick a TODO or two
in test/CXX/class.access/class.friend/p11.cpp as a guidepost and reminder
that those notes shouldn't be referencing the typo-corrected friend decls.

- Kaelyn

>
> Nick
>
> On Sat, Dec 13, 2014 at 2:47 AM, Nick Lewycky <nlewycky at google.com> wrote:
>>
>>> This patch removes Sema::UnqualifiedTyposCorrected.
>>>
>>> I'm not sure this is the right thing to do. As far as I can tell this
>>> cache can't be made to be correct (wouldn't we need to invalidate it every
>>> time a new decl is declared?) and I'm not convinced it saves us much
>>> compile time. However, I'm not confident in my understanding of the code,
>>> so please do push back if it is beneficial.
>>>
>>> One bug it fixes is a case where the same full-expression has the two
>>> typos in the same identifier, and we emit the correction at the SourceRange
>>> for the first typo twice instead of once for each place it's misspelled.
>>> This is because the SourceRange is on the TypoCorrection, and the cache key
>>> is an IdentifierInfo. Adding the SourceRange to the cache key wouldn't make
>>> much sense because we wouldn't ever re-use it. (NB, I have similar feelings
>>> about TypoCorrectionFailures which I am not touching in this patch. Why
>>> would we do typo correction twice at the same location?)
>>>
>>> Removing it improves typo correction quality and there are changes to
>>> the tests (see p11.cpp, and the last two tests in typo-correction) where it
>>> fired. Because the cutoff for typos is a function of this cache, I move
>>> that to a flag (-fspell-checking-limit=...) and turn the limit off entirely
>>> to merge typo-correction-pt2.cpp and typo-correction.cpp. Also turn it up
>>> from 20 to 50 to accommodate existing tests. Without caching, we run up the
>>> counter more quickly.
>>>
>>> Please review!
>>>
>>> Nick
>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141216/c033fb1f/attachment.html>