Page MenuHomePhabricator

[clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index
AbandonedPublic

Authored by carlosgalvezp on Jan 13 2022, 2:19 AM.

Details

Summary

Currently the fix hint is hardcoded to gsl::at(). This poses
a problem for people who, for a number of reasons, don't want
or cannot use the GSL library.

In these situations, the fix hint does more harm than good
as it creates confusion as to what the fix should be. People
can even misinterpret the fix "gsl::at" as e.g. "std::array::at",
which can lead to even more trouble (e.g. when having guidelines
that disallow exceptions).

Therefore, introduce a new option FixHint that allows users
to specify exactly what the fix hint should be for their
use case. The default is kept to gsl::at.

Since the users can write anything in this option, clang-tidy
should not add fix-it hints in this case, as they could
lead to invalid code. For example, a reasonable fix hint
like "MyCustomSafeArrayWrapper::at()" would not be possible
to replace directly in code, yet it would be of great help
pointing to users in the right direction.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Jan 13 2022, 2:19 AM
carlosgalvezp requested review of this revision.Jan 13 2022, 2:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 2:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Update release notes.

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
156

Please sort checks in this section alphabetically.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
30

Please use single back-ticks for gsl::at() (option value).

carlosgalvezp marked an inline comment as done.

Single backticks for gsl::at

clang-tools-extra/docs/ReleaseNotes.rst
156

Hmm I'm not sure what you mean, should these bullets be ordered alphabetically? If so, why is "Fixed" showing up after "Updated"?

Eugene.Zelenko added inline comments.Jan 13 2022, 7:37 AM
clang-tools-extra/docs/ReleaseNotes.rst
156

Now cppcoreguidelines-pro-bounds-constant-array-index is before cppcoreguidelines-explicit-virtual-functions.

Fix check order

carlosgalvezp marked an inline comment as done.Jan 13 2022, 7:39 AM
carlosgalvezp added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
156

Ah, of course. Fixed!

carlosgalvezp marked 2 inline comments as done.Jan 14 2022, 2:00 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
26

Should elide the braces here for the option and insert them at the warning stage.

79–82
84

This restriction on only offering a fix if using gsl::at seem a little too restrictive, especially when you factor that the GslHeader is defaulted to an empty string and that is already required for a fix to be emitted.

carlosgalvezp added inline comments.Jan 16 2022, 12:14 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
84

What would this change for the user? Current behavior is kept, and we can't really predict a good fix if the hint is user-provided. There's many other ways to perform safe indexing than using a free standing function gsl::at-like, and those will require a larger, non-automated refactor.

Another alternative I had in mind is to completely remove the "use gsl::at() instead" hint from the message, and simply warn about the problem (unsafe indexing). CppCoreGuidelines don't actually mention that you need to use gsl::at, at least not from what I can see in the link provided in the docs.

Removing the fix hint from the warning would solve my concerns (confusing information), and we don't need to introduce extra configuration options.

What do you think?

I believe this other solution has lower complexity and still achieves my goal, let me know which one you prefer!
https://reviews.llvm.org/D117857