patch: remove the unqualified name cache for typos
Kaelyn Takata
rikka at google.com
Tue Jan 20 16:39:43 PST 2015
On Tue, Jan 20, 2015 at 4:32 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> On Tue, Jan 20, 2015 at 4:27 PM, Kaelyn Takata <rikka at google.com> wrote:
> >
> >
> > 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.
>
> That's not what's happening: the surrounding template is instantiated
> twice. The first time it produces
>
> template<typename U, int V> struct /*...*/
>
> and the second time it produces
>
> template<typename U, long V> struct /*...*/
>
> It's the mismatch between these two that we're diagnosing. Note that
> the first of these two does not have any errors.
>
Ah, I see. In that case, I'll defer to you as to whether the FIXMEs are
even valid (if they aren't valid, it's probably worth adding a note in
their place briefly explaining why the notes are in the right place--that
the errors on those decls are from a specific instantiation of the template
decl, not the template decl itself).
>
> > 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/e23a2af7/attachment.html>
More information about the cfe-commits
mailing list