This is an archive of the discontinued LLVM Phabricator instance.

Avoid printing some redundant name qualifiers in completion
ClosedPublic

Authored by ilya-biryukov on Oct 4 2017, 5:41 AM.

Details

Summary

Adjusted PrintingPolicy inside code completion to avoid printing some
redundant name qualifiers.

Before this change, typedefs that were written unqualified in source
code were printed with qualifiers in completion. For example, in the
following code

struct foo {
    typedef int type;
    type method();
};

completion item for method had return type of foo::type, even
though the original code used type without qualifiers.
After this change, the completion item has return type type, as
originally written in the source code.

Note that this change does not suppress qualifiers written by the
user. For example, in the following code

typedef int type;
struct foo {
    typedef int type;
    ::type method(foo::type);
};

completion item for method has return type of ::type and
parameter type of foo::type, as originally written in the source
code.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Oct 4 2017, 5:41 AM
ilya-biryukov added inline comments.Oct 4 2017, 5:45 AM
test/CodeCompletion/enum-switch-case-qualified.cpp
25 ↗(On Diff #117662)

This may be a somewhat unwanted part of this change.
Enum type is now written without qualifier here. I would argue that's ok, since the actual enum values are always properly qualified (they have to be, as they are actually inserted by completion) and those qualifiers provide all the necessary context for the user.

arphaman added inline comments.Oct 6 2017, 10:27 AM
test/CodeCompletion/call.cpp
22 ↗(On Diff #117662)

Could we also test a similar class to Y that's not typedefed, so you'd see f(N::Y)?

test/CodeCompletion/enum-switch-case-qualified.cpp
25 ↗(On Diff #117662)

I'm not 100% comfortable with making this kind of change right now. I'll try to investigate what's best for our users.

  • Restore qualifiers for types of EnumConstantDecl.
ilya-biryukov added inline comments.Oct 9 2017, 1:41 AM
test/CodeCompletion/enum-switch-case-qualified.cpp
25 ↗(On Diff #117662)

I changed code to keep qualifiers for enums as is.
(This required moving some code from libTooling to AST in order to reuse it, I'll create a separate review for that if you're ok with the overall change).

  • Included more cases into a qualifiers-as-written.cpp test.
ilya-biryukov added inline comments.Oct 9 2017, 8:02 AM
test/CodeCompletion/call.cpp
22 ↗(On Diff #117662)

Not sure if you expect this to behave differently, but I've updated qualifiers-as-written.cpp to include a similar case. You could take a look and validate whether it matches your expected behavior.

The general idea is to not add extra qualification in completion items, where possible, and prefer to show what was written in the source code.
This is what currently happens for all names that had name-qualifiers. However, before this patch, unqualified names would printed with an added name qualifiers.

This sometimes led to really bad results. One example is vector's push_back that roughly gets printed as (libstdc++ version) push_back(std::vector<int, std::allocator<int>>::value_type _Value). Whereas in the source code it's just push_back(value_type _Value).

arphaman added inline comments.Oct 10 2017, 11:08 AM
test/CodeCompletion/qualifiers-as-written.cpp
29 ↗(On Diff #118205)

Sorry, I see the issue now. However, I don't think that we'd like to change the signature for a function like this, as we'd still prefer to show func (foo::type, ns::bar, ns::baz); on this line.

In Xcode we actually avoid the problem with std::vector<>s that you've pointed out entirely by using value_type. I'll check what our solution does.

Btw, maybe using things like value_type is completely wrong (with or without the qualifier)? If we have std::vector<int> shouldn't we rather show push_back(int _Value), rather than the value_type? Perhaps this kind of change should be discussed with a wider community somehow to find out what's best for all users.

ilya-biryukov added inline comments.Oct 10 2017, 12:27 PM
test/CodeCompletion/qualifiers-as-written.cpp
29 ↗(On Diff #118205)

Using int _Value may be good in case of vector<int>, but it would probably loose typedefs, i.e. vector<int32> would still have int, which may be undesirable.
Also, for vector<vector<int>>, the type inside push_back would probably end up being vector<int, std::allocator<int>>.

As for the current vs new behavior, I can totally see why you want to see more context in completion items.

I would argue that the current code does not do a great job there, as it only adds qualifiers to unqualified references. But the context may be missing for qualified references as well.
For example, in the following code:

template <class T, class Alloc = std::allocator<T>>
struct vector {
  typedef T value_type;

  typename value_type::some_type foo();
  value_type bar()
};

Completion item for vector<X>::bar will have return type vector<X, std::allocator<X>>::value_type. However, completion item for vector<X, std::allocator<X>>::foo will have return type value_type::some_type (note that no vector<X, std::allocator<X>> qualifier was added).

I would also question the intent of the current implementation. The following line suggest that adding those qualifers was not intended in the first place:

Policy.SuppressUnwrittenScope = true;

But that's just a wild guess, I may be completely wrong.

That being said, I don't think there is one right preference, different people may prefer different options. We can surely discuss that in a wider community, though.

Would you be open to adding an option for that in completion and keeping the current behavior as a default?

Friendly ping.
Would it be okay to make the new behaviour optional? (Leaving old behaviour to be the default).

arphaman accepted this revision.Oct 23 2017, 3:38 PM
arphaman added inline comments.
test/CodeCompletion/qualifiers-as-written.cpp
29 ↗(On Diff #118205)

Sorry about the delay.

I clarified Xcode's behavior - we actually didn't show the qualifiers for typedefs and typealiases because of an older Clang, with ToT the behavior is as you describe.

So this patch will remove qualifiers for structs and classes, like in the func example here. I'm willing to accept it right now. We might have to revisit this change at some point though. Let's also keep one behavior right now, I don't think we need to diverge yet.

This revision is now accepted and ready to land.Oct 23 2017, 3:38 PM
  • Extracted the move of QualTypeNames to AST into a separate revision.
This revision was automatically updated to reflect the committed changes.