patch: remove the unqualified name cache for typos

Kaelyn Takata rikka at google.com
Tue Jan 20 16:27:04 PST 2015 

On Tue, Jan 20, 2015 at 2:46 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> 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).
>

I don't think that's quite right, at least in this case.... the friend
declaration on line 78 has the error "template non-type parameter has a
different type 'long' in template redeclaration" so it seems weird to point
the note from another error to a redeclaration that has an error in it.
Same for the friend declaration on line 300 vs the actual declaration on
line 297. Though it just occurred to me the solution would be to ignore the
friend declarations that had errors... but unfortunately they aren't marked
as invalid (changing getRealDecl to only get the previous decl when
isInvalidDecl is also true for the current decl didn't change which decls
the notes referred to).

>
>
>>>> 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/20150120/787c9639/attachment.html>

More information about the cfe-commits mailing list