Page MenuHomePhabricator

[clang-tidy] new cppcoreguidelines-narrowing-conversions check.
ClosedPublic

Authored by courbet on Oct 2 2017, 5:48 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
alexfh edited edge metadata.Oct 2 2017, 7:28 AM

Before going deeper, I'd like to understand what's the difference between this check and -Wconversion group of clang warnings?

This check is a more useful subset of -Wfloat-conversion that targets cases that are more likely to be bugs than the rest of -Wfloat-conversion.

Since "more useful" is a somewhat subjective notion, let me point out a typical class of bugs that this uncovers:

// Paying 0.5 less dollars per day.
int to_be_payed = 0;
for (const auto& v : us_dollars_per_day) {
  to_be_payed += v[i];
}

This is not necessarily more dangerous than int my_int = my_float;, but the effects will add up dramatically.

Why can't this be done in clang ?

It could, but we would need to add more fine-grained control of what to enable or not enable. It feels more inline with clang-tidy's options approach, but I don;t have strong feelings on this.

JonasToth set the repository for this revision to rL LLVM.
JonasToth added a project: Restricted Project.
JonasToth added a subscriber: JonasToth.

One more reason for this check being in clang-tidy would be automatically inserting narrow_cast<T>(v), which is the utility of the gsl to make narrowing casts explicitly visible.
Doing might be the result of future work.

clang-tidy implements bugprone-integer-division already, which does a somewhat related thing:
"Finds cases where integer division in a floating point context is likely to cause unintended loss of precision." - problematic narrowing conversion.

One more reason for this check being in clang-tidy would be automatically inserting narrow_cast<T>(v), which is the utility of the gsl to make narrowing casts explicitly visible.
Doing might be the result of future work.

Do we already have other cppcoreguidelines checks that provides fixes with symbols from the the gsl ?

Do we already have other cppcoreguidelines checks that provides fixes with symbols from the the gsl ?

AFAIK not yet. I am working on fixits for gsl::owner<> and the bounds-* stuff could suggest gsl::at() as well.

I think you should add std::floor and std::ceil recognition here. This would be inline with hicpp, too.

Now I finally got around to this.

Since the "Enforcement" part of the relevant C++ Core Guidelines rule is not very helpful (kind of a "hey, we know enforcing this rule will lead to tons of false positive, so we don't know what exactly makes sense to enforce"), this may well be a good first step. I personally have no concerns. Maybe someone more interested in this module will have something to say though.

Could you rebase the patch against current head?

I think you should add std::floor and std::ceil recognition here.

+1

alexfh requested changes to this revision.Mar 14 2018, 8:09 AM
This revision now requires changes to proceed.Mar 14 2018, 8:09 AM
courbet updated this revision to Diff 138910.Mar 19 2018, 7:16 AM

Do not trigger on some_int += std::floor(some_float);

courbet edited the summary of this revision. (Show Details)Mar 19 2018, 7:16 AM
aaron.ballman added inline comments.Mar 20 2018, 9:28 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
26 ↗(On Diff #138910)

Why only these two operators? This does not match the behavior from the C++ Core Guideline check itself (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-narrowing).

courbet added inline comments.Mar 20 2018, 9:36 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
26 ↗(On Diff #138910)

These provided the best signal to noise ratio. Also they are the most dangerous (in a loop, you might end up losing one unit per iteration). I'll add other operators later if that's fine with you.

aaron.ballman added inline comments.Mar 20 2018, 9:41 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
26 ↗(On Diff #138910)

If the check doesn't cover anything listed in the C++ Core Guideline examples or enforcement wording, I have a hard time accepting it as the initial commit for such a check.

Of special interest to me is the request from the C++ Core Guideline authors themselves:

- flag all floating-point to integer conversions (maybe only float->char and double->int. Here be dragons! we need data)
- flag all long->char (I suspect int->char is very common. Here be dragons! we need data)

I think we need data as well -- writing the check and then testing it over several million lines of source could give us some of that data, which we can then feed back to the guideline authors, so that we come up with a check that's realistic.

courbet added inline comments.Mar 21 2018, 2:30 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
26 ↗(On Diff #138910)

Quoting the guideline:
"A good analyzer can detect all narrowing conversions. However, flagging all narrowing conversions will lead to a lot of false positives." Then follows a list of suggestions. While +=/-= are not in the list of suggestions, they are a subset of "flag all floating-point to integer conversions" that I've found to be useful (very low rate of false positives) by running on our codebase.
I agree that more data would be useful, so I'll do an analysis of flagging all (non-ceil/floor float/double)->integral conversions.

alexfh requested changes to this revision.Mar 26 2018, 8:49 AM

I agree that more data would be useful, so I'll do an analysis of flagging all (non-ceil/floor float/double)->integral conversions.

Removing from my dashboard for now.

This revision now requires changes to proceed.Mar 26 2018, 8:49 AM

OK, here's an analysis of 100 instances of a check that would warn on all fully implicit (no C/static/reinterpret/...) casts from float/double to integral: link
I don't expect the analysis to be perfect, but that gives us a better idea of what to expect.

  • A bit more than 1/3 of instances are OK and should be using a cast.
  • A bit less than 1/4 of instances is brittle code and should use int types.
  • The two false positive cases are easy to handle.
  • I was surprised about the "truth value of float" one. I think we should provide a fix to turn !f this into f != 0.0f.
courbet requested review of this revision.Mar 29 2018, 6:06 AM

Hi,

my 2 cents:

  • On which codebases did you run the check?
  • did you consider looking for implicitCastExpr? You can capture all narrowing conversion with that and analyze them further. I think it is possible to warn for the subset mentioned in the guidelines.
  • you match for binaryOperator("+=", "-") maybe all assignments can be looked at? (binaryOperator(isASsignmentOperator()), defined in clang-tidy/util/Matchers.h or similar) That includes all calculate-and-assign operations. Those should be equally dangerous.

Hi Jonas,

Hi,

my 2 cents:

  • On which codebases did you run the check?

A large repository of open-source code, plus internal code at google. External code includes e.g. code from ffmpeg, Eigen, R, Chromium, gnuplot, lua ,...

  • did you consider looking for implicitCastExpr? You can capture all narrowing conversion with that and analyze them further. I think it is possible to warn for the subset mentioned in the guidelines.

Yes, that's the version for which I have provided analysis. I'll update the diff with that version.

  • you match for binaryOperator("+=", "-") maybe all assignments can be looked at? (binaryOperator(isASsignmentOperator()), defined in clang-tidy/util/Matchers.h or similar) That includes all calculate-and-assign operations. Those should be equally dangerous.

The "normal" assignments are covered by the implicitCastExpr() above.

That sounds good.

Removing from my dashboard for now.

@aaron.ballman seems to be busy now, maybe you should add alexfh again and discuss the results.

courbet updated this revision to Diff 141610.Apr 9 2018, 3:51 AM
courbet edited the summary of this revision. (Show Details)
  • Add support for bad cast detection.

That sounds good.

Removing from my dashboard for now.

maybe you should add alexfh again and discuss the results.

Is there anything I need to do for the diff to change state ? I thought updating the code would automatically mark it "ready for review" again.

Is there anything I need to do for the diff to change state ? I

thought updating the code would automatically mark it "ready for review"
again.

I think all comments must be marked as done to be ready for review
again. Usually the reviewer reacts to changed code, too.

But aaron seems busy right now and i think alexfh did disable the
notifications for now. Add/ping him again.

Am 09.04.2018 um 12:55 schrieb Clement Courbet via Phabricator:

courbet added a comment.

In https://reviews.llvm.org/D38455#1061300, @JonasToth wrote:

That sounds good.

Removing from my dashboard for now.

maybe you should add alexfh again and discuss the results.

Is there anything I need to do for the diff to change state ? I thought updating the code would automatically mark it "ready for review" again.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

I think all comments must be marked as done to be ready for review
again.

I think alexfh did disable the notifications for now. Add/ping him again.

I see, thanks.

courbet removed a reviewer: alexfh.Apr 9 2018, 4:05 AM
courbet added a reviewer: alexfh.
JonasToth added inline comments.Apr 9 2018, 4:13 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
32 ↗(On Diff #141610)

Do you plan to check for double -> float conversion, too?

Having a limited scope for now is ok, but a FIXME would be nice.

34 ↗(On Diff #141610)

Given C++ is weird sometimes: Is there a way that a cast might imply an narrowing conversion?

double value = 0.4;
int i = static_cast<float>(value);

or

void some_function(int);

double d = 0.42;
some_function(static_cast<float>(d));

would come to mind.

Nice to have, IMHO not necessary.

41 ↗(On Diff #141610)

I would really like to add all other calc-and-assign operations. At least "*=" and "/=".

courbet marked 2 inline comments as done.Apr 9 2018, 6:36 AM

Thanks.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
32 ↗(On Diff #141610)

I've added a FIXME.

34 ↗(On Diff #141610)

Luckily that does not seem to be implicit:

void f(double value) {
  int i = static_cast<float>(value);
}
FunctionDecl 0x7fe5ffbd4150 <experimental/users/courbet/benchmark/toto.cc:1:1, line:3:1> line:1:6 f 'void (double)'
|-ParmVarDecl 0x7fe5ffbd4088 <col:8, col:15> col:15 used value 'double'
`-CompoundStmt 0x7fe5ffbd4380 <col:22, line:3:1>
  `-DeclStmt 0x7fe5ffbd4368 <line:2:3, col:36>
    `-VarDecl 0x7fe5ffbd4250 <col:3, col:35> col:7 i 'int' cinit
      `-ImplicitCastExpr 0x7fe5ffbd4350 <col:11, col:35> 'int' <FloatingToIntegral>
        `-CXXStaticCastExpr 0x7fe5ffbd4320 <col:11, col:35> 'float' static_cast<float> <NoOp>
          `-ImplicitCastExpr 0x7fe5ffbd4308 <col:30> 'float' <FloatingCast>
            `-ImplicitCastExpr 0x7fe5ffbd42f0 <col:30> 'double' <LValueToRValue>
              `-DeclRefExpr 0x7fe5ffbd42b0 <col:30> 'double' lvalue ParmVar 0x7fe5ffbd4088 'value' 'double'
41 ↗(On Diff #141610)

Makes sense, I've added *= and /=. All others (%=, &=, <<= and variations thereof) trigger a compilation error if the RHS is a float (rightfully).

courbet updated this revision to Diff 141635.Apr 9 2018, 6:36 AM
courbet marked 2 inline comments as done.
  • Add *= and /=.
JonasToth added inline comments.Apr 9 2018, 6:59 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
34 ↗(On Diff #141610)

The interesting one should get diagnosed. Could you add a test?

41 ↗(On Diff #141610)

For floats that is true.

If we care about the ìnt->char casts later, they should be added.

Could you please add some tests that include user defined literals and there interaction with other literals. We should catch narrowing conversions from them, too.

alexfh added a comment.Apr 9 2018, 7:48 AM

ping

Sorry for the long review due to the holidays.

Generally, I would also like Aaron to take a look when he's back, since he had some concerns. While we're waiting, one minor comment from me.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
41 ↗(On Diff #141610)

Does it make sense to match all assignment operators including just =? If so, the matcher will become a bit simpler: binaryOperator(isAssignmentOperator(), ...). If there's a reason not to do this, maybe adding a comment would be useful

Apart from that, I wonder whether the matcher above would also cover the case of assignment operators? I would expect the AST for assignment expressions with different types to have ImplicitCast nodes anyway.

courbet added inline comments.Apr 9 2018, 7:53 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
41 ↗(On Diff #141610)

If we care about the ìnt->char casts later, they should be added.

Will do. I'd like to start by implementing only floats first though, because they are the ones where I have data.

Apart from that, I wonder whether the matcher above would also cover the case of assignment operators?

It does cover = (which generates an implicit cast), but not +=.

This seems like it would be nice to have in bugprone-* as well.

Could you please add some tests that include user defined literals and there interaction with other literals. We should catch narrowing conversions from them, too.

User defined literals do not have this issue: the only accepted signatures are with long double or unsigned long long parameters. Narrowing cannot happen because XYZ_ud actually means operator""(XYZull) (same for floats).
(see http://en.cppreference.com/w/cpp/language/user_literal).

I've added a test with a narrowing on the return value.

courbet updated this revision to Diff 141800.Apr 10 2018, 12:12 AM
  • Simplify matcher expression
  • Add other binary operators
  • Add more tests.

This seems like it would be nice to have in bugprone-* as well.

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

Take a look in the hicpp module, almost everything there is aliased :)

courbet updated this revision to Diff 141801.Apr 10 2018, 12:27 AM
  • Add NarrowingConversionsCheck to bugprone module.

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

Take a look in the hicpp module, almost everything there is aliased :)

Thanks for the pointer !

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

Take a look in the hicpp module, almost everything there is aliased :)

Thanks for the pointer !

We currently don't have any strict rules about aliases, but we should remember that aliases have certain costs (in terms of maintenance and possible user-facing issues like duplicate warnings from different checks). I think that in general it would be better to avoid excessive aliases. There are cases when aliases are inevitable (or at least feel like a proper solution), like when multiple style guides have the same rule or very similar rules that make sense to be implemented in a single check with a bit of configurability to easily accommodate the differences. But in this case there's so far one specific rule of the C++ Core Guidelines that the check is enforcing, and adding an alias under bugprone- for it seems to be unnecessary. I also think that if a user wants to enable this check, they'd better know that it backs up a rule of the C++ Core Guidelines rather than being just an invention of clang-tidy contributors (there's nothing wrong with the latter, it's just nice to give credits where appropriate ;).

I suggest to try applying the following decision process to find proper place(s) for a check:

  1. List all use cases we want to support, decide whether customizations (via check options) will be needed to support them. Use case may be
    • enforcement of a rule of a certain style guide a clang-tidy module for which exists or is being added together with the check;
    • generic use case not covered by a rule of a style guide. These only count if there is no "style guide" use case that doesn't require customizations.
  1. If the check only has a single "style guide" use case, place it to the corresponding module.
  2. If a check (without customizations) implements rules of two or more style guides, and dependency structure allows, place it to one of the corresponding modules and add aliases to the others.
  3. Otherwise choose a "generic" module for the check (bugprone-, readability-, modernize-, performance-, portability-, or misc-, if the check doesn't suit a more specific module). Add aliases to the "style guide" modules, if needed.
aaron.ballman added inline comments.May 18 2018, 6:15 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
35 ↗(On Diff #141801)

I believe this code will not diagnose under this check -- is that intended as a way to silence the check?

i += (double)0.5;
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
10–11 ↗(On Diff #141801)

More exposition is needed because this is only a very small part of the core guideline. You should be explicit about where we deviate (or what we support, depending on which is the clearest exposition).

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
81 ↗(On Diff #141801)

What should happen in cases like the following:

template <typename T1, typename T2>
void f(T1 one, T2 two) {
  one += two;
}

void g() {
  f(1, 2);
  f(1, .5);
}

#define DERP(i, j) (i += j)

void h() {
  int i = 0;
  DERP(1, 2);
  DERP(i, .5);
}
courbet updated this revision to Diff 147508.May 18 2018, 7:40 AM
courbet marked 2 inline comments as done.
  • More explicit documentation.
  • Do not trigger in template and macro contexts.
  • More tests.

Thanks.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
35 ↗(On Diff #141801)

Did you mean (int)0.5 ?

Yes, the user essentially told us they knew what they were doing. I've added an explicit test for this.

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
81 ↗(On Diff #141801)

I added more tests.

aaron.ballman added inline comments.May 18 2018, 7:44 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
35 ↗(On Diff #141801)

I truly meant (double)0.5 -- where the cast has no impact on the narrowing conversion, but the check still doesn't diagnose because there's an explicit cast present. Should the check be checking for explicit casts to the narrowed type?

courbet added inline comments.May 18 2018, 8:08 AM
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
35 ↗(On Diff #141801)

OK, then that's fine (I added a test for that for): there is an explicit cast to double, but then the compiler generates an extra cast to int on top, which triggers.

aaron.ballman accepted this revision.May 18 2018, 8:11 AM

I think this generally LGTM. You should wait a bit to see if @alexfh has any other concerns.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
35 ↗(On Diff #141801)

Ah, excellent, thank you!

This revision is now accepted and ready to land.May 18 2018, 8:11 AM

Great, thanks for the review !

This revision was automatically updated to reflect the committed changes.