[PATCH] Offer typo suggestions for 'using'.

Tue Jul 9 17:05:25 PDT 2013

The latest patch looks good to me. Thanks for expanding the coverage of
Clang's typo correction! :)

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

> Awesome. I've updated the patch with the change you suggested. Thanks
> again for your help!
>
> On Tue, Jul 9, 2013 at 3:37 PM, Kaelyn Uhrain <rikka at google.com> wrote:
> > That looks much better!
> >
> > One last note: in UsingValidatorCCC, instead of defining
> droppedSpecifier as
> > "Candidate.WillReplaceSpecifier() && LookupName ==
> > Candidate.getAsString(LangOpts)", it would be faster and safer to define
> it
> > as "Candidate.WillReplaceSpecifier() &&
> > !Candidate.getCorrectionSpecifier()". The former is checking specifically
> > for changing "a::foo" to "foo" (dropping the qualifier from the current
> > name), while the latter would also match changing "a::foo" to "bar" The
> > latter is safer since a completely unqualified name is invalid for a
> "using"
> > declaration, and faster since it doesn't have to build a string
> > representation of the possibly-qualified name and then do a string
> > comparison, but simply checks that the candidate has a
> NestedNameSpecifier*
> > if it is going to replace the current specifier. And UsingValidation
> won't
> > need LookupName or LangOpts. :)
> >
> >
> > On Tue, Jul 9, 2013 at 3:23 PM, Luke Zarko <zarko at google.com> wrote:
> >>
> >> That cleaned things up a bit. Now for the cases where the code used to
> >> remove the using decl it instead suggests a fully-qualified
> >> replacement to preserve syntax correctness while still allowing the
> >> compilation to continue:
> >>
> >> namespace using_suggestion_ty_dropped_specifier {
> >> class AAA {}; // expected-note
> >> {{'::using_suggestion_ty_dropped_specifier::AAA' declared here}}
> >> namespace N { }
> >> using N::AAA; // expected-error {{no member named 'AAA' in namespace
> >> 'using_suggestion_ty_dropped_specifier::N'; did you mean
> >> '::using_suggestion_ty_dropped_specifier::AAA'?}}
> >> }
> >>
> >>
> >> On Tue, Jul 9, 2013 at 1:09 PM, Kaelyn Uhrain <rikka at google.com> wrote:
> >> > 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/129ca29b/attachment.html>