Re: [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993)

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2018-09-24 at 10:56 -0600, Martin Sebor wrote:
> On 09/21/2018 04:09 PM, David Malcolm wrote:
> > This is v2 of the patch; I managed to bit-rot my own patch due to
> > my
> > fix for r264335, which tightened up the "is this meaningful"
> > threshold
> > on edit distances when finding spelling correction candidates.
> > 
> > The only change in this version is to rename various things in
> > the testcase so that they continue to be suggested
> > ("colour" vs "m_color" are no longer near enough, so I renamed
> > "colour" to "m_colour").
> > 
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > 
> > OK for trunk?
> > 
> > Blurb from v1:
> > 
> > PR c++/84993 identifies a problem with our suggestions for
> > misspelled member names in the C++ FE for the case where the
> > member is private.
> > 
> > For example, given:
> > 
> > class foo
> > {
> > public:
> >   double get_ratio() const { return m_ratio; }
> > 
> > private:
> >   double m_ratio;
> > };
> > 
> > void test(foo *ptr)
> > {
> >   if (ptr->ratio >= 0.5)
> >     ;// etc
> > }
> > 
> > ...we currently emit this suggestion:
> > 
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> > you mean 'm_ratio'?
> >    if (ptr->ratio >= 0.5)
> >             ^~~~~
> >             m_ratio
> > 
> > ...but if the user follows this suggestion, they get:
> > 
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'double foo::m_ratio' is private within this
> > context
> >    if (ptr->m_ratio >= 0.5)
> >             ^~~~~~~
> > <source>:7:10: note: declared private here
> >    double m_ratio;
> >           ^~~~~~~
> > <source>:12:12: note: field 'double foo::m_ratio' can be accessed
> > via 'double foo::get_ratio() const'
> >    if (ptr->m_ratio >= 0.5)
> >             ^~~~~~~
> >             get_ratio()
> > 
> > It feels wrong to be emitting a fix-it hint that doesn't compile,
> > so this
> > patch adds the accessor fix-it hint logic to this case, so that we
> > directly
> > offer a valid suggestion:
> > 
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> > you mean
> > 'double foo::m_ratio'? (accessible via 'double foo::get_ratio()
> > const')
> >    if (ptr->ratio >= 0.5)
> >             ^~~~~
> >             get_ratio()
> 
> I wonder if suggesting the inaccessible member is at all helpful
> if it cannot be used.  

Of the two members "m_ratio" and "get_ratio", the winning candidate
based on edit-distance lookup was the inaccessible member; the accessor
is "further away" in terms of edit distance.  I think it's useful to
the user to identify both, as this patch does.

> Would mentioning only the accessor be
> a better approach?

I think it's more helpful to the user to be explicit about what the
compiler is "thinking", and mention both.  Consider the case where the
accessor has a wildy different name to the misspelled member:

class foo {
public:
  int get_pertinent_data () { return m_val; }

private:
  int m_val;
};

int test (foo *f)
{
   return f->val;
}

The patch emits:

t.c: In function ‘int test(foo*)’:
t.c:11:14: error: ‘class foo’ has no member named ‘val’; did you mean
‘int foo::m_val’? (accessible via ‘int foo::get_pertinent_data()’)
11 |    return f->val;
   |              ^~~
   |              get_pertinent_data()

which I think is clear and helpful.

> Also, to echo a comment made by someone else in a bug I don't
> remember, rather than phrasing the error in the form of
> a question ("did you mean x?") it might be better to either
> state what we know:
> 
>    error: 'class foo' has no member named 'ratio'; the nearest
>      match is 'double foo::m_ratio' (accessible via 'double
>      foo::get_ratio() const')
> 
> or offer a suggestion:
> 
>    error: 'class foo' has no member named 'ratio'; suggest
>      using 'double foo::get_ratio() const' instead

I think you're referring to PR 84890, though that bug was about this
kind of message:
  note: ‘INT_MAX’ is defined in header ‘<limits.h>’; did you forget to
‘#include <limits.h>’?

PR 84890 combines several concerns:
(a) is the message too verbose? (initial comment)
(b) is the message potentially annoying to the user? (comment 5)
(c) is it better to phrase the message as a suggestion, rather than as
a question? (comment 8)

Taking these in turn:

(a) may apply to the missing include message, but I think the "did you
mean 'foo'" is short and clear

(b) both styles of message refer to the user as "you", which has a risk
of making things personal.  I think it's fine for the "did you mean"
case.  I think there's an argument that the "did you forget" could be
annoying - but that's not a concern for this patch.

(c) of the various wordings:
  (c.1) "did you mean 'foo'?" (as in the patch, and the status quo for
the rest of the C and C++ FE)
  (c.2) "suggest using 'foo' instead" doesn't seem grammatically
correct to me, in that it sounds like the user is being asked to
suggest something, rather than to change their code.  I think there's
an implicit "I" in there - "I suggest" - but after abbreviation it
seems clunky to me.
  (c.3) "suggestion: use 'foo'" might work, but (c.1) seems much
simpler to me

So I prefer the wording in the patch.  If we're going to change the
wording across the compiler, I'd prefer to do that in a separate patch.

> A different approach altogether to these spelling messages that
> occurs to me but that you may have already considered and rejected
> would be to do what GCC does for errors due to ambiguous overloads:
> i.e., enumerate the available candidates.  This would work well in
> cases when multiple members are a close match.  It would also make
> it possible to explain, for each candidate member, whether it's
> accessible.

I hadn't thought of that approach, but my hunch is that there's likely
to only be one good suggestion: this use-case is trying to handle the
case where the user is trying to access a field of a class, and can't
quite remember the spelling, or what the accessor is.

FWIW I actually have another patch I'm about to post which updates
those available candidates suggestions (it special-cases the common
case where there's just one candidate: it uses the "did you mean %qs?"
wording, and the user gets a fix-it hint inline, and two less "note"
diagnostics)

Dave