Page MenuHomePhabricator

[clangd] Handle enumerators in named, unscoped enums similarly to scoped enums
Needs ReviewPublic

Authored by malaperle on May 22 2018, 1:36 PM.

Details

Reviewers
ilya-biryukov
Summary

For enumerators in unscoped enums that have names, even if they are not
scoped, we add the enum name to the scope so that users can find the
enumerators when fully qualifying them, for example: MyEnum::Enumerator.
This makes it more consistent with how the code is visually laid out.

Signed-off-by: Marc-Andre Laperle <marc-andre.laperle@ericsson.com>

Event Timeline

malaperle created this revision.May 22 2018, 1:36 PM

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

It's one of those cases where code completion and workspace symbol search seem to want different results :-(
I suggest to add an extra string field for containing unscoped enum name, maybe into symbol details? And add a parameter to Index::fuzzyFind on whether we need to match enum scopes or not.
+@ioeric, +@sammccall, WDYT?

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

Not quite but still potentially problematic. With the patch, A, B, C would be found but not ::A, ::B, ::C.

It's one of those cases where code completion and workspace symbol search seem to want different results :-(
I suggest to add an extra string field for containing unscoped enum name, maybe into symbol details? And add a parameter to Index::fuzzyFind on whether we need to match enum scopes or not.
+@ioeric, +@sammccall, WDYT?

I'll wait to see what others think before changing it. But I feel it's a bit odd that completion and workspace symbols would be inconsistent. I'd rather have it that A, ::A, and Foo::A work for both completion and workspace. Maybe it would complicate things too much...

But I feel it's a bit odd that completion and workspace symbols would be inconsistent.

It does not seem that odd to me. Completion is something that should follow the language rules more closely, i.e. we don't want to deviate from sema completions too much. While workspaceSymbol is a user-facing feature and we probably want users to type as little as possible there.
E.g. not showing namespace_name::A in completion seems bad to me, since people are used to referring to the enumerators this way and sema completion gives you those results.

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

Not quite but still potentially problematic. With the patch, A, B, C would be found but not ::A, ::B, ::C.

It's one of those cases where code completion and workspace symbol search seem to want different results :-(
I suggest to add an extra string field for containing unscoped enum name, maybe into symbol details? And add a parameter to Index::fuzzyFind on whether we need to match enum scopes or not.
+@ioeric, +@sammccall, WDYT?

I'll wait to see what others think before changing it. But I feel it's a bit odd that completion and workspace symbols would be inconsistent. I'd rather have it that A, ::A, and Foo::A work for both completion and workspace. Maybe it would complicate things too much...

Having "wrong" names in workspaceSymbol (e.g. ::A instead of ::Foo::A) and missing results in code completions (e.g. ::A missing in ::^) seem equally bad.

I think the fundamental problem here is that C++ symbols (esp. enum constants) can have different names (e.g. ns::Foo::A and ns::A). We want ns::Foo::A for workspaceSymbols, but we also want ns::A for index-based code completions (sema completion would give us ns::Foo::A right? ). One possible solution would be adding a short name in Symbol (e.g. ns::A is a short name for ns::Foo::A), and index/fuzzyFind can match on both names.

clangd/index/SymbolCollector.cpp
365

drive-by nit: please pull this to a helper function.

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

Not quite but still potentially problematic. With the patch, A, B, C would be found but not ::A, ::B, ::C.

I don't understand how this would still work (at least when using the index). The index call would have Scopes set to the enclosing scopes {""}, which is equivalent to a search for ::A. Maybe I'm missing something.

I think the fundamental problem here is that C++ symbols (esp. enum constants) can have different names (e.g. ns::Foo::A and ns::A).

Yup. There are a few other instances of this:

  1. decls aliased with using decls - we model these as duplicate symbols
  2. decls aliased with using directives - we ask clang to resolve the namespaces in scope when searching
  3. inline namespaces - we mostly get away with ignoring them

AFAICS none of these are ideal for enums. (2 fails because the list of namespaces would get too long).
This is messy :-(

So to keep completion working, maybe we can give add enumerators with full qualification and have a flag that determines whether the last component of the qualifier can be ignored?
This would give a simple implementation path for now, and will add a semantic signal that we need to support enumerators, at least.
For example,

enum En {
  A,
};

enum class ScopedEn {
  A
};

will produce symbols with

[ { Scope = "En::", 
    InsideUnscopedEnum=true, 
    Name="A"}, 
  { Scope = "ScopedEn::",
    InsideUnscopedEnum = false,
    Name = "A" } ]

fuzzyFind will have 2 variants:

  • For code completion, it will return En::A only when queries with ::A (e.g. in global scope).
  • For workspace symbol, it can return En::A for A and En::A. For ::A it can return empty results.

Scoped enums work the same as before. Other case (using decls and inline namespaces) will also work as before.