patch: remove the unqualified name cache for typos

Richard Smith richard at metafoo.co.uk
Tue Jan 20 14:46:37 PST 2015 

On Tue, Jan 20, 2015 at 1:13 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>
> On Wed, Jan 14, 2015 at 4:54 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>>
>> On Mon, Jan 5, 2015 at 2:47 PM, Kaelyn Takata <rikka at google.com> wrote:
>>>
>>> Just realized I accidently hit "Reply" instead of "Reply All" when
>>> sending this email several weeks ago...
>>>
>>> ---------- Forwarded message ----------
>>> From: Kaelyn Takata <rikka at google.com>
>>> Date: Sat, Dec 20, 2014 at 1:01 PM
>>> Subject: Re: patch: remove the unqualified name cache for typos
>>> To: Richard Smith <richard at metafoo.co.uk>
>>>
>>>
>>>
>>>
>>> On Fri, Dec 19, 2014 at 5:00 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>>
>>>> On Fri, Dec 19, 2014 at 4:46 PM, Kaelyn Takata <rikka at google.com> wrote:
>>>>>
>>>>> Here's a patch I whipped up that changes NamedDecl::getUnderlyingDecl
>>>>> to look back through friend decls to find a non-friend decl, which causes
>>>>> notes to be given in the right place in the test Nick changed
>>>>> (test/CXX/class.access/class.friend/p11.cpp) and in another test with FIXMEs
>>>>> about a couple of notes being in the wrong place
>>>>> (test/SemaTemplate/friend-template.cpp). Doing so seemed to be appropriate
>>>>> behavior for getUnderlyingDecl, instead of adding the code to look back
>>>>> through friend decls to both LookupResult::getFoundDecl and
>>>>> TypoCorrection::addCorrectionDecl. This seem good?
>>>>
>>>>
>>>> In your scan, you should also skip names in the LocalExtern namespace.
>>>> In particular, given:
>>>>
>>>> void f(int n);
>>>> void g() { void f(int n = 0); }
>>>> struct S { friend void f(int n); };
>>>> void h() { f(); }
>>>>
>>>> I think you'll skip backwards from line 3 to line 2, but we want to find
>>>> the declaration on line 1. The end result will presumably be that we accept
>>>> this ill-formed code.
>>>>
>>>> I'm also not sure that getUnderlyingDecl is the right place to skip
>>>> friends; my inclination is that this is something that should happen
>>>> internally within name lookup (name lookup should act as if it simply does
>>>> not find these friend declarations), rather than being something done by
>>>> consumers of the lookup result.
>>>
>>>
>>> I tried adding the logic to the various places where the Lookup*
>>> functions/methods add decls to the LookupResult, and the primary place that
>>> needs it to make the two tests my patch changed work properly breaks another
>>> test in a way that I'm pretty sure is a real failure. The change in question
>>> is:
>>>
>>> diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
>>> index 3445264..9d85555 100644
>>> --- a/lib/Sema/SemaLookup.cpp
>>> +++ b/lib/Sema/SemaLookup.cpp
>>> @@ -660,6 +660,13 @@ static void
>>> DeclareImplicitMemberFunctionsWithName(Sema &S,
>>>    }
>>>  }
>>>
>>> +static NamedDecl* getRealDecl(NamedDecl *ND) {
>>> +  Decl *Primary = ND;
>>> +  while (Primary &&
>>> Primary->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend))
>>> +    Primary = Primary->getPreviousDecl();
>>> +  return Primary ? cast<NamedDecl>(Primary) : ND;
>>> +}
>>> +
>>>  // Adds all qualifying matches for a name within a decl context to the
>>>  // given lookup result.  Returns true if any matches were found.
>>>  static bool LookupDirect(Sema &S, LookupResult &R, const DeclContext
>>> *DC) {
>>> @@ -675,7 +682,7 @@ static bool LookupDirect(Sema &S, LookupResult &R,
>>> const DeclContext *DC) {
>>>         ++I) {
>>>      NamedDecl *D = *I;
>>>      if ((D = R.getAcceptableDecl(D))) {
>>> -      R.addDecl(D);
>>> +      R.addDecl(getRealDecl(D));
>>>        Found = true;
>>>      }
>>>    }
>>>
>>> And the test that legitimately starts failing is CXX/drs/dr1xx.cpp:
>>>
>>> error: 'error' diagnostics expected but not seen:
>>>   File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 391: friend
>>> declaration specifying a default argument must be the only declaration
>>> error: 'error' diagnostics seen but not expected:
>>>   File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 391: missing
>>> default argument on parameter
>>> error: 'note' diagnostics expected but not seen:
>>>   File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 385: previous
>>> declaration is here
>>> 3 errors generated.
>>>
>>> So the only two options I can see for getting the notes right is the
>>> patch to getUnderlyingDecl that I sent earlier, or my original approach
>>> which was to add the logic for going back through the friend decls to both
>>> TransformTypos::EmitAllDiagnostics and LookupResult::getFoundDecl.
>>
>>
>> I think there's one more tweak missing here: we should only skip over
>> friend declarations if we're not performing redeclaration lookup.
>
>
> I tried my last patch with a slight tweak to only call getRealDecl when not
> performing redeclaration lookup, and while it fixes the above error it also
> undoes the fix for the note locations. Changing the line with the call to
> getRealDecl to "R.addDecl(R.isForRedeclaration() ? D : getRealDecl(D));"
> results in:
>
> error: 'note' diagnostics expected but not seen:
>   File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 72:
> previous non-type template parameter with type 'int' is here
>   File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 297:
> previous non-type template parameter with type 'int' is here
> error: 'note' diagnostics seen but not expected:
>   File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 78:
> previous non-type template parameter with type 'int' is here
>   File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 300:
> previous non-type template parameter with type 'int' is here
> 4 errors generated.
>
> Which puts the seen diagnostics back to what friend-template.cpp currently
> expects and has FIXMEs for. :(

I think that's a separate issue: I think that it's correct for
redeclaration lookup to find the declaration on line 78 from the
instantiation of X3<int>; that is the most recent preceding
declaration of the same entity in the same scope (friend injection
results in friends being declared in the innermost enclosing non-class
scope).

>>>> That said, the test changes look like a real improvement.
>>>>
>>>>> On Thu, Dec 18, 2014 at 5:34 PM, Richard Smith <richard at metafoo.co.uk>
>>>>> wrote:
>>>>>>
>>>>>> On Tue, Dec 16, 2014 at 9:47 AM, Kaelyn Takata <rikka at google.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>> I think this should be pretty straightforward to fix: when name lookup
>>>>>> finds a declaration whose IDNS contains both Ordinary/Tag and Friend, scan
>>>>>> backwards over the redecl chain until you find one that isn't marked Friend
>>>>>> nor LocalExtern (in MS compat mode, there might not be one, so you need to
>>>>>> return the original one in that case). The only tricky part is picking the
>>>>>> right place to insert this check into name lookup.
>>>>>>
>>>>>>> - 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> cfe-commits mailing list
>>>>>>> cfe-commits at cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>
>>>
>>
>

More information about the cfe-commits mailing list