[cfe-commits] PATCH: Generate more useful messages for out-of-line definition errors

[Kaelyn Uhrain]

On Wed, Aug 3, 2011 at 2:48 PM, Douglas Gregor <dgregor at apple.com> wrote:

> Hi Kaelyn,
>
> On Jul 28, 2011, at 1:53 PM, Kaelyn Uhrain wrote:
>
> On Wed, Jul 27, 2011 at 11:20 AM, Douglas Gregor <dgregor at apple.com>wrote:
>
>>
>> On Jul 25, 2011, at 6:12 PM, Kaelyn Uhrain wrote:
>>
>> Doug,
>>
>> Thanks for the review. I've attached an updated patch (and uploaded the
>> patchset to Rietveld) incorporating your feedback. The new patch is smaller
>> and touches fewer files. :)
>>
>> On Mon, Jul 25, 2011 at 11:50 AM, Douglas Gregor <dgregor at apple.com>wrote:
>>
>>>
>>> On Jul 25, 2011, at 11:38 AM, Kaelyn Uhrain wrote:
>>>
>>> On Mon, Jul 25, 2011 at 11:13 AM, Douglas Gregor <dgregor at apple.com>wrote:
>>>
>>>>
>>>> On Jul 22, 2011, at 4:19 PM, Kaelyn Uhrain wrote:
>>>>
>>>> Here's a patch to improve the messages for out-of-line definition errors
>>>> by including the function's signature along with its name, and in the
>>>> candidate function notes including the candidates name and signature as
>>>> namespace qualifiers aren't always apparent at the line of code with the
>>>> declaration.
>>>>
>>>> The patch is also available for review at
>>>> http://codereview.appspot.com/4817047
>>>>
>>>> For example, given a file tmp.cpp containing:
>>>>
>>>> namespace N2 {
>>>>  struct S1;
>>>>
>>>>   namespace N1 {
>>>>    struct S2 {
>>>>      void func(S1*);
>>>>    };
>>>>
>>>>    struct S1 {};
>>>>   }
>>>> }
>>>> void N2::N1::S2::func(S1*) {}
>>>>
>>>> Clang currently reports:
>>>>
>>>> tmp.cpp:12:18: error: out-of-line definition of 'func' does not match
>>>> any declaration in 'N2::N1::S2'
>>>> void N2::N1::S2::func(S1*) {}
>>>>      ~~~~~~~~~~~~^
>>>>
>>>> Sadly, g++ (version 4.4.3) gives better messages:
>>>>
>>>> tmp.cpp:12: error: prototype for 'void N2::N1::S2::func(N2::N1::S1*)'
>>>> does not match any in class 'N2::N1::S2'
>>>> tmp.cpp:6: error: candidate is: void N2::N1::S2::func(N2::S1*)
>>>>
>>>> With this patch, clang yields:
>>>>
>>>> tmp.cpp:12:18: error: out-of-line definition of 'void func(N2::N1::S1
>>>> *)' does not match any
>>>>       declaration in 'N2::N1::S2'
>>>> void N2::N1::S2::func(S1*) {}
>>>>      ~~~~~~~~~~~~^
>>>> tmp.cpp:6:11: note: member declaration 'void func(N2::S1 *)' nearly
>>>> matches
>>>>      void func(S1*);
>>>>           ^
>>>>
>>>>
>>>> I'm not convinced we want to go in this direction. Or general principle
>>>> for Clang diagnostics has been "show, don't tell", and we'd much rather
>>>> point out specifically where the mismatch occurs than print out the entire
>>>> type of the function and force the user to walk through the parameters to
>>>> find the mismatch. This approach also allows us to add Fix-Its when the
>>>> mismatch is due to something easily fixed (e.g., missing cv-qualifiers).
>>>>
>>>
>>> Clang currently only suggests match candidates if there is a slight
>>> mismatch such as missing cv-qualifers, but all of the underlying
>>> (unqualified) types have to match. My patch just expands that to match cases
>>> where the types are different but have the same name--where there are
>>> missing/implicit namespace qualifers, which currently will give a totally
>>> useless and very baffling message to the user. In the example I gave
>>> previously, it says N2::N1::S2::func(S1*) is unmatched but the user will
>>> look in the code and see func(S1*) declared in struct N2::N1::S2 with no
>>> idea why clang wasn't accepting the message.
>>>
>>>
>>> I'm objecting more to the form of the diagnostic than anything else.
>>> Printing the full function signature is going to make the diagnostic text
>>> very, very long. I'd rather have the diagnostic focus in on the specific
>>> parameter that mismatched, e.g.,
>>>
>>>  note: first parameter of member declaration has a type incompatible with
>>> corresponding parameter in the out-of-line definition ('N2::S1 *' vs.
>>> 'N2::N1::S1*')
>>>
>>> Otherwise, we're sending the user hunting for the problem for any
>>> function with more than one parameter.
>>>
>>
>> Yeah, the form of the diagnostic was the part I was most unsure about.
>> With my new patch, the original example will now yield:
>>
>>  tmp.cpp:12:18: error: out-of-line definition of 'func' does not match any
>> declaration in 'N2::N1::S2'
>> void N2::N1::S2::func(S1*) {}
>>      ~~~~~~~~~~~~^
>> tmp.cpp:6:16: note: Type of 1st parameter of member declaration does not
>> match definition: 'N2::S1 *'
>>       vs 'N2::N1::S1 *'
>>      void func(S1*);
>>                ^
>> 1 error generated.
>>
>> Mismatched built-in types such as double* instead of double now print the
>> above note too.
>>
>>
>> Great! That's a big improvement.
>>
>> The original cases where clang would output a "member declaration nearly
>> matches" note (where ASTContext::hasSameUnqualifiedType returns true but the
>> qualified types differ, e.g. double vs double&) are now back to how they
>> currently are in svn--shall I replace those cases in
>> isNearlyMatchingFunction to also use the new note instead of "member
>> declaration nearly matches"?
>>
>>
>> Yes, definitely. My old "member declaration nearly matches" stuff is
>> really lame; your patch is far better. And we can specialize it further for
>> common issues (wrong cv qualifiers on the member function, for example).
>>
>
> I've expanded the new message to apply to any mismatched parameter
> including ones with missing qualifiers. The old message still is used for
> cases where the functions don't quite match but have identical parameter
> lists.
>
> Yeah, adding fixits in the cases that makes sense would be really cool but
>> for a separate patch. My comments regarding fixits was more just me dumping
>> what my thinking was regarding the diagnostic message. ;)
>>
>>
>> Agreed that Fix-Its can be a follow-on patch. This patch already improves
>> things quite a bit.
>>
>> P.S. It just occurred to me that capturing the parameters for which
>> hasSameUnqualifiedType is true and the qualified types are different (like
>> my patch already does for cases where hasSameUnqualifiedType is false but
>> the base type names without namespace qualifiers are the same) would
>> simplify adding fixits for those cases.
>> <out-of-line-def-error2.diff>
>>
>>
>> +  /// Retrieves the base type, e.g. for a "const int*" returns the
>> QualType for
>> +  /// int, but for a "double" returns itself.
>> +  const QualType getBaseType() const;
>>
>> I have a hard time calling this the "base type". I'm not sure what name to
>> give it, but "base type" sounds too much like inheritance. Perhaps I'm
>> confused because...
>>
>
> I've replaced QualType::getBaseType with a non-recursive static function in
> SemaDecl.cpp (called getCoreType to help it sound less related to
> inheritance). If you'd rather keep QualType::getBaseType (possibly renamed),
> I committed its removal to my local git tree separately from the rest of the
> changes this patch.
>
>
>>      if
>> (!Context.hasSameUnqualifiedType(DeclParamTy.getNonReferenceType(),
>> -
>>  DefParamTy.getNonReferenceType()))
>> -      return false;
>> +
>>  DefParamTy.getNonReferenceType())) {
>> +      if (DeclParamBaseTy == DefParamBaseTy ||
>> +          (DeclTyName && DeclTyName == DefTyName))
>> +        Params.push_back(Idx);
>> +      else
>> +        return false;
>> +    }
>>    }
>>
>> I don't see why we're bothering with the name checks here. Perhaps the
>> presence of isNearlyMatchingFunction() is actually confusing things. I,
>> personally, would rather see something more like overload resolution:
>> provide a list of possible matches, and for each describe the first problem
>> that caused it not to match. Over time, we can hone in the diagnostics (with
>> Fix-Its for common issues), rank the candidates and perhaps prune the worst
>> of the candidates. isNearlyMatchingFunction() is the wrong abstraction, and
>> while your patch makes it better, I think we need to drop the
>> "nearly-matching" focus to make progress.
>>
>
> I looked at the overload resolution code and from what I could tell the
> only real difference between what it does and what isNearlyMatchingFunction
> does (not counting the ranking the overload resolution does to try to come
> up with a single "best" match) is that the overload resolution handles
> mismatched argument types by attempting implicit type conversions. Since
> we're trying to match declarations and not a call to a declaration,
> parameter type conversions are irrelevant; what is needed is to determine if
> the parameters in a given position are similar enough to be what the
> programmer may have meant. Part of that determination is to see if the
> unqualified non-reference types match, and this all that was originally done
> for the parameters. I've added two more pieces to that fuzzy matching:
>    1) do the underlying (or "core") types match, regardless of '&', '*'. or
> '[]', i.e. is the type a double* instead of a double or a const &int[]
> instead of an ∫ or
>    2) do the textual names of the types--sans namespace qualifiers and
> such--match, i.e. was "S1" typed when "N2::S1" needed to be typed because
> the compiler treats the plain "S1" as "N2::N1::S1" in the definition even
> though it resolved a plain "S1" to "N2::S1" in the declaration.
>
> Also, building a list of possible matches and choosing between them would
> be independent of the changes to isNearlyMatchingFunction as that function
> is just a helper called by DiagnoseInvalidRedeclaration to compare a single
> function declaration with the redeclaration being diagnosed.
> DiagnoseInvalidRedeclaration handles the set of function declarations with
> the same name as the redeclaration being diagnosed; right now
> DiagnoseInvalidRedeclaration just prints out notes about the candidates it
> finds that are similar enough ("nearly matching") as it finds them. I feel
> ranking and further pruning the set of candidates is a refinement for a
> later patch, to help move closer to providing fixits for common issues while
> also improving the ordering of notes in cases where the invalid
> redeclaration is for an overloaded function. This patch is more about
> improving the search for likely candidates (and improving the contents of
> each displayed note) than about figuring out which of the candidates found
> is most likely what the programmer meant. ;)
>
>
> I think this is a great step forward. I have only one trivial request
> before you commit it:
>
> ndex 239bd45..f5dd1fb 100644
> --- a/include/clang/Basic/DiagnosticSemaKinds.td
> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2965,6 +2965,9 @@ def warn_member_extra_qualification : Warning<
>  def err_member_qualification : Error<
>    "non-friend class member %0 cannot have a qualified name">;
>  def note_member_def_close_match : Note<"member declaration nearly
> matches">;
> +def note_member_def_close_param_match : Note<
> +  "Type of %ordinal0 parameter of member declaration does not match "
> +  "definition: %1 vs %2">;
>
> "Type" should not be capitalized. I also see that it's more common for us
> to use "(%1 vs %2)" rather than ": %1 vs %2".
>

Changed and changed.


>
> And we're totally inconsistent about "vs" vs. "vs." Or is it "vs." vs "vs"
> that we're inconsistent about?
>

Right now the score between "vs" and "vs." is tied 3 to 3.

Cheers,
Kaelyn
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110804/0677fa07/attachment.html>