Page MenuHomePhabricator

[clang][sema] Note decl location on missing member
AbandonedPublic

Authored by tbaeder on Jan 27 2021, 8:37 AM.

Details

Summary

Hi,

when trying to access a member of a namespace/class/struct/enum, clang currently just prints that the given namespace does not have a member with the given name, for example:

./enum.cpp:12:7: error: no member named 'b' in 'Foo'
  foo.b = 10;
  ~~~ ^
1 error generated.

However, for programmers it is usually also important to know where the namespace has been declared, e.g. in the example above it would be useful to know where foo's type has been declared. The following patch adds support for this by simply printing a note:

./enum.cpp:12:7: error: no member named 'b' in 'Foo'
  foo.b = 10;
  ~~~ ^
./enum.cpp:3:7: note: 'Foo' declared here
class Foo {
      ^~~
1 error generated.

That is of course a very simple example, but I think this adds value in real-world scenarios.

The patch is unfortunately all over the place and mostly updating test cases with the additional expected note.

In addition to the usual review I'd like opinions on the following open questions:

  • There are now three places where the patch adds the exact same code, would a helper function (in Sema?) be better and if so, where is the best place for it?
  • The notes are pretty clunky to read when the namespace is created by a lambda: " note: '(lambda at enum.cpp:41:12)' declared here" - should lambdas simply be ignored (how to do that?) or should I handle them differently?
  • The patch only emits the note if clang does not output a typo correction. I thought it might not be as useful in this case since it would clutter the output and chances are that the typo fix _is_ the right thing to show. Opinions on this?

Thanks,
Timm

Diff Detail

Event Timeline

tbaeder created this revision.Jan 27 2021, 8:37 AM
tbaeder requested review of this revision.Jan 27 2021, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 8:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 319671.Jan 27 2021, 2:13 PM

Forgot clang-format of course, sorry.

tbaeder updated this revision to Diff 319780.Jan 28 2021, 1:03 AM

Answered one of my questions by adding a new function directly to Sema.

tbaeder updated this revision to Diff 319820.Jan 28 2021, 3:38 AM

Fixed the Lambda oddity as well.

Any update on this?

Any update on this?

Thank you for the patch! Do you have some motivating examples of when this would really add clarity to the diagnostic? I'm not opposed to the patch per se, but I'm a bit wary about adding an additional note because that makes the diagnostic output seem that much longer, which makes the salient diagnostics a bit harder to find (colorization in the terminal helps with this, though).

The patch only emits the note if clang does not output a typo correction. I thought it might not be as useful in this case since it would clutter the output and chances are that the typo fix _is_ the right thing to show. Opinions on this?

I think that makes sense -- the typo correction already has a note that points to somewhere which is likely to get the user into the right area anyway: https://godbolt.org/z/1Yo3Pj

clang/include/clang/Sema/Sema.h
9795
clang/lib/Sema/Sema.cpp
2549
2553
2557–2560

Any update on this?

Thank you for the patch! Do you have some motivating examples of when this would really add clarity to the diagnostic? I'm not opposed to the patch per se, but I'm a bit wary about adding an additional note because that makes the diagnostic output seem that much longer, which makes the salient diagnostics a bit harder to find (colorization in the terminal helps with this, though).

I run into this a lot when looking into a larger, new code base. When adding new code, I often look at surrounding code and infer how e.g. an enum member is called. Take https://godbolt.org/z/Tcoxf5 for example, I could've seen code using OsType::Unix and inferred that the OsType for Windows will be called OsType::Windows, but it's Win32. The next step for me as a human is of course to grep the source code for the declaration of OsType and check the members. (On the other hand, a "Foo is not a member of enum FooEnum, existing members are: Bar1, Bar2, Bar3, ..." diagnostic would probably be more useful. But that has its own drawbacks).

Any update on this?

Thank you for the patch! Do you have some motivating examples of when this would really add clarity to the diagnostic? I'm not opposed to the patch per se, but I'm a bit wary about adding an additional note because that makes the diagnostic output seem that much longer, which makes the salient diagnostics a bit harder to find (colorization in the terminal helps with this, though).

I run into this a lot when looking into a larger, new code base. When adding new code, I often look at surrounding code and infer how e.g. an enum member is called. Take https://godbolt.org/z/Tcoxf5 for example, I could've seen code using OsType::Unix and inferred that the OsType for Windows will be called OsType::Windows, but it's Win32. The next step for me as a human is of course to grep the source code for the declaration of OsType and check the members. (On the other hand, a "Foo is not a member of enum FooEnum, existing members are: Bar1, Bar2, Bar3, ..." diagnostic would probably be more useful. But that has its own drawbacks).

Hmm... I feel like the diagnostic should already be sufficient to locate the originating location of the class or namespace and the note is adding a bit more (almost, but not quite) noise, but at the same time, I don't feel so strongly that I'd block the patch. I'd like to hear from other reviewers though.

As for some interesting test cases -- I don't think we should issue the note when the diagnostic already appears within the class declaration itself. e.g.,

struct S {
  void func();

  void other(S s) {
    s.foo(); // error, but no need to point to where S is declared
  }
};

Another similar case but involving an anonymous class:

struct S {
  struct {
    void func();
  } t;

  void foo() {
    t.blech(); // Should we do anything about this?
  }
};

A pathological case for C code, which shows another case where I think the note is more noise than anything (not that I expect anyone to ever write this code, so not worth special handling for unless it's fallout from other special handling code that's more useful):

int func(struct S { int a; } s) {
  return s.b;
}

Anonymous namespaces:

namespace foo {
namespace {
  void func();
}
}

void bar() {
  foo::blarg(); // Should point to 'foo'?
}

Hmm... I feel like the diagnostic should already be sufficient to locate the originating location of the class or namespace and the note is adding a bit more (almost, but not quite) noise,

I guess this makes sense when you're talking about https://godbolt.org/z/1Yo3Pj, but I don't understand how I would know where OsType is defined in https://godbolt.org/ in a larger code base (e.g. when using OsType in clang, with it being defined in llvm).

Anonymous namespaces:

namespace foo {
namespace {
  void func();
}
}

void bar() {
  foo::blarg(); // Should point to 'foo'?
}

Seems to work:

./enum.cpp:56:8: error: no member named 'blarg' in namespace 'foo'
  foo::blarg(); // Should point to 'foo'?
  ~~~~~^
./enum.cpp:49:11: note: namespace 'foo' declared here
namespace foo {
          ^~~

You list a few more interesting corner cases however. I'm not sure if I want to pursue this patch further as it is already quite ugly because it's touching all those tests. Or if it would be better to implement a note that lists all enum members (up to a certain threshold?), but just for enums.

Hmm... I feel like the diagnostic should already be sufficient to locate the originating location of the class or namespace and the note is adding a bit more (almost, but not quite) noise,

I guess this makes sense when you're talking about https://godbolt.org/z/1Yo3Pj, but I don't understand how I would know where OsType is defined in https://godbolt.org/ in a larger code base (e.g. when using OsType in clang, with it being defined in llvm).

I think that the name in the diagnostic should be sufficient for you to track that down using the editor of your choice (including super simple editors like notepad), such as by using a simple search. With that example, when I search for "enum OsType" in my editor, there's only one hit in the code base. Even in slightly more complex cases where you try to make things look ambiguous, the diagnostic text does a pretty good job of making it clear roughly where to look: https://godbolt.org/z/41nPnd

Anonymous namespaces:

namespace foo {
namespace {
  void func();
}
}

void bar() {
  foo::blarg(); // Should point to 'foo'?
}

Seems to work:

./enum.cpp:56:8: error: no member named 'blarg' in namespace 'foo'
  foo::blarg(); // Should point to 'foo'?
  ~~~~~^
./enum.cpp:49:11: note: namespace 'foo' declared here
namespace foo {
          ^~~

You list a few more interesting corner cases however. I'm not sure if I want to pursue this patch further as it is already quite ugly because it's touching all those tests. Or if it would be better to implement a note that lists all enum members (up to a certain threshold?), but just for enums.

I'd be worried that would add even more output that would largely be noise in the common case. My personal preference in this case would be to try to improve the existing diagnostics to be more clear as to what's referenced rather than adding an extra note. However, I won't block the patch based solely on my personal preference if you'd like to continue to pursue this -- I leave the final decision to @rsmith in that case.

tbaeder abandoned this revision.Apr 6 2021, 3:24 AM

Abandoning this since it's probably not worth it.