clang-tidy bug?

For the following code (wrong.cpp):

bool check(bool isValid)
{
bool retVal = false;

if (( isValid == true ))
{
retVal = true;
}

return retVal;
}

when I run:
clang-tidy -checks=modernize-use-default-member-init wrong.cpp

I get:
4 warnings and 1 error generated.
Error while processing /llvm/match/ctBad/wrong.cpp.
/llvm/match/ctBad/wrong.cpp:5:19: error: equality comparison with extraneous parentheses [clang-diagnostic-parentheses-equality]
if (( isValid == true ))
~ ^~ ~

clang-tidy in the command line you gave didn’t seem to modify the file for me, did it modify the file for you?

Are you objecting to the suggestion, or that it was automatically applied? I would think it’d be a bug to apply any fixit/hint if there are multiple possible suggestions.

But the existence of the suggestion (without the application of it) to the user seems right to me. The use of extra () to suppress the assignment-in-conditional warning (-Wparentheses) has been around for quite a while, so it’s possible that the user intended assignment rather than comparison when they added the extra parentheses.

  • Dave

Two separate issues here

  1. the fixit hint, as one of a set of alternatives, isn’t likely to be removed/changed - the (albeit quirky) convention of using extra () to indicate an intentional assignment in a condition has been around for a while. So if you use the extra parens without writing an assignment, the compiler’s going to suggest you resolve this “conflict” with the style - either you didn’t intend the extra (), or you intended to use assignment. Those are the two alternative suggestions being made.

  2. automatically applying one fixit hint of several ambiguous ones seems like a bug to me - Aaron - do you know anything about this? Is this accurate/intended/etc?

Two separate issues here

1) the fixit hint, as one of a set of alternatives, isn't likely to be removed/changed - the (albeit quirky) convention of using extra () to indicate an intentional assignment in a condition has been around for a while. So if you use the extra parens without writing an assignment, the compiler's going to suggest you resolve this "conflict" with the style - either you didn't intend the extra (), or you intended to use assignment. Those are the two alternative suggestions being made.

2) automatically applying one fixit hint of several ambiguous ones seems like a bug to me - Aaron - do you know anything about this? Is this accurate/intended/etc?

I also think that's a bug. It looks like it's coming from
Sema::DiagnoseEqualityWithExtraParens(). It looks like it presents
both fixits, which strikes me as a bad thing to do when automatically
applying fixits.

~Aaron

Two separate issues here

  1. the fixit hint, as one of a set of alternatives, isn’t likely to be removed/changed - the (albeit quirky) convention of using extra () to indicate an intentional assignment in a condition has been around for a while. So if you use the extra parens without writing an assignment, the compiler’s going to suggest you resolve this “conflict” with the style - either you didn’t intend the extra (), or you intended to use assignment. Those are the two alternative suggestions being made.

  2. automatically applying one fixit hint of several ambiguous ones seems like a bug to me - Aaron - do you know anything about this? Is this accurate/intended/etc?

I also think that’s a bug. It looks like it’s coming from
Sema::DiagnoseEqualityWithExtraParens(). It looks like it presents
both fixits, which strikes me as a bad thing to do when automatically
applying fixits.

These fixits should be attached to notes, though, right? & Clang produces all sorts of fixits on notes that are not semantics-preserving, I think? (“oh, I think you meant to write this other thing”)

My understanding is that the fixits are correct (in the clang diagnostic experience - “here’s a warning, here are some ideas of what you might’ve intended that would address the warning”) - but it seems incorrect to automatically apply especially ambiguous suggesitons like this. How would clang-tidy choose between the two alternatives?

>
> Two separate issues here
>
> 1) the fixit hint, as one of a set of alternatives, isn't likely to be removed/changed - the (albeit quirky) convention of using extra () to indicate an intentional assignment in a condition has been around for a while. So if you use the extra parens without writing an assignment, the compiler's going to suggest you resolve this "conflict" with the style - either you didn't intend the extra (), or you intended to use assignment. Those are the two alternative suggestions being made.
>
> 2) automatically applying one fixit hint of several ambiguous ones seems like a bug to me - Aaron - do you know anything about this? Is this accurate/intended/etc?

I also think that's a bug. It looks like it's coming from
Sema::DiagnoseEqualityWithExtraParens(). It looks like it presents
both fixits, which strikes me as a bad thing to do when automatically
applying fixits.

These fixits should be attached to notes, though, right? & Clang produces all sorts of fixits on notes that are not semantics-preserving, I think? ("oh, I think you meant to write this other thing")

Yes, they're both attached to notes but the notes point to the same
location as the error.

My understanding is that the fixits are correct (in the clang diagnostic experience - "here's a warning, here are some ideas of what you might've intended that would address the warning") - but it seems incorrect to automatically apply especially ambiguous suggesitons like this. How would clang-tidy choose between the two alternatives?

I don't think it has a way to select between alternatives; I don't
think that was a use case we had envisioned for automatically applying
fix-its.

~Aaron

Two separate issues here

  1. the fixit hint, as one of a set of alternatives, isn’t likely to be removed/changed - the (albeit quirky) convention of using extra () to indicate an intentional assignment in a condition has been around for a while. So if you use the extra parens without writing an assignment, the compiler’s going to suggest you resolve this “conflict” with the style - either you didn’t intend the extra (), or you intended to use assignment. Those are the two alternative suggestions being made.

  2. automatically applying one fixit hint of several ambiguous ones seems like a bug to me - Aaron - do you know anything about this? Is this accurate/intended/etc?

I also think that’s a bug. It looks like it’s coming from
Sema::DiagnoseEqualityWithExtraParens(). It looks like it presents
both fixits, which strikes me as a bad thing to do when automatically
applying fixits.

These fixits should be attached to notes, though, right? & Clang produces all sorts of fixits on notes that are not semantics-preserving, I think? (“oh, I think you meant to write this other thing”)

Yes, they’re both attached to notes but the notes point to the same
location as the error.

My understanding is that the fixits are correct (in the clang diagnostic experience - “here’s a warning, here are some ideas of what you might’ve intended that would address the warning”) - but it seems incorrect to automatically apply especially ambiguous suggesitons like this. How would clang-tidy choose between the two alternatives?

I don’t think it has a way to select between alternatives; I don’t
think that was a use case we had envisioned for automatically applying
fix-its.

I wasn’t thinking a way to select - but I’d expect an automated tool to, when presented with an ambiguity, decide that not doing anything was the best course of action (same as if the warning had no notes).

>
>
>
>>
>> >
>> > Two separate issues here
>> >
>> > 1) the fixit hint, as one of a set of alternatives, isn't likely to be removed/changed - the (albeit quirky) convention of using extra () to indicate an intentional assignment in a condition has been around for a while. So if you use the extra parens without writing an assignment, the compiler's going to suggest you resolve this "conflict" with the style - either you didn't intend the extra (), or you intended to use assignment. Those are the two alternative suggestions being made.
>> >
>> > 2) automatically applying one fixit hint of several ambiguous ones seems like a bug to me - Aaron - do you know anything about this? Is this accurate/intended/etc?
>>
>> I also think that's a bug. It looks like it's coming from
>> Sema::DiagnoseEqualityWithExtraParens(). It looks like it presents
>> both fixits, which strikes me as a bad thing to do when automatically
>> applying fixits.
>
>
> These fixits should be attached to notes, though, right? & Clang produces all sorts of fixits on notes that are not semantics-preserving, I think? ("oh, I think you meant to write this other thing")

Yes, they're both attached to notes but the notes point to the same
location as the error.

> My understanding is that the fixits are correct (in the clang diagnostic experience - "here's a warning, here are some ideas of what you might've intended that would address the warning") - but it seems incorrect to automatically apply especially ambiguous suggesitons like this. How would clang-tidy choose between the two alternatives?

I don't think it has a way to select between alternatives; I don't
think that was a use case we had envisioned for automatically applying
fix-its.

I wasn't thinking a way to select - but I'd expect an automated tool to, when presented with an ambiguity, decide that not doing anything was the best course of action (same as if the warning had no notes).

I agree. I've CCed Alex in case he has opinions as well.

~Aaron