Optionally add code completion results for arrow instead of dot

[yvvan]

Summary

Currently getting such completions requires source correction, reparsing and calling completion again. And if it shows no results and rollback is required then it costs one more reparse.
With this change it's possible to get all results which can be later filtered to split changes which require correction.

[yvvan]

Update CIndex minor version, add call to libclang.exports

[yvvan]

ping

[yvvan]

Rebased. Applies for current master.

Also ping again...

[yvvan]

One more Ping!

[yvvan]

Ping!!!

[ilya-biryukov]

Sorry for not taking a look for a long time. I think that would be a very useful feature to have.
Here are a few comments (also see the inline comments)

• Please add tests
• How would the clients know how to correct dot to arrow? It'd be cool if code completion results to provide a replacement for that (e.g. return a FixItHint that should be applied along with the completion items that have RequiresDotToArrowCorrection == true).
• We should probably have a more general option, i.e. AllowCorrections. We could reuse it for other advanced completion scenarios.
• This patch calls HandleCodeCompleteResults twice and it breaks at least one existing client (clangd) and, arguably, makes the contract of CodeCompleteConsumer more complicated. I have a patch that addresses that: D42474. The change also moves some of the code to parser, arguably making the overall patch.

include/clang/Sema/CodeCompleteConsumer.h

493 ↗

(On Diff #130384)

Maybe remove the default argument?
This constructor seems to only be called CodeCompletionBuilder::TakeString.

595 ↗

(On Diff #130384)

Maybe remove = false, initialize in constructor to be consistent with how other members are initialized?

612 ↗

(On Diff #130384)

Maybe remove the default argument too?
This method seems to have only 5 callsites, they should be very easy to update.

include/clang/Sema/CodeCompleteOptions.h

43 ↗

(On Diff #130384)

Could we make a more general option (say, IncludeCorrections)?
Arrow instead of dot is useful, but we might try to add more items that require various corrections when doing completions and it would be nice to reuse the option between those.

lib/Sema/SemaCodeComplete.cpp

4066 ↗

(On Diff #130384)

The code here seems to replicate what the parser does, so certifying that's it's correct is hard and we may not pick up the changes when something changes in parser.
I suggest we compute the Expr* Base for an alternate operator and pass it to code complete function. Here's a change that does that, feel free to pick it up unless you have objections to my line of thought: D42474 (it's missing marking the results with proper flags, so that's something that should be extended).

[ilya-biryukov]

I also propose to try correcting in both directions, i.e. here's how completion completion could work with this feature:

struct X { int foobarbaz; };

void test() {
  X var;
  X* ptr;
  std::unique_ptr<X> uptr;
   
  var->foobar // replace with var.foobarbaz
  ptr.foobar // replace with ptr->foobar
  uptr->get // replace with uptr.get
  uptr.foobar // replace with uptr->foobarbaz
}

[yvvan]

@ilya-biryukov
Thanks a lot for you comments and for the provided code replacement. I'm checking now how it works and will continue addressing other comments.

[yvvan]

Use D42474 code. Add missing parts and tests for errors and fixits.

[yvvan]

ping...

[yvvan]

ping

[ilya-biryukov]

Sorry for not getting back on this earlier.

I wanted to discuss whether returning correction as a enum value is a proper interface for users of libclang.
It seems easy to replace . with -> inside clang, properly handling all the tricky cases (tokens coming from macro substitution, etc) and making sure it's a viable option (again, macro substitutions may make it non-viable).
And even though it's not hard to implement things like "replace . with ->" for any particular editor, we still need to do that for every small correction that may we want to add to the completion in every editor.

I propose to return something similar to a FixItHint from Diagnostic.h, i.e. description of a simple text edit that should be applied before inserting the completion identifier. In that case, we may hope that every editor will implement the corrections once, and we could add more features into code completions that will be immediately picked up by all the editors that support it.
WDYT?

[yvvan]

I've already added hints in this patch and it also do not add extra completions unless the flag IncludeCorrections is set. So this will not force editors to use corrections.

Did you mean that you think it's good to have extra fixit hints even if this flag is not set?

[yvvan]

Or is your idea is to return the char sequence instead to use this correction in some universal way?

[ilya-biryukov]

In D41537#1015563, @yvvan wrote:

Or is your idea is to return the char sequence instead to use this correction in some universal way?

Exactly. Editors that implement corrections would pick up any new corrections automatically, rather than implementing each specific correction separately.

[yvvan]

Return possibly required corrections in the string form

[yvvan]

Ping

[ilya-biryukov]

Sorry for the delay.

include/clang/Sema/CodeCompleteConsumer.h

565 ↗

(On Diff #138139)

Having a string replacement without an actual range to replace still moves a lot of responsibility onto the clients. We should probably model corrections after the fix-its for diagnostics:

• replacements need to provide a range to be replaced, alongside with a text for the replacement,
• we should provide a list of edits to allow corrections that touch two separate pieces of code.

For the fix-its in the diagnostics, see CXLoadedDiagnostic.h for an interface exported via libclang and Diagnostic.h for an interface exported in C++ API.
WDYT?

[yvvan]

include/clang/Sema/CodeCompleteConsumer.h

565 ↗

(On Diff #138139)

I thought that fixits always exist separately from diagnostics. I will check this out, thanks.

[yvvan]

include/clang/Sema/CodeCompleteConsumer.h

565 ↗

(On Diff #138139)

I've looked into diagnostics and realized that i already use them here but the are just not forwarded to completion results which is of source possible.

I have a draft with Optional<FixItHint> instead of Optional<std::string>
It allows to use further both string and replaced/removed range and also mostly uses the same code as already provided in this patch since I already generated FixItHint for Diagnostics (line 4117 lib/Sema/SemaCodeComplete.cpp in this patch)

Is that fine with you?

[ilya-biryukov]

Sorry for the late response, was on vacation.

include/clang/Sema/CodeCompleteConsumer.h

565 ↗

(On Diff #138139)

Sounds good.
One caveat is that we should probably go with std::vector<FixItHint> instead of Optional<FixItHint>. This would mirror what clang::Diagnostic does.
"Dot to arrow" will only ever add one element there, but more intricate fixes might require more than a single edit.

[yvvan]

Use wrapped FixItHint to keep corrections.

Can be quite easily changed from Optional to some container if required.

[yvvan]

Use vector instead of optional as Ilya suggested

[ilya-biryukov]

Important: please upload the patch with full context diff

include/clang-c/Index.h

5278 ↗

(On Diff #143572)

This implies that "dot to arrow" is the only available correction. Maybe rephrase to mention that others are possible in theory?
E.g.

Whether to include completion items with corrections (small fix-its), e.g. change '.' to '->' on member access, etc.

include/clang/Sema/CodeCompleteConsumer.h

415 ↗

(On Diff #143572)

Why do we need this wrapper?
It seems that storing SourceManager and LangOpts in each fix-it is clearly confusing (they are the same for all diags in the file).
All clients that want to access the fix-it should have a reference to an existing SourceManager, right?

577 ↗

(On Diff #143572)

Storing fix-its in CodeCompletionString seems like to be against its original intention, i.e. now it depends on SourceLocations, which require SourceManager, etc.
Is there a way to get to a CodeCompletionResult from libclang? Storing fix-its there would be ideal.

704 ↗

(On Diff #143572)

Maybe accept the vector by value instead of const reference to allow the clients to std::move the argument and avoid copies?

[yvvan]

include/clang-c/Index.h

5278 ↗

(On Diff #143572)

thanks, forgot to change that one

include/clang/Sema/CodeCompleteConsumer.h

415 ↗

(On Diff #143572)

My first version took source manager and language options from CXCodeCompleteResults (which is in fact AllocatedCXCodeCompleteResults) so I needed to pass it as an extra parameter which looks kind of ugly,

The single CXCodeCompleteResult does not have them. But I can leave CXCodeCompleteResults pointer as a parameter to avoid this wrapper.

And since it's a libclang design part it probably should be fixed/workarounded there.

577 ↗

(On Diff #143572)

CodeCompletionString is an abstraction used by libclang to store everything. So every completion detail getter in libclang works through it.

CXCompletionResult stores only cursor kind and a pointer to that string.

704 ↗

(On Diff #143572)

but if it's accepted by value - it's one copy already by default

Instead I can add one more constructor with rvalue reference.

[yvvan]

Wrapper around FIxItHint is removed, other review comments are addressed .

[ilya-biryukov]

Could you upload the patch with full context?

include/clang/Sema/CodeCompleteConsumer.h

704 ↗

(On Diff #143572)

If it's accepted by value, it's copy by default for l-values, but callers have an option to std::move explicitly.
Having an r-value ref overload prevents from calling with l-values (e.g. local variables), i.e. requires either an explicitly copy to a temporary or std::move.

In general, I would still suggest passing by-value, unless it's particularly important to prohibit extra copies (e.g. for performance reasons, but I don't think it's the case here).

600 ↗

(On Diff #144076)

NIT: return llvm::ArrayRef<FixItHint> instead.

1040 ↗

(On Diff #144076)

Mention other corrections are possible in this comment too?

include/clang/Sema/CodeCompleteOptions.h

43 ↗

(On Diff #144076)

Mention other corrections are possible in this comment too?

[yvvan]

Minor clean-up according to the last comments

include/clang/Sema/CodeCompleteConsumer.h

704 ↗

(On Diff #143572)

Ok, it was always a bit unclear what compiler does in such cases but I've found other references that it's actually a move in that case.

So it's changed to pass-by-value in the next diff update.

1040 ↗

(On Diff #144076)

Thanks!

[yvvan]

Ping

[ilya-biryukov]

Sorry for the delay. I really like the direction of this patch!
What's left is defining the semantics of corrections more thoroughly to make sure we don't have tricky corner cases that users of the API can't deal with.

PS. This patch is still lacking full context of the diff.
Please use arc or send diff with full context.

include/clang-c/Index.h

5220 ↗

(On Diff #144285)

I'm a bit vary about exposing source manager and language options in the API just for the sake of corrections.
I suggest we add an extra parameter of an existing class (CXCodeCompleteResults) instead and remove the corresponding helpers to get source manager and language options:
clang_getCompletionNumCorrections(CXCompletionString completion_string, CXCodeCompleteResults* results);

include/clang/Sema/CodeCompleteConsumer.h

564 ↗

(On Diff #144285)

Adding some docs here would be useful. These fix-its could be interpreted too broadly at this point.
I suggest the following semantics and the comment:

/// \brief FixIts that *must* be applied before inserting the text for the corresponding completion item.
/// Completion items with non-empty fixits will not be returned by default, they should be explicitly requested by setting CompletionOptions::IncludeCorrections.
/// For the editors to be able to compute position of the cursor for the completion item itself, the following conditions are guaranteed to hold for RemoveRange of the stored fixits:
///  - Ranges in the fixits are guaranteed to never contain the completion point (or identifier under completion point, if any) inside them, except at the start or at the end of the range.
///  - If a fixit range starts or ends with completion point (or starts or ends after the identifier under completion point), it will contain at least one character. It allows to unambiguously recompute completion point after applying the fixit.
/// The intuition is that provided fixits change code around the identifier we complete, but are not allowed to touch the identifier itself or the completion point.
/// One example of completion items with corrections are the ones replacing '.' with '->' and vice versa:
///      std::unique_ptr<std::vector<int>> vec_ptr;
///      vec_ptr.^  // completion returns an item 'push_back', replacing '.' with '->'
///      vec_ptr->^ // completion returns an item 'release', replacing '->' with '.'let's put

Do those invariants sound reasonable? Could we add asserts that they hold when constructing the completion results?

565 ↗

(On Diff #144285)

I wonder if we should call them Fixits instead?
To follow the pattern for diagnostics.

tools/c-index-test/c-index-test.c

2346 ↗

(On Diff #144285)

We should print all provided fixits in the test code

2350 ↗

(On Diff #144285)

maybe add a source range or source of the replaced text to the printf here?
e.g. "replaces '.' with '->'"?

tools/libclang/CIndexCodeCompletion.cpp

340 ↗

(On Diff #144285)

Maybe return a struct containing both the string and CXSourceRange instead of this output parameter?
It does not make sense to get a string without the corresponding source range.
How does libclang return fixits for diagnostics? We probably want to follow the same pattern here.

[yvvan]

include/clang-c/Index.h

5220 ↗

(On Diff #144285)

Haha, I had that interface initially but thought it's too ugly and needs to be improved :)

include/clang/Sema/CodeCompleteConsumer.h

564 ↗

(On Diff #144285)

Thanks! I usually feel the lack of patience when writing descriptions :)

564 ↗

(On Diff #144285)

"Could we add asserts that they hold when constructing the completion results"

The current way of creating them actually assures that by default. And to check it once again it's required to change many things.
Starting with the fact that there is no SourceRange contains() methos or something similar,

565 ↗

(On Diff #144285)

Ok, let's call them fixit-s

[yvvan]

Address comments and provide diff with full context

[ilya-biryukov]

include/clang-c/Index.h

5237 ↗

(On Diff #145664)

This seems too large for a brief comment :-)
Maybe break with a newline after the first sentence.

include/clang/Sema/CodeCompleteConsumer.h

564 ↗

(On Diff #145664)

I suggest moving this doc comment to getFixits method, as it is the public interface.

592 ↗

(On Diff #145664)

We can't store fixits in CodeCompletionString, it should not contain source locatins, which are only valid for the lifetime of SourceManager.
Note that CCS has a lifetime independent of both the SourceManager and the AST.
Storing them in CodeCompletionResult should be good enough for all our use-cases.

814 ↗

(On Diff #145664)

We shouldn't duplicate such a large comment in too many places, it would be impossible to keep it in sync.
I would suggest only keeping it in CodeCompletionResult and add a reference to it in other places.
libclang is a bit tricky, though. It is distributed without other LLVM headers, right?

564 ↗

(On Diff #144285)

The current way of creating them actually assures that by default. And to check it once again it's required to change many things.

It's still nice to have a sanity check there, mostly for the sake of new checks that are gonna get added.
But even the current one is tricky, since it may actually contain a range that ends exactly at the cursor (when completing right after the dot/arrow foo->|).

Starting with the fact that there is no SourceRange contains() methos or something similar,

Moreover, SourceRange doesn't have clear semantics AFAIK. It can either be a half-open or closed range.
Let's add a FIXME, at least, that we should add those asserts later.

[yvvan]

I have some failing tests... So I will update the diff a bit later (Friday most likely)

include/clang/Sema/CodeCompleteConsumer.h

592 ↗

(On Diff #145664)

To support that I had to make libclang calls even worse but whatever.

814 ↗

(On Diff #145664)

I will keep the comment only here and in libclang

564 ↗

(On Diff #144285)

It's removed from CodeCompletionString therefore it's now only above the public declaration.

[yvvan]

Only keep small completion fixit-s in CodeCompletionResults.
Change libclang calls to overcome that and not use CXCompletionString

[yvvan]

ping

I hope this review will be over some day :)

[ilya-biryukov]

In D41537#1097601, @yvvan wrote:

I hope this review will be over some day :)

Sorry for keeping it that long.
FWIW, I think we keep jumping back and forth between the clang and libclang changes here.
I should've suggested splitting the change into two earlier.

I promise to promptly review the changes, let's try getting it in this week. I am very excited to have this feature!

lib/Sema/SemaCodeComplete.cpp

4107 ↗

(On Diff #146289)

This FIXME should probably go into the CompletionResult constructor, we want to check the invariants in a single place.

4118 ↗

(On Diff #146289)

Why do we need to report an extra diagnostic here in completion?
Our completion items contain all the relevant information for actually doing the change, and having different diagnostics in completion vs non-completion modes does not seem like the right way to do things.

tools/libclang/CIndexCodeCompletion.cpp

309 ↗

(On Diff #146289)

Storing vector<vector<>> here makes looks like a hack. Even though it might seem more tricky, I suggest storing an opaque pointer to vector<FixItHint> in each CXCompletionResult. Managing the lifetime of vectors in the AllocatedCXCodeCompleteResults seems totally fine, but there should be a way to get to the fixits in a similar way we can get to the completion string.
More concretely, I suggest the following API:

// === Index.h
typedef void* CXCompletionFixIts;
typedef struct {
   // ...
   CXCompletionFixIts FixIts;
};

// Get the number of fix-its.
unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result);
// ... Similar to getDiagnosticFixIt
CXString clang_getCompletionFixIt((CXCompletionResult *Result, unsigned fixit_index, CXSourceRange *replacement_range);



// === Impl.cpp
struct AllocatedCXCodeCompleteResults : public CXCodeCompleteResults {
// .....
// Pool for allocating non-empty fixit vectors in the CXCompletionResult.
std::vector<std::vector<FixItHint>> FixItsVector
};

unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result) {
  auto* FixIts = static_cast<std::vector<FixItHint>*>(Result->FixIts);
  if (!FixIts)
    return 0;
  return FixIts->size();
}

[yvvan]

In D41537#1097763, @ilya-biryukov wrote:

I should've suggested splitting the change into two earlier.

Next time I will do that from the beginning :)

tools/libclang/CIndexCodeCompletion.cpp

309 ↗

(On Diff #146289)

unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result);

Do you mean appending CXCompletionResult struct with the "CXCompletionFixIts FixIts" member? Doesn't it break binary compatibility (changing the struct size)?

[ilya-biryukov]

tools/libclang/CIndexCodeCompletion.cpp

309 ↗

(On Diff #146289)

Ah, you're right. If libclang promises binary compatibility with binaries compiled from headers with previous versions, we're not allowed to do this change.
I'm not sure what's the best way to do that, probably the interface you propose does what we want with the least amount of friction.
I'd be fine with leaving as is, but given that libclang has a stable interface, I'd get more opinions from someone who owns the code before adding this. (Another reason to split this change into two?)

A possible alternative that won't break binary compatibility would be to change the opaque CXCompletionString to point into a struct that has both CodeCompletionString and the fixits vector, but that means finding all use-sites of CXCompletionString and that might be tricky.

[yvvan]

Only C++ part, libclang part has moved to https://reviews.llvm.org/D46862

[yvvan]

lib/Sema/SemaCodeComplete.cpp

4118 ↗

(On Diff #146289)

In current version of this patch it's not needed anymore. Removed.

tools/libclang/CIndexCodeCompletion.cpp

309 ↗

(On Diff #146289)

I already tried to have it in CXCompletionString. And I've even found and replaced all usages. But there's a bigger issue: CXCompletionString does not have dispose method and it exists not only in context of CXCompletionChunkKind (for example as a return value of clang_getCompletionChunkCompletionString). It's easy to add such dispose method but it will introduce leaks in already existing code bases.

By all that I mean that "the interface you propose does what we want with the least amount of friction".

[ilya-biryukov]

Maybe move the tests back to this revision?
There are tests for code completion that don't depend on libindex or libclang and use clang -cc1 directly, i.e. tools/clang/test/CodeComplete. This would require adding a flag to clang and patching up PrintingCodeCompleConsumer to print the fix-its, but that should be easy. Moreover, it's useful to have the option to show those completions in clang anyway.

include/clang/Basic/DiagnosticSemaKinds.td

5715 ↗

(On Diff #146749)

This is not used anymore, right? Maybe remove it?

include/clang/Sema/CodeCompleteConsumer.h

807 ↗

(On Diff #146749)

NIT: reflow on this comment broke formatting, maybe keep it unindented instead? I.e., this should give better formatting:

///      std::unique_ptr<std::vector<int>> vec_ptr;
///      vec_ptr.^ 
///      vec_ptr->^
///
///  In 'vec_ptr.^', one of completion items is 'push_back', it requires replacing '.' with '->'.
///  In 'vec_ptr->^', one of completion items is 'release', it requires replacing '.' with '->'.

lib/Sema/SemaCodeComplete.cpp

3954 ↗

(On Diff #146749)

Maybe pass a single fix-it here too with a more descriptive name?
It would make the code more readable.

4022 ↗

(On Diff #146749)

Maybe pass a single fix-it with more descriptive name, e.g. Optional<FixItHint> AccessOpFixIt?

4105 ↗

(On Diff #146749)

This FIXME should probably live in CodeCompletionResult constructor or ResultBuilder::AddResult. It's pretty obvious that this specific code does the right thing, but it may be non-obvious about more generic code that stores a vector<FixItHint>

test/SemaCXX/member-expr.cpp

193 ↗

(On Diff #146749)

Is this still needed after we removed the diagnostics code?

tools/libclang/CIndexCodeCompletion.cpp

309 ↗

(On Diff #146289)

I already tried to have it in CXCompletionString. And I've even found and replaced all usages. But there's a bigger issue: CXCompletionString does not have dispose method and it exists not only in context of CXCompletionChunkKind (for example as a return value of clang_getCompletionChunkCompletionString). It's easy to add such dispose method but it will introduce leaks in already existing code bases.

Yeah, that's probably too compilcated. And it does not make sense for completion strings inside chunks to have fix-its, the fix-its only make sense for the completion item as a whole.

[yvvan]

I will add more tests...

test/SemaCXX/member-expr.cpp

193 ↗

(On Diff #146749)

Yes, even if it does not fail it is not related to that change anymore

[yvvan]

Append PrintingCodeCompleteConsumer and CompilerInvocation options, add CodeCompletion tests.

[ilya-biryukov]

Sorry, a few more things and we're good to go.

include/clang/Driver/CC1Options.td

449 ↗

(On Diff #147039)

NIT: maybe simplify to "which require small fix-its"?

lib/Frontend/CompilerInvocation.cpp

1541 ↗

(On Diff #147039)

Following naming convention of other options, maybe use OPT_code_completion_with_fixits? (i.e. none of them start with include)

lib/Sema/CodeCompleteConsumer.cpp

559 ↗

(On Diff #147039)

Unfortunately, that might not work for some ranges, see docs of CharSourceRange:

/// In the token range case, the
/// size of the last token must be measured to determine the actual end of the
/// range.

The TextDiagnostic::emitParseableFixits handles it, I suggest we do it similarly:

FixItHint*I = //....;
SourceLocation BLoc = I->RemoveRange.getBegin();
SourceLocation ELoc = I->RemoveRange.getEnd();

std::pair<FileID, unsigned> BInfo = SM.getDecomposedLoc(BLoc);
std::pair<FileID, unsigned> EInfo = SM.getDecomposedLoc(ELoc);

// Adjust for token ranges.
if (I->RemoveRange.isTokenRange())
  EInfo.second += Lexer::MeasureTokenLength(ELoc, SM, LangOpts);

// We specifically do not do word-wrapping or tab-expansion here,
// because this is supposed to be easy to parse.
PresumedLoc PLoc = SM.getPresumedLoc(BLoc);
if (PLoc.isInvalid())
  break;

OS << "fix-it:\"";
OS.write_escaped(PLoc.getFilename());
OS << "\":{" << SM.getLineNumber(BInfo.first, BInfo.second)
  << ':' << SM.getColumnNumber(BInfo.first, BInfo.second)
  << '-' << SM.getLineNumber(EInfo.first, EInfo.second)
  << ':' << SM.getColumnNumber(EInfo.first, EInfo.second)
  << "}:\"";
OS.write_escaped(I->CodeToInsert);
OS << "\"\n";

lib/Sema/SemaCodeComplete.cpp

4109 ↗

(On Diff #147039)

I'd use token ranges here to avoid assumptions about sizes of tokens, e.g. CreateReplacemen(CharSourceRange::getTokenRange(OpLoc, OpLoc), IsArrow ? '.' : '->')
There are complicated cases like \ that end of the line and macros and it's definitely better to use an abstraction that hides those cases.

[yvvan]

lib/Sema/CodeCompleteConsumer.cpp

559 ↗

(On Diff #147039)

yeah. sorry. it's always like that with source ranges. thanks for a hint

lib/Sema/SemaCodeComplete.cpp

4109 ↗

(On Diff #147039)

the problem is that I need the range, not just a location.... and I don't know how to extract it here. is there a way to get next token location here?

[ilya-biryukov]

lib/Sema/SemaCodeComplete.cpp

4109 ↗

(On Diff #147039)

IIUC, using CharSourceRange::getTokenRange(OpLoc, OpLoc) will do what you want.
When CharSourceRange is a token range, the client code should measurements the token length and the producing code would use the starting location of the last token.
Here are the relevant bits from the comment of the CharSourceRange:

/// In the token range case, the
/// size of the last token must be measured to determine the actual end of the
/// range.

[yvvan]

lib/Sema/SemaCodeComplete.cpp

4109 ↗

(On Diff #147039)

ok, i will recheck that it does calculate the token range

[ilya-biryukov]

I have only a few nits left, otherwise looks good. Thanks for making this change! Really excited for it to get in finally!

include/clang/Driver/CC1Options.td

449 ↗

(On Diff #148164)

NIT: there's a redundant space before the closing .

include/clang/Sema/CodeCompleteConsumer.h

786 ↗

(On Diff #148164)

NIT: remove \brief from the start of the comment. Those recently got removed from all of llvm

1059 ↗

(On Diff #148164)

NIT: another redundant \brief

include/clang/Sema/CodeCompleteOptions.h

42 ↗

(On Diff #148164)

NIT: Maybe s/Show also results/Include results?

lib/Sema/SemaCodeComplete.cpp

4148 ↗

(On Diff #148164)

NIT: maybe swap the two cases to do the non-fixit ones first? It is the most common case, so it should probably go first.

[yvvan]

lib/Sema/SemaCodeComplete.cpp

4148 ↗

(On Diff #148164)

The actual completion should be the last because of the "Completion context". I've put them in this order intentionally. I will add comment about that.

[yvvan]

lib/Sema/SemaCodeComplete.cpp

4148 ↗

(On Diff #148164)

I've rechecked this part - I can actually now move it without breaking anything

[yvvan]

NIT-s addressed

[ilya-biryukov]

Thanks for addressing the NITs. LGTM!

cfe/trunk/include/clang/Driver/CC1Options.td

 def code_completion_patterns : Flag<["-"], "code-completion-patterns">,
   HelpText<"Include code patterns in code-completion results">;
 def no_code_completion_globals : Flag<["-"], "no-code-completion-globals">,
   HelpText<"Do not include global declarations in code-completion results.">;
 def no_code_completion_ns_level_decls : Flag<["-"], "no-code-completion-ns-level-decls">,
   HelpText<"Do not include declarations inside namespaces (incl. global namespace) in the code-completion results.">;
 def code_completion_brief_comments : Flag<["-"], "code-completion-brief-comments">,
   HelpText<"Include brief documentation comments in code-completion results.">;
+def code_completion_with_fixits : Flag<["-"], "code-completion-with-fixits">,
+  HelpText<"Include code completion results which require small fix-its.">;
 def disable_free : Flag<["-"], "disable-free">,
   HelpText<"Disable freeing of memory on exit">;
 def discard_value_names : Flag<["-"], "discard-value-names">,
   HelpText<"Discard value names in LLVM IR">;
 def load : Separate<["-"], "load">, MetaVarName<"<dsopath>">,
   HelpText<"Load the named plugin (dynamic shared object)">;
 def plugin : Separate<["-"], "plugin">, MetaVarName<"<name>">,
   HelpText<"Use the named plugin action instead of the default action (use \"help\" to list available options)">;

cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h

public:
   ResultKind Kind;
 
   /// The cursor kind that describes this result.
   CXCursorKind CursorKind;
 
   /// The availability of this result.
   CXAvailabilityKind Availability = CXAvailability_Available;
 
+  /// FixIts that *must* be applied before inserting the text for the
+  /// corresponding completion item.
+  ///
+  /// Completion items with non-empty fixits will not be returned by default,
+  /// they should be explicitly requested by setting
+  /// CompletionOptions::IncludeFixIts. For the editors to be able to
+  /// compute position of the cursor for the completion item itself, the
+  /// following conditions are guaranteed to hold for RemoveRange of the stored
+  /// fixits:
+  ///  - Ranges in the fixits are guaranteed to never contain the completion
+  ///  point (or identifier under completion point, if any) inside them, except
+  ///  at the start or at the end of the range.
+  ///  - If a fixit range starts or ends with completion point (or starts or
+  ///  ends after the identifier under completion point), it will contain at
+  ///  least one character. It allows to unambiguously recompute completion
+  ///  point after applying the fixit.
+  /// The intuition is that provided fixits change code around the identifier we
+  /// complete, but are not allowed to touch the identifier itself or the
+  /// completion point. One example of completion items with corrections are the
+  /// ones replacing '.' with '->' and vice versa:
+  /// std::unique_ptr<std::vector<int>> vec_ptr;
+  /// In 'vec_ptr.^', one of completion items is 'push_back', it requires
+  /// replacing '.' with '->'.
+  /// In 'vec_ptr->^', one of completion items is 'release', it requires
+  /// replacing '->' with '.'.
+  std::vector<FixItHint> FixIts;
+
   /// Whether this result is hidden by another name.
   bool Hidden : 1;
 
   /// Whether this result was found via lookup into a base class.
   bool QualifierIsInformative : 1;
 
   /// Whether this declaration is the beginning of a
   /// nested-name-specifier and, therefore, should be followed by '::'.
   bool StartsNestedNameSpecifier : 1;
 
   /// Whether all parameters (of a function, Objective-C
   /// method, etc.) should be considered "informative".
   bool AllParametersAreInformative : 1;
 
   /// Whether we're completing a declaration of the given entity,
   /// rather than a use of that entity.
   bool DeclaringEntity : 1;
 
   /// If the result should have a nested-name-specifier, this is it.
   /// When \c QualifierIsInformative, the nested-name-specifier is
   /// informative rather than required.
   NestedNameSpecifier *Qualifier = nullptr;
 
   /// Build a result that refers to a declaration.
-  CodeCompletionResult(const NamedDecl *Declaration,
-                       unsigned Priority,
+  CodeCompletionResult(const NamedDecl *Declaration, unsigned Priority,
                        NestedNameSpecifier *Qualifier = nullptr,
                        bool QualifierIsInformative = false,
-                       bool Accessible = true)
+                       bool Accessible = true,
+                       std::vector<FixItHint> FixIts = std::vector<FixItHint>())
       : Declaration(Declaration), Priority(Priority), Kind(RK_Declaration),
         Hidden(false), QualifierIsInformative(QualifierIsInformative),
         StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
-        DeclaringEntity(false), Qualifier(Qualifier) {
+        DeclaringEntity(false), Qualifier(Qualifier),
+        FixIts(std::move(FixIts)) {
+    //FIXME: Add assert to check FixIts range requirements.
     computeCursorKindAndAvailability(Accessible);
   }
 
   /// Build a result that refers to a keyword or symbol.
   CodeCompletionResult(const char *Keyword, unsigned Priority = CCP_Keyword)
       : Keyword(Keyword), Priority(Priority), Kind(RK_Keyword),
         CursorKind(CXCursor_NotImplemented), Hidden(false),
         QualifierIsInformative(false), StartsNestedNameSpecifier(false),
public:
   }
 
   /// Whether to include brief documentation comments within the set of
   /// code completions returned.
   bool includeBriefComments() const {
     return CodeCompleteOpts.IncludeBriefComments;
   }
 
+  /// Whether to include completion items with small fix-its, e.g. change
+  /// '.' to '->' on member access, etc.
+  bool includeFixIts() const { return CodeCompleteOpts.IncludeFixIts; }
+
   /// Hint whether to load data from the external AST in order to provide
   /// full results. If false, declarations from the preamble may be omitted.
   bool loadExternal() const {
     return CodeCompleteOpts.LoadExternal;
   }
 
   /// Determine whether the output of this consumer is binary.
   bool isOutputBinary() const { return OutputIsBinary; }

cfe/trunk/include/clang/Sema/CodeCompleteOptions.h

public:
 
   /// Show brief documentation comments in code completion results.
   unsigned IncludeBriefComments : 1;
 
   /// Hint whether to load data from the external AST to provide full results.
   /// If false, namespace-level declarations from the preamble may be omitted.
   unsigned LoadExternal : 1;
 
+  /// Include results after corrections (small fix-its), e.g. change '.' to '->'
+  /// on member access, etc.
+  unsigned IncludeFixIts : 1;
+
   CodeCompleteOptions()
       : IncludeMacros(0), IncludeCodePatterns(0), IncludeGlobals(1),
         IncludeNamespaceLevelDecls(1), IncludeBriefComments(0),
-        LoadExternal(1) {}
+        LoadExternal(1), IncludeFixIts(0) {}
 };
 
 } // namespace clang
 
 #endif
 

cfe/trunk/include/clang/Sema/Sema.h

void CodeCompleteOrdinaryName(Scope *S,
                                 ParserCompletionContext CompletionContext);
   void CodeCompleteDeclSpec(Scope *S, DeclSpec &DS,
                             bool AllowNonIdentifiers,
                             bool AllowNestedNameSpecifiers);
 
   struct CodeCompleteExpressionData;
   void CodeCompleteExpression(Scope *S,
                               const CodeCompleteExpressionData &Data);
-  void CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base,
+  void CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base, Expr *OtherOpBase,
                                        SourceLocation OpLoc, bool IsArrow,
                                        bool IsBaseExprStatement);
   void CodeCompletePostfixExpression(Scope *S, ExprResult LHS);
   void CodeCompleteTag(Scope *S, unsigned TagSpec);
   void CodeCompleteTypeQualifiers(DeclSpec &DS);
   void CodeCompleteFunctionQualifiers(DeclSpec &DS, Declarator &D,
                                       const VirtSpecifiers *VS = nullptr);
   void CodeCompleteBracketDeclarator(Scope *S);

cfe/trunk/lib/Frontend/ASTUnit.cpp

void ASTUnit::CodeComplete(
   PreprocessorOptions &PreprocessorOpts = CCInvocation->getPreprocessorOpts();
 
   CodeCompleteOpts.IncludeMacros = IncludeMacros &&
                                    CachedCompletionResults.empty();
   CodeCompleteOpts.IncludeCodePatterns = IncludeCodePatterns;
   CodeCompleteOpts.IncludeGlobals = CachedCompletionResults.empty();
   CodeCompleteOpts.IncludeBriefComments = IncludeBriefComments;
   CodeCompleteOpts.LoadExternal = Consumer.loadExternal();
+  CodeCompleteOpts.IncludeFixIts = Consumer.includeFixIts();
 
   assert(IncludeBriefComments == this->IncludeBriefCommentsInCodeCompletion);
 
   FrontendOpts.CodeCompletionAt.FileName = File;
   FrontendOpts.CodeCompletionAt.Line = Line;
   FrontendOpts.CodeCompletionAt.Column = Column;
 
   // Set the language options appropriately.

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static InputKind ParseFrontendArgs(FrontendOptions &Opts, ArgList &Args,
   Opts.CodeCompleteOpts.IncludeCodePatterns
     = Args.hasArg(OPT_code_completion_patterns);
   Opts.CodeCompleteOpts.IncludeGlobals
     = !Args.hasArg(OPT_no_code_completion_globals);
   Opts.CodeCompleteOpts.IncludeNamespaceLevelDecls
     = !Args.hasArg(OPT_no_code_completion_ns_level_decls);
   Opts.CodeCompleteOpts.IncludeBriefComments
     = Args.hasArg(OPT_code_completion_brief_comments);
+  Opts.CodeCompleteOpts.IncludeFixIts
+    = Args.hasArg(OPT_code_completion_with_fixits);
 
   Opts.OverrideRecordLayoutsFile
     = Args.getLastArgValue(OPT_foverride_record_layout_EQ);
   Opts.AuxTriple =
       llvm::Triple::normalize(Args.getLastArgValue(OPT_aux_triple));
   Opts.FindPchSource = Args.getLastArgValue(OPT_find_pch_source_EQ);
   Opts.StatsFile = Args.getLastArgValue(OPT_stats_file);
 

cfe/trunk/lib/Parse/ParseExpr.cpp

case tok::period: {
       // postfix-expression: p-e '->' template[opt] id-expression
       // postfix-expression: p-e '.' template[opt] id-expression
       tok::TokenKind OpKind = Tok.getKind();
       SourceLocation OpLoc = ConsumeToken();  // Eat the "." or "->" token.
 
       CXXScopeSpec SS;
       ParsedType ObjectType;
       bool MayBePseudoDestructor = false;
+      Expr* OrigLHS = !LHS.isInvalid() ? LHS.get() : nullptr;
+
       if (getLangOpts().CPlusPlus && !LHS.isInvalid()) {
-        Expr *Base = LHS.get();
+        Expr *Base = OrigLHS;
         const Type* BaseType = Base->getType().getTypePtrOrNull();
         if (BaseType && Tok.is(tok::l_paren) &&
             (BaseType->isFunctionType() ||
              BaseType->isSpecificPlaceholderType(BuiltinType::BoundMember))) {
           Diag(OpLoc, diag::err_function_is_not_record)
               << OpKind << Base->getSourceRange()
               << FixItHint::CreateRemoval(OpLoc);
           return ParsePostfixExpressionSuffix(Base);
         }
 
         LHS = Actions.ActOnStartCXXMemberReference(getCurScope(), Base,
                                                    OpLoc, OpKind, ObjectType,
                                                    MayBePseudoDestructor);
         if (LHS.isInvalid())
           break;
 
         ParseOptionalCXXScopeSpecifier(SS, ObjectType, 
                                        /*EnteringContext=*/false,
                                        &MayBePseudoDestructor);
         if (SS.isNotEmpty())
           ObjectType = nullptr;
       }
 
       if (Tok.is(tok::code_completion)) {
+        tok::TokenKind CorrectedOpKind =
+            OpKind == tok::arrow ? tok::period : tok::arrow;
+        ExprResult CorrectedLHS(/*IsInvalid=*/true);
+        if (getLangOpts().CPlusPlus && OrigLHS) {
+          const bool DiagsAreSuppressed = Diags.getSuppressAllDiagnostics();
+          Diags.setSuppressAllDiagnostics(true);
+          CorrectedLHS = Actions.ActOnStartCXXMemberReference(
+              getCurScope(), OrigLHS, OpLoc, CorrectedOpKind, ObjectType,
+              MayBePseudoDestructor);
+          Diags.setSuppressAllDiagnostics(DiagsAreSuppressed);
+        }
+
+        Expr *Base = LHS.get();
+        Expr *CorrectedBase = CorrectedLHS.get();
+
         // Code completion for a member access expression.
-        if (Expr *Base = LHS.get())
-          Actions.CodeCompleteMemberReferenceExpr(
-              getCurScope(), Base, OpLoc, OpKind == tok::arrow,
-              ExprStatementTokLoc == Base->getLocStart());
+            getCurScope(), Base, CorrectedBase, OpLoc, OpKind == tok::arrow,
+            Base && ExprStatementTokLoc == Base->getLocStart());
 
         cutOffParsing();
         return ExprError();
       }
 
       if (MayBePseudoDestructor && !LHS.isInvalid()) {
         LHS = ParseCXXPseudoDestructor(LHS.get(), OpLoc, OpKind, SS, 
                                        ObjectType);

cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp

case CodeCompletionResult::RK_Declaration:
             = Results[I].CreateCodeCompletionString(SemaRef, Context,
                                                     getAllocator(),
                                                     CCTUInfo,
                                                     includeBriefComments())) {
         OS << " : " << CCS->getAsString();
         if (const char *BriefComment = CCS->getBriefComment())
           OS << " : " << BriefComment;
       }
+      for (const FixItHint &FixIt : Results[I].FixIts) {
+        const SourceLocation BLoc = FixIt.RemoveRange.getBegin();
+        const SourceLocation ELoc = FixIt.RemoveRange.getEnd();
+
+        SourceManager &SM = SemaRef.SourceMgr;
+        std::pair<FileID, unsigned> BInfo = SM.getDecomposedLoc(BLoc);
+        std::pair<FileID, unsigned> EInfo = SM.getDecomposedLoc(ELoc);
+        // Adjust for token ranges.
+        if (FixIt.RemoveRange.isTokenRange())
+          EInfo.second += Lexer::MeasureTokenLength(ELoc, SM, SemaRef.LangOpts);
+
+        OS << " (requires fix-it:"
+           << " {" << SM.getLineNumber(BInfo.first, BInfo.second) << ':'
+           << SM.getColumnNumber(BInfo.first, BInfo.second) << '-'
+           << SM.getLineNumber(EInfo.first, EInfo.second) << ':'
+           << SM.getColumnNumber(EInfo.first, EInfo.second) << "}"
+           << " to \"" << FixIt.CodeToInsert << "\")";
+      }
       OS << '\n';
       break;
       
     case CodeCompletionResult::RK_Keyword:
       OS << Results[I].Keyword << '\n';
       break;
         
     case CodeCompletionResult::RK_Macro:

cfe/trunk/lib/Sema/SemaCodeComplete.cpp

 }
 
 namespace {
   /// Visible declaration consumer that adds a code-completion result
   /// for each visible declaration.
   class CodeCompletionDeclConsumer : public VisibleDeclConsumer {
     ResultBuilder &Results;
     DeclContext *CurContext;
+    std::vector<FixItHint> FixIts;
 
   public:
-    CodeCompletionDeclConsumer(ResultBuilder &Results, DeclContext *CurContext)
-      : Results(Results), CurContext(CurContext) { }
+    CodeCompletionDeclConsumer(
+        ResultBuilder &Results, DeclContext *CurContext,
+        std::vector<FixItHint> FixIts = std::vector<FixItHint>())
+        : Results(Results), CurContext(CurContext), FixIts(std::move(FixIts)) {}
 
     void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
                    bool InBaseClass) override {
       bool Accessible = true;
       if (Ctx)
         Accessible = Results.getSema().IsSimplyAccessible(ND, Ctx);
 
       ResultBuilder::Result Result(ND, Results.getBasePriority(ND), nullptr,
-                                   false, Accessible);
+                                   false, Accessible, FixIts);
       Results.AddResult(Result, CurContext, Hiding, InBaseClass);
     }
 
     void EnteredContext(DeclContext* Ctx) override {
       Results.addVisitedContext(Ctx);
     }
   };
 }
for (auto *P : Category->protocols())
                         CurContext, AddedProperties, Results,
                         IsBaseExprStatement, IsClassProperty);
   }
 }
 
 static void AddRecordMembersCompletionResults(Sema &SemaRef,
                                               ResultBuilder &Results, Scope *S,
                                               QualType BaseType,
-                                              RecordDecl *RD) {
+                                              RecordDecl *RD,
+                                              Optional<FixItHint> AccessOpFixIt) {
   // Indicate that we are performing a member access, and the cv-qualifiers
   // for the base object type.
   Results.setObjectTypeQualifiers(BaseType.getQualifiers());
 
   // Access to a C/C++ class, struct, or union.
   Results.allowNestedNameSpecifiers();
-  CodeCompletionDeclConsumer Consumer(Results, SemaRef.CurContext);
+  std::vector<FixItHint> FixIts;
+  if (AccessOpFixIt)
+      FixIts.emplace_back(AccessOpFixIt.getValue());
+  CodeCompletionDeclConsumer Consumer(Results, SemaRef.CurContext, std::move(FixIts));
   SemaRef.LookupVisibleDecls(RD, Sema::LookupMemberName, Consumer,
                              SemaRef.CodeCompleter->includeGlobals(),
                              /*IncludeDependentBases=*/true,
                              SemaRef.CodeCompleter->loadExternal());
 
   if (SemaRef.getLangOpts().CPlusPlus) {
     if (!Results.empty()) {
       // The "template" keyword can follow "->" or "." in the grammar.
if (!Results.empty()) {
 
       if (IsDependent)
         Results.AddResult(CodeCompletionResult("template"));
     }
   }
 }
 
 void Sema::CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base,
+                                           Expr *OtherOpBase,
                                            SourceLocation OpLoc, bool IsArrow,
                                            bool IsBaseExprStatement) {
   if (!Base || !CodeCompleter)
     return;
-  
   ExprResult ConvertedBase = PerformMemberExprBaseConversion(Base, IsArrow);
   if (ConvertedBase.isInvalid())
     return;
-  Base = ConvertedBase.get();
-  
-  QualType BaseType = Base->getType();
+  QualType ConvertedBaseType = ConvertedBase.get()->getType();
+  enum CodeCompletionContext::Kind contextKind;
 
   if (IsArrow) {
-    if (const PointerType *Ptr = BaseType->getAs<PointerType>())
-      BaseType = Ptr->getPointeeType();
-    else if (BaseType->isObjCObjectPointerType())
-      /*Do nothing*/ ;
-    else
-      return;
+    if (const PointerType *Ptr = ConvertedBaseType->getAs<PointerType>())
+      ConvertedBaseType = Ptr->getPointeeType();
   }
-  
-  enum CodeCompletionContext::Kind contextKind;
-  
   if (IsArrow) {
     contextKind = CodeCompletionContext::CCC_ArrowMemberAccess;
-  }
-  else {
-    if (BaseType->isObjCObjectPointerType() ||
-        BaseType->isObjCObjectOrInterfaceType()) {
+  } else {
+    if (ConvertedBaseType->isObjCObjectPointerType() ||
+        ConvertedBaseType->isObjCObjectOrInterfaceType()) {
       contextKind = CodeCompletionContext::CCC_ObjCPropertyAccess;
-    }
-    else {
+    } else {
       contextKind = CodeCompletionContext::CCC_DotMemberAccess;
     }
   }
 
-  CodeCompletionContext CCContext(contextKind, BaseType);
+  CodeCompletionContext CCContext(contextKind, ConvertedBaseType);
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
-                        CodeCompleter->getCodeCompletionTUInfo(),
-                        CCContext,
+                        CodeCompleter->getCodeCompletionTUInfo(), CCContext,
                         &ResultBuilder::IsMember);
-  Results.EnterNewScope();
+
+  auto DoCompletion = [&](Expr *Base, bool IsArrow, Optional<FixItHint> AccessOpFixIt) -> bool {
+    if (!Base)
+      return false;
+
+    ExprResult ConvertedBase = PerformMemberExprBaseConversion(Base, IsArrow);
+    if (ConvertedBase.isInvalid())
+      return false;
+    Base = ConvertedBase.get();
+
+    QualType BaseType = Base->getType();
+
+    if (IsArrow) {
+      if (const PointerType *Ptr = BaseType->getAs<PointerType>())
+        BaseType = Ptr->getPointeeType();
+      else if (BaseType->isObjCObjectPointerType())
+        /*Do nothing*/;
+      else
+        return false;
+    }
+
     if (const RecordType *Record = BaseType->getAs<RecordType>()) {
       AddRecordMembersCompletionResults(*this, Results, S, BaseType,
-                                      Record->getDecl());
-  } else if (const auto *TST = BaseType->getAs<TemplateSpecializationType>()) {
+                                        Record->getDecl(),
+                                        std::move(AccessOpFixIt));
+    } else if (const auto *TST =
+                   BaseType->getAs<TemplateSpecializationType>()) {
       TemplateName TN = TST->getTemplateName();
       if (const auto *TD =
               dyn_cast_or_null<ClassTemplateDecl>(TN.getAsTemplateDecl())) {
         CXXRecordDecl *RD = TD->getTemplatedDecl();
-      AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD);
+        AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD,
+                                          std::move(AccessOpFixIt));
       }
     } else if (const auto *ICNT = BaseType->getAs<InjectedClassNameType>()) {
       if (auto *RD = ICNT->getDecl())
-      AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD);
+        AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD,
+                                          std::move(AccessOpFixIt));
     } else if (!IsArrow && BaseType->isObjCObjectPointerType()) {
       // Objective-C property reference.
       AddedPropertiesSet AddedProperties;
 
       if (const ObjCObjectPointerType *ObjCPtr =
               BaseType->getAsObjCInterfacePointerType()) {
         // Add property results based on our interface.
         assert(ObjCPtr && "Non-NULL pointer guaranteed above!");
         AddObjCProperties(CCContext, ObjCPtr->getInterfaceDecl(), true,
                           /*AllowNullaryMethods=*/true, CurContext,
                           AddedProperties, Results, IsBaseExprStatement);
       }
 
       // Add properties from the protocols in a qualified interface.
       for (auto *I : BaseType->getAs<ObjCObjectPointerType>()->quals())
         AddObjCProperties(CCContext, I, true, /*AllowNullaryMethods=*/true,
                           CurContext, AddedProperties, Results,
                           IsBaseExprStatement);
     } else if ((IsArrow && BaseType->isObjCObjectPointerType()) ||
                (!IsArrow && BaseType->isObjCObjectType())) {
       // Objective-C instance variable access.
       ObjCInterfaceDecl *Class = nullptr;
-    if (const ObjCObjectPointerType *ObjCPtr
-                                    = BaseType->getAs<ObjCObjectPointerType>())
+      if (const ObjCObjectPointerType *ObjCPtr =
+              BaseType->getAs<ObjCObjectPointerType>())
         Class = ObjCPtr->getInterfaceDecl();
       else
         Class = BaseType->getAs<ObjCObjectType>()->getInterface();
-    
       // Add all ivars from this class and its superclasses.
       if (Class) {
         CodeCompletionDeclConsumer Consumer(Results, CurContext);
         Results.setFilter(&ResultBuilder::IsObjCIvar);
         LookupVisibleDecls(
             Class, LookupMemberName, Consumer, CodeCompleter->includeGlobals(),
             /*IncludeDependentBases=*/false, CodeCompleter->loadExternal());
       }
     }
-  
     // FIXME: How do we cope with isa?
-  
+    return true;
+  };
+
+  Results.EnterNewScope();
+
+  bool CompletionSucceded = DoCompletion(Base, IsArrow, None);
+  if (CodeCompleter->includeFixIts()) {
+    const CharSourceRange OpRange =
+        CharSourceRange::getTokenRange(OpLoc, OpLoc);
+    CompletionSucceded |= DoCompletion(
+        OtherOpBase, !IsArrow,
+        FixItHint::CreateReplacement(OpRange, IsArrow ? "." : "->"));
+  }
   Results.ExitScope();
 
+  if (!CompletionSucceded)
+    return;
+
   // Hand off the results found for code completion.
-  HandleCodeCompleteResults(this, CodeCompleter, 
-                            Results.getCompletionContext(),
-                            Results.data(),Results.size());
+  HandleCodeCompleteResults(this, CodeCompleter, Results.getCompletionContext(),
+                            Results.data(), Results.size());
 }
 
 void Sema::CodeCompleteObjCClassPropertyRefExpr(Scope *S,
                                                 IdentifierInfo &ClassName,
                                                 SourceLocation ClassNameLoc,
                                                 bool IsBaseExprStatement) {
   IdentifierInfo *ClassNamePtr = &ClassName;
   ObjCInterfaceDecl *IFace = getObjCInterfaceDecl(ClassNamePtr, ClassNameLoc);

cfe/trunk/test/CodeCompletion/member-access.cpp

 // CHECK-CC7: o1 : [#BaseTemplate<int>#]o1
 // CHECK-CC7: o2 : [#BaseTemplate<T>#]o2
 // CHECK-CC7: staticFn : [#void#]staticFn(<#T &obj#>)
 // CHECK-CC7: Template : Template
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:158:16 %s -o - | FileCheck -check-prefix=CHECK-CC7 %s
   typename Template<T>::Nested m;
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:166:25 %s -o - | FileCheck -check-prefix=CHECK-CC7 %s
 }
+
+class Proxy2 {
+public:
+  Derived *operator->() const;
+  int member5;
+};
+
+void test2(const Proxy2 &p) {
+  p->
+}
+
+void test3(const Proxy2 &p) {
+  p.
+}
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-with-fixits -code-completion-at=%s:177:6 %s -o - | FileCheck -check-prefix=CHECK-CC8 --implicit-check-not="Derived : Derived(" %s
+// CHECK-CC8: Base1 : Base1::
+// CHECK-CC8: member1 : [#int#][#Base1::#]member1
+// CHECK-CC8: member1 : [#int#][#Base2::#]member1
+// CHECK-CC8: member2 : [#float#][#Base1::#]member2
+// CHECK-CC8: member3 : [#double#][#Base2::#]member3
+// CHECK-CC8: member4 : [#int#]member4
+// CHECK-CC8: member5 : [#int#]member5 (requires fix-it: {177:4-177:6} to ".")
+// CHECK-CC8: memfun1 : [#void#][#Base3::#]memfun1(<#float#>)
+// CHECK-CC8: memfun1 : [#void#][#Base3::#]memfun1(<#double#>)[# const#]
+// CHECK-CC8: memfun1 (Hidden) : [#void#]Base2::memfun1(<#int#>)
+// CHECK-CC8: memfun2 : [#void#][#Base3::#]memfun2(<#int#>)
+// CHECK-CC8: memfun3 : [#int#]memfun3(<#int#>)
+// CHECK-CC8: operator-> : [#Derived *#]operator->()[# const#] (requires fix-it: {177:4-177:6} to ".")
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-with-fixits -code-completion-at=%s:181:6 %s -o - | FileCheck -check-prefix=CHECK-CC9 --implicit-check-not="Derived : Derived(" %s
+// CHECK-CC9: Base1 : Base1::
+// CHECK-CC9: member1 : [#int#][#Base1::#]member1 (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: member1 : [#int#][#Base2::#]member1 (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: member2 : [#float#][#Base1::#]member2 (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: member3 : [#double#][#Base2::#]member3 (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: member4 : [#int#]member4 (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: member5 : [#int#]member5
+// CHECK-CC9: memfun1 : [#void#][#Base3::#]memfun1(<#float#>) (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: memfun1 : [#void#][#Base3::#]memfun1(<#double#>)[# const#] (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: memfun1 (Hidden) : [#void#]Base2::memfun1(<#int#>) (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: memfun2 : [#void#][#Base3::#]memfun2(<#int#>) (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: memfun3 : [#int#]memfun3(<#int#>) (requires fix-it: {181:4-181:5} to "->")
+// CHECK-CC9: operator-> : [#Derived *#]operator->()[# const#]