patch: remove the unqualified name cache for typos

Richard Smith richard at metafoo.co.uk
Tue Jan 20 16:32:21 PST 2015 

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.

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

More information about the cfe-commits mailing list