Add a clang-tidy check for inadvertent conversions

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 C++ Core Guidelines).
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

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 C++ Core Guidelines).
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 Clang-Tidy — Extra Clang Tools 16.0.0git documentation )

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.

> 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 C++ Core Guidelines).
> 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 Clang-Tidy — Extra Clang Tools 16.0.0git documentation )

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.

[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 <BN6PR13MB1569F3CBF08B69AED1DEC207E8D40@BN6PR13MB1569.namprd13.prod.outlook.com>,
    "Krzemienski, Andrzej via cfe-dev" <cfe-dev@lists.llvm.org> 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;

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

[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
<BN6PR13MB1569F3CBF08B69AED1DEC207E8D40@BN6PR13MB1569.namprd13.prod.outlook.com>,
    "Krzemienski, Andrzej via cfe-dev" <cfe-dev@lists.llvm.org> 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.

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;

[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=r0DXpfuNZg2GMCfQ@mail.gmail.com>,
    Andrzej Krzemienski via cfe-dev <cfe-dev@lists.llvm.org> writes: