Add a clang-tidy check for inadvertent conversions

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev
Hi Everyone,
I am considering contributing a clang-tidy check that will also require changes to clang sources. I would like to solicit feedback from people in this list. Here is the idea.

The goal is to detect constructors unintentionally defined as converting (when someone has forgotten to declare one's constructor as explicit).
I want to add an attribute, call it, say, [[conversion]] or [[implicit]]. It applies only to constructors. I want to add another check to clang tidy, say cppcoreguidelinse-explicit-ctor (referring to http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-explicit).
The warning is triggered when:

1. We see a declaration of non-copy, non-move constructor that can be called with a single argument and it is neither declared explicit not annotated with attribute [[conversion]].

2. We see a declaration of a copy ctor or a move ctor or a constructor that cannot be called with a single argument which annotated with attribute [[conversion]].

Example:

```
Struct X
{
  X(X const&); // ok: copy-ctor is special
  X(int I, int j, int k); // ok: cannot be called with one arg
  X(int i, int j = 0); // warn: non-copy, non-move, non-explicit, can be called with one arg
  template <typename... T> X(T&&... args); // warn: can be called with 1 argument
  explicit X(double); // ok: explicit
  X(char) [[conversion]]; // ok: warning disabled by attribute

  explicit X(std::string) [[conversion]]; // warn: attribute is redundant
  X(char, char) [[conversion]]; // warn: attribute is redundant
};
```

Proposed implementation:
Currently the AST matchers are not capable of extracting information from user-defined attributes. Implementing such extraction is beyond my capacity. Instead, I could teach clang to recognize one more attribute, this [[conversion]], the same way as it recognizes [[noreturn]]. Hen adding a clang-tidy check would be quite trivial.

What do you think about it?

Regards,
Andrzej Krzemienski
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev
On Mon, Mar 19, 2018 at 2:40 PM, Krzemienski, Andrzej via cfe-dev
<[hidden email]> wrote:

> Hi Everyone,
> I am considering contributing a clang-tidy check that will also require changes to clang sources. I would like to solicit feedback from people in this list. Here is the idea.
>
> The goal is to detect constructors unintentionally defined as converting (when someone has forgotten to declare one's constructor as explicit).
> I want to add an attribute, call it, say, [[conversion]] or [[implicit]]. It applies only to constructors. I want to add another check to clang tidy, say cppcoreguidelinse-explicit-ctor (referring to http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-explicit).
> The warning is triggered when:
>
> 1. We see a declaration of non-copy, non-move constructor that can be called with a single argument and it is neither declared explicit not annotated with attribute [[conversion]].
>
> 2. We see a declaration of a copy ctor or a move ctor or a constructor that cannot be called with a single argument which annotated with attribute [[conversion]].
>
> Example:
>
> ```
> Struct X
> {
>   X(X const&); // ok: copy-ctor is special
>   X(int I, int j, int k); // ok: cannot be called with one arg
>   X(int i, int j = 0); // warn: non-copy, non-move, non-explicit, can be called with one arg
>   template <typename... T> X(T&&... args); // warn: can be called with 1 argument
>   explicit X(double); // ok: explicit
>   X(char) [[conversion]]; // ok: warning disabled by attribute
>
>   explicit X(std::string) [[conversion]]; // warn: attribute is redundant
>   X(char, char) [[conversion]]; // warn: attribute is redundant
> };
> ```
FWIW there already is a clang-tidy google-explicit-constructor check,
https://clang.llvm.org/extra/clang-tidy/checks/google-explicit-constructor.html
and one can silence the selected cases with  // NOLINT
And with newest clang-tidy versions support //
NOLINT(google-explicit-constructor)
(See http://clang.llvm.org/extra/clang-tidy/ )

So i may have missed something, but i think the main idea is already
supported, although not as pretty..

> Proposed implementation:
> Currently the AST matchers are not capable of extracting information from user-defined attributes. Implementing such extraction is beyond my capacity. Instead, I could teach clang to recognize one more attribute, this [[conversion]], the same way as it recognizes [[noreturn]]. Hen adding a clang-tidy check would be quite trivial.
>
> What do you think about it?
>
> Regards,
> Andrzej Krzemienski
Roman.

> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev


-----Original Message-----
From: Roman Lebedev [mailto:[hidden email]]
Sent: Monday, March 19, 2018 1:50 PM
To: Krzemienski, Andrzej <[hidden email]>
Cc: [hidden email]
Subject: Re: [cfe-dev] Add a clang-tidy check for inadvertent conversions

On Mon, Mar 19, 2018 at 2:40 PM, Krzemienski, Andrzej via cfe-dev <[hidden email]> wrote:

> > Hi Everyone,
> > I am considering contributing a clang-tidy check that will also require changes to clang sources. I would like to solicit feedback from people in this list. Here is the idea.
>
> > The goal is to detect constructors unintentionally defined as converting (when someone has forgotten to declare one's constructor as explicit).
> > I want to add an attribute, call it, say, [[conversion]] or [[implicit]]. It applies only to constructors. I want to add another check to clang tidy, say cppcoreguidelinse-explicit-ctor (referring to http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-explicit).
> > The warning is triggered when:
>
> > 1. We see a declaration of non-copy, non-move constructor that can be called with a single argument and it is neither declared explicit not annotated with attribute [[conversion]].
>
> > 2. We see a declaration of a copy ctor or a move ctor or a constructor that cannot be called with a single argument which annotated with attribute [[conversion]].
>
> > Example:
>
> > ```
> > Struct X
> > {
> >   X(X const&); // ok: copy-ctor is special
> >   X(int I, int j, int k); // ok: cannot be called with one arg
> >   X(int i, int j = 0); // warn: non-copy, non-move, non-explicit, can be called with one arg
> >   template <typename... T> > X(T&&... args); // warn: can be called with 1 argument
> >   explicit X(double); // ok: explicit
> >   X(char) [[conversion]]; // ok: warning disabled by attribute
>
> >   explicit X(std::string) [[conversion]]; // warn: attribute is redundant
> >   X(char, char) [[conversion]]; // warn: attribute is redundant }; ```
> FWIW there already is a clang-tidy google-explicit-constructor check, https://clang.llvm.org/extra/clang-tidy/checks/google-explicit-constructor.html
> and one can silence the selected cases with  // NOLINT And with newest clang-tidy versions support //
> NOLINT(google-explicit-constructor)
> (See http://clang.llvm.org/extra/clang-tidy/ )

> So i may have missed something, but i think the main idea is already supported, although not as pretty..

Thanks, for bringing this up. Yes, in a way it does address the same problem, although I feel using it is more like a workaround rather than a proper solution.

Google's policy (and the corresponding check) is to consider any converting constructor a bug (or a problem, or whatever we want to call it). What I am after is to warn only about conversions that occur inadvertently, but not in the code where it is the programmers intention to provide the conversion. Warning on conversion operator (the Google check does this also) is something I definitely do not want.

It would be too much false positives to NOLINT.


> > Proposed implementation:
> > Currently the AST matchers are not capable of extracting information from user-defined attributes. Implementing such extraction is beyond my capacity. Instead, I could teach clang to recognize one more attribute, this [[conversion]], the same way as it recognizes [[noreturn]]. Hen adding a clang-tidy check would be quite trivial.


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    "Krzemienski, Andrzej via cfe-dev" <[hidden email]> writes:


> It would be too much false positives to NOLINT.

It seems that you have to do the edits to add the attribute, so how is
that different from doing the edits to add NOLINT?
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev


2018-03-20 22:03 GMT+01:00 Richard via cfe-dev <[hidden email]>:
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    "Krzemienski, Andrzej via cfe-dev" <[hidden email]> writes:


> It would be too much false positives to NOLINT.

It seems that you have to do the edits to add the attribute, so how is
that different from doing the edits to add NOLINT?

The way I see the difference is that putting a NOLINT is an indication of either:
1. An inaccurate check (it has false positives that I have to silence)
2. My inability to fix my code (because I am not in control of it, or because I have no time to investigate)

Silencing the warning with a dedicated attribute is saying: the check is accurate, my code is correct and clearly states intentions.

And there is still this other point. I only want to check for inadvertent conversions caused by programmer forgetting to declare her constructor explicit. I *do not want* to warn on non-explicit conversions operators: they are never added by omission.
If I go with google-explicit-constructor I will have to NOLINT every non-explicit conversion operator. If I go with the proposed check (which does not check conversion operators -- only constructors) I do not have to NOLINT any conversion operator.

Does this make sense?

Regards,
&rzej;


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev

I am also trying to determine if such a contribution would be accepted into clang and clang-tidy sources. This would help me decide if it is worth to invest time in this development. I wonder how this process of commits from external parties looks like.

 

Regards,

Andrzej

 

From: Andrzej Krzemienski [mailto:[hidden email]]
Sent: Monday, March 26, 2018 2:29 PM
To: Krzemienski, Andrzej <[hidden email]>
Subject: Fwd: [cfe-dev] Add a clang-tidy check for inadvertent conversions

 

 

---------- Forwarded message ----------
From: Andrzej Krzemienski <[hidden email]>
Date: 2018-03-21 1:00 GMT+01:00
Subject: Re: [cfe-dev] Add a clang-tidy check for inadvertent conversions
To: cfe-dev <[hidden email]>

 

 

2018-03-20 22:03 GMT+01:00 Richard via cfe-dev <[hidden email]>:

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    "Krzemienski, Andrzej via cfe-dev" <[hidden email]> writes:


> It would be too much false positives to NOLINT.

It seems that you have to do the edits to add the attribute, so how is
that different from doing the edits to add NOLINT?

 

The way I see the difference is that putting a NOLINT is an indication of either:

1. An inaccurate check (it has false positives that I have to silence)

2. My inability to fix my code (because I am not in control of it, or because I have no time to investigate)

Silencing the warning with a dedicated attribute is saying: the check is accurate, my code is correct and clearly states intentions.

And there is still this other point. I only want to check for inadvertent conversions caused by programmer forgetting to declare her constructor explicit. I *do not want* to warn on non-explicit conversions operators: they are never added by omission.

If I go with google-explicit-constructor I will have to NOLINT every non-explicit conversion operator. If I go with the proposed check (which does not check conversion operators -- only constructors) I do not have to NOLINT any conversion operator.

Does this make sense?

Regards,

&rzej;

 

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev
In reply to this post by Louis Dionne via cfe-dev
On Wed, Mar 21, 2018 at 3:00 AM, Andrzej Krzemienski via cfe-dev
<[hidden email]> wrote:

>
>
> 2018-03-20 22:03 GMT+01:00 Richard via cfe-dev <[hidden email]>:
>>
>> [Please reply *only* to the list and do not include my email directly
>> in the To: or Cc: of your reply; otherwise I will not see your reply.
>> Thanks.]
>>
>> In article
>> <[hidden email]>,
>>     "Krzemienski, Andrzej via cfe-dev" <[hidden email]> writes:
>>
>>
>> > It would be too much false positives to NOLINT.
>>
>> It seems that you have to do the edits to add the attribute, so how is
>> that different from doing the edits to add NOLINT?
>
>
> The way I see the difference is that putting a NOLINT is an indication of
> either:
> 1. An inaccurate check (it has false positives that I have to silence)
> 2. My inability to fix my code (because I am not in control of it, or
> because I have no time to investigate)
>
> Silencing the warning with a dedicated attribute is saying: the check is
> accurate, my code is correct and clearly states intentions.
>
> And there is still this other point. I only want to check for inadvertent
> conversions caused by programmer forgetting to declare her constructor
> explicit. I *do not want* to warn on non-explicit conversions operators:
> they are never added by omission.
> If I go with google-explicit-constructor I will have to NOLINT every
> non-explicit conversion operator.

> If I go with the proposed check (which does not check conversion operators -- only constructors)
Ah. Then i'd suggest to simply extend
clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
with two options, toggling checking of constructors, and conversion
operators. (both default to on)
That sounds so less intrusive, and cleaner, no?

> I do not have to NOLINT any conversion operator.
>
> Does this make sense?
>
> Regards,
> &rzej;
Roman.

> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev


2018-03-26 15:01 GMT+02:00 Roman Lebedev <[hidden email]>:
Ah. Then i'd suggest to simply extend
clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
with two options, toggling checking of constructors, and conversion
operators. (both default to on)
That sounds so less intrusive, and cleaner, no?

Oh, so the checks can have parameters, can't they? Is it possible to pass the parameters from clang-tidy's command line?

Regards,
&rzej;


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Add a clang-tidy check for inadvertent conversions

Louis Dionne via cfe-dev
[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAOenAXiKTCnpQx9YrNQF6v0G1=KggExYb=[hidden email]>,
    Andrzej Krzemienski via cfe-dev <[hidden email]> writes:

> 2018-03-26 15:01 GMT+02:00 Roman Lebedev <[hidden email]>:
>
> > Ah. Then i'd suggest to simply extend
> > clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
> > with two options, toggling checking of constructors, and conversion
> > operators. (both default to on)
> > That sounds so less intrusive, and cleaner, no?
> >
>
> Oh, so the checks can have parameters, can't they? Is it possible to pass
> the parameters from clang-tidy's command line?

Yes.  The syntax is awful, but it can be done.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev