[PATCH] Offer typo suggestions for 'using'.

[Luke Zarko]

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 --------------
A non-text attachment was scrubbed...
Name: using-typo-v5.patch
Type: application/octet-stream
Size: 8025 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130709/37980a6b/attachment.obj>