[PATCH] Offer typo suggestions for 'using'.

[Kaelyn Uhrain]

The new patch looks pretty good except for one thing: the handling of a
typo correction that removed the name qualifier.

Right now in the case where the best correction is an unqualified name, the
error message suggests replacing the qualified name with the unqualified
one in the "using" statement but will sometimes include a FixIt to remove
the entire "using" statement. Since a "using" statement with an unqualified
name is not valid syntax, you need to either change the error message to
suggest dropping the "using" statement in this situation, or you need to
change UsingValidatorCCC::ValidateCandidate to exclude any correction
candidates that would drop the qualifier. Since removing entire statements
is a big stretch beyond typo correction (which just suggests alternate,
potentially qualified, identifiers), I vote strongly for the latter option
of rejecting candidates that would result in having an unqualified name in
the "using" statement--which is to say, reject candidates where
droppedSpecifier would be true, and in the diagnostic message just use a
literal 0 since droppedSpecifier would never be true.

As a minor nit, your change to Sema::BuildUsingDeclaration can be further
simplified by dropping the "if" from "if (NamedDecl *ND =
Corrected.getCorrectionDecl())" since UsingValidatorCCC rejects any
candidate that does not have a NamedDecl. Once that "if" is gone you can
drop the second "if (R.empty())" and just mark the UsingDecl as invalid,
etc, if typo correction fails.

On Tue, Jul 9, 2013 at 10:46 AM, Luke Zarko <zarko at google.com> wrote:

> Thanks for the feedback!
>
> 1- I've removed the guard.
> 2- Done. I've also added a FixIt to remove using decls that typo
> correction claims have unnecessary specifiers.
> 3- Done; I believe I understand what droppedSpecifier means, but
> there's a few tests I added in test/SemaCXX/using-decl-1.cpp that seem
> to show that it's maybe not making all the corrections it might be
> able to (is droppedSpecifier all-or-nothing?)
>
> On Mon, Jul 8, 2013 at 11:14 AM, Kaelyn Uhrain <rikka at google.com> wrote:
> > Luke,
> >
> > I just looked through the patch and have a few questions/comments about
> it:
> >
> > 1) Why do you only perform typo correction when the scope is not NULL? I
> did
> > not see any issues within tools/clang/test/ with the guard removed.
> Plus, as
> > I mentioned on IRC, there is only one path in typo correction that
> > dereferences the scope without checking it is not NULL, that path seems
> to
> > be a hard-to-reach path and/or irrelevant in the context you are running
> it
> > (iirc, it was only reached from a very ObjC-specific code path), and is
> most
> > likely a bug in the code.
> >
> > 2) Is there a reason you aren't providing a fixit hint for the typo
> > correction? If so, why can't one be provided? (And in the case that a
> fixit
> > cannot be provided, you need to drop the declaration of CorrectedStr
> since
> > it isn't being used.) If a fixit hint can be provided, please do and add
> a
> > test for the fixit hint to an appropriate file under test/FixIt/;
> basically
> > you just need to put one of your tests that offers a correction under
> there
> > to make sure that the fixit hint provides the right substitution in the
> > right place and doesn't introduce additional compilation errors.
> >
> > 3) You'll need to update your patch against clang HEAD and make sure your
> > tests still run, since last week I submitted some typo correction changes
> > and diag::err_no_member_suggest takes an additional argument to support
> > dropping namespace qualifiers (look for declarations of "bool
> > droppedSpecifier", e.g. in TryNamespaceTypoCorrection in SemaDeclCXX.cpp
> or
> > in Sema::BuildCXXNestedNameSpecifier in SemaCXXScopeSpec.cpp, for
> examples
> > of how to determine if the correction is dropping the name specifier but
> not
> > changing the base identifier).
> >
> > Cheers,
> > Kaelyn
> >
> >
> > On Mon, Jul 8, 2013 at 10:28 AM, Luke Zarko <zarko at google.com> wrote:
> >>
> >> Ping--is there still something that needs to be done to this?
> >>
> >>
> >> On Wed, Jun 26, 2013 at 3:04 PM, Luke Zarko <zarko at google.com> wrote:
> >> > Thanks for looking it over!
> >> >
> >> > I've attached an updated patch with the indent fix (clang-format
> >> > wanted the extra two spaces for some reason) and tests for
> >> > IsTypeName/isa<TypeDecl> combinations. I moved the tests to
> >> > using-decl-1.cpp; they seemed more appropriate there.
> >> >
> >> > It's unclear how to get into the IsInstantiation case, since the
> >> > callers of BuildUsingDeclaration always seem to pass in 0 for Scope
> >> > when they pass true for instantiation
> >> > (TemplateDeclInstantiator::VisitUnresolvedUsingTypenameDecl,
> >> > TemplateDeclInstantiator::VisitUnresolvedUsingValueDecl). Using
> >> > getScopeForContext(CurContext) to try and get at a Scope when I need
> >> > it for CorrectTypo returned null. UsingValidatorCCC's checks should be
> >> > logically equivalent to the checks subsequent in the source file.
> >> >
> >> > On Wed, Jun 26, 2013 at 12:41 PM, Richard Smith <
> richard at metafoo.co.uk>
> >> > wrote:
> >> >> On Wed, Jun 26, 2013 at 12:35 PM, Luke Zarko <zarko at google.com>
> wrote:
> >> >>> I found a case where Clang doesn't yet help with typo correction:
> >> >>>
> >> >>> tests/base_using.cc:
> >> >>>
> >> >>> namespace Q { class AAA {}; }
> >> >>> using Q::AAB;
> >> >>>
> >> >>> Before:
> >> >>>
> >> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in namespace
> >> >>> 'Q'
> >> >>> using Q::AAB;
> >> >>>       ~~~^
> >> >>>
> >> >>> After:
> >> >>>
> >> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in namespace
> >> >>> 'Q'; did you mean 'AAA'?
> >> >>> using Q::AAB;
> >> >>>       ~~~^
> >> >>> tests/base_using.cc:1:21: note: 'AAA' declared here
> >> >>> namespace Q { class AAA {}; }
> >> >>>                     ^
> >> >>
> >> >> Code change looks good, but could do with more test coverage for the
> >> >> type name / non-type-name / in instantiation cases.
> >> >>
> >> >> +        Diag(R.getNameLoc(), diag::err_no_member_suggest)
> >> >> +            << NameInfo.getName() << LookupContext <<
> >> >> CorrectedQuotedStr
> >> >> +            << SS.getRange();
> >> >>
> >> >> We typically indent << only two spaces, not four (clang-format bug?).
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
>
> _______________________________________________
> 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/20130709/9443f1ee/attachment.html>