-
Notifications
You must be signed in to change notification settings - Fork 10.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] Create check for inconsistent/missed typedef/using alias type names #86714
Comments
@llvm/issue-subscribers-clang-tidy Author: None (FruitClover)
Consider the following example: https://godbolt.org/z/jE8Yc7sz6
```c++
using Type = int;
struct Base { struct Derived : Base {
| | Base Type | Derived Type | Warning | FixItHint |
This probably belongs to Background: PoC version was prepared for internal projects, here I want to check if clang-tidy would be interested in it before publishing PR, and discuss what to warn and if we should emit more fix hints. |
Example diagnostics with poc: llvm-project/clang-tools-extra/clangd/index/Index.cpp:75:1: warning: Inconsistent alias name for 'clang::clangd::SwapIndex::indexedFiles' return type: Base return type is alias, but derived return type is not; Consider replacing with base type alias [bugprone-inconsistent-alias-names]
75 | llvm::unique_function<IndexContents(llvm::StringRef) const>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| clang::clangd::SymbolIndex::IndexedFiles
llvm-project/clang-tools-extra/clangd/index/Index.h:156:11: note: Base return type is here
156 | virtual IndexedFiles indexedFiles() const = 0;
| ^~~~~~~~~~~~
llvm-project/clang-tools-extra/clangd/index/Index.h:154:9: note: Alias for base return type: 'clang::clangd::SymbolIndex::IndexedFiles'
154 | using IndexedFiles =
| ~~~~~~^~~~~~~~~~~~~~
155 | llvm::unique_function<IndexContents(llvm::StringRef) const>;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorLevel.cpp:73:3: warning: Inconsistent alias name for '(anonymous namespace)::DenseLevel::peekRangeAt' return type: Derived return type is alias, but base return type is not [bugprone-inconsistent-alias-names]
73 | ValuePair peekRangeAt(OpBuilder &b, Location l, Value p,
| ^~~~~~~~~
llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorLevel.h:50:11: note: Base return type is here
50 | virtual std::pair<Value, Value> peekRangeAt(OpBuilder &b, Location l, Value p,
| ^~~~~~~~~~~~~~~~~~~~~~~
llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/Utils/SparseTensorLevel.cpp:18:7: note: Alias for derived return type: 'ValuePair'
18 | using ValuePair = std::pair<Value, Value>;
| ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
bump |
I think a check like this would be nice. Your table of what you suggest to be diagnosing looks good as well. Only fixing the one case is the right way to go, in the other cases it may not be correct or good to fix the declarations (from a semantic point of view in their code-base, as well as the issue with where the alias is defined and if it is even available at the base class). I think that a chain alias
should basically be the same as
: just another level of sugar. There shouldn't be a difference in how to diagnose using
and using The // header:
int foo();
using I = int;
// impl
I foo() {} The name sounds good to me. I thought of |
Thanks for the feedback @5chmidti !
Right, I should have mentioned this explicitly
Agree
Yes, I intend to extend this check for the forward declarations (both return type and arguments), but not in the initial PR, so reflecting this in checker name would be great. |
Does it mean that the methods with the same name will be suggested to have the same parameters and return types in Base and Derived? The Derived just overrides the Base without any errors. |
Right, there are no compiler errors (underlying types are the same), but we could have different meaning/semantic due to different alias names, for example: using MemoryType = int;
using EdgeType = int;
struct B { virtual MemoryType foo(); };
struct D : B { EdgeType foo() override; }; which could lead to false assumptions for the users of the derived class.
That's the tricky part, so currently there is only 1 replacement suggestion (out of 4 cases), where the rest are "please check if this is intentional" type warnings (which we probably could control with options and sane defaults). |
From my perspective, warnings don't work well here. |
Sorry if this is going off track. As C/C++ do not have "strong typedef" I have been looking forward to such a check as it could be used to enforce type safety across compatible POD types. The case here is just about overloads with wrong aliases but this could be spun further to all usages. There is also |
You might be right that warnings for all cases don't work well here, but at least some could be useful, specifically:
with the intention to warn users that changes in alias names in the And we should take care of user-defined types, e.g., for the following example from /usr/include/c++/v1/locale:2668:18: warning: Inconsistent alias name for 'std::moneypunct_byname<char>::do_neg_format' return type [bugprone-inconsistent-use-of-alias-in-declaration]
/usr/include/c++/v1/locale:2668:18: warning: derived type is alias, but base type is not
2668 | pattern do_neg_format() const override {return __neg_format_;}
| ~~~~~~~ ^
/usr/include/c++/v1/locale:2619:25: note: Base return type is here
2619 | virtual pattern do_neg_format() const
| ~~~~~~~ ^
/usr/include/c++/v1/locale:2645:34: note: Alias for derived return type: 'std::moneypunct_byname::pattern'
2645 | typedef money_base::pattern pattern;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
Sorry, I don't quite get |
Yes, that's one of the initial reasons for such check, and, as @5chmidti noticed, with the intention to extend into forward declarations. |
Consider the following example: https://godbolt.org/z/jE8Yc7sz6
Here we misuse/miss consistent alias names in derived methods (compared to base
class signatures), which could lead to tricky bugs (after
using Type = ...
changes) and this also complicates plain text search.
The idea is that we could detect such
using
/typedef
inconsistency insome cases with clang-tidy.
To start up, we could focus on:
To illustrate which cases are compatible and which are not - check the following table:
legend
Base Type
: return/argument type for base class methods.Derived Type
: return/argument type for derived class methods.Warning/FixItHint
: if we should emit warning/hint(we use
int
as some non-aliased type, and<alias>
asusing T = ..
ortypedef .. T
;(*)
Both types are aliases: AnalyzeBaseT
vsDerivedT
:Alias names are compatible: (no warning)
1.1 One of (
BaseT
,DerivedT
) is implicit2.1 Same fully-qualified names:
BaseT.getQualifiedNameAsString == DerivedT.getQualifiedNameAsString
Alias names are non-compatible: (warning)
2.1. Strict check:
TD1.getQualifiedNameAsString != TD2.getQualifiedNameAsString
(N1::Type vs. N2::Type
)Questinable: (Warning: ?)
3.1. Chain alias (maybe control with an option). If so, should check it before 2.1
This probably belongs to
bugprone
category, something likeinconsistent-alias-names
/inconsistent-using-typedef
(suggestions are welcome)Background: PoC version was prepared for internal projects, here I want to check if clang-tidy would be interested in it before publishing PR, and discuss what to warn and if we should emit more fix hints.
The text was updated successfully, but these errors were encountered: