patch: remove the unqualified name cache for typos

Richard Smith richard at metafoo.co.uk
Wed Jan 14 16:54:42 PST 2015 

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.


> 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
>>>>>
>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150114/4e9c0469/attachment.html>

More information about the cfe-commits mailing list