[clang-tidy] Fix for cppcoreguidelines-pro-type-union-access if memLoc is invalid #104540

ksromanov · 2024-08-16T02:23:30Z

[cppcoreguidelines-pro-type-union-access] clang-tidy check issues a warning, which location points to the union field access using getMemberLoc(). However, sometimes getMemberLoc() returns invalid location, for instance when the accessed union member is not a data field, but a function.

The main problem is that the check issues warning without location information is only confusing people, when the checked file contains lots of includes. This situation often occurs in practice.

This PR addresses this situation, returning the original expression location if member location is not valid.

Fixes #102945

github-actions · 2024-08-16T02:23:47Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-08-16T02:24:15Z

@llvm/pr-subscribers-clang-tools-extra

Author: Konstantin Romanov (ksromanov)

Changes

Fixes #102945

Full diff: https://github.com/llvm/llvm-project/pull/104540.diff

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp (+6-2)
(modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp (+7)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp
index 1ed444e630ec25..0e9185956b7aa8 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp
@@ -23,8 +23,12 @@ void ProTypeUnionAccessCheck::registerMatchers(MatchFinder *Finder) {
 
 void ProTypeUnionAccessCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Matched = Result.Nodes.getNodeAs<MemberExpr>("expr");
-  diag(Matched->getMemberLoc(),
-       "do not access members of unions; use (boost::)variant instead");
+  if (auto MemberLoc = Matched->getMemberLoc(); MemberLoc.isValid())
+    diag(MemberLoc,
+         "do not access members of unions; use (boost::)variant instead");
+  else
+    diag(Matched->getBeginLoc(),
+         "do not access members of unions; use (boost::)variant instead");
 }
 
 } // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp
index 6abc22b9e4345e..46bb06ba2c8fbe 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp
@@ -5,6 +5,10 @@ union U {
   char union_member2;
 } u;
 
+union W {
+  template <class TP> operator TP *() const;
+};
+
 struct S {
   int non_union_member;
   union {
@@ -20,6 +24,7 @@ void f(char);
 void f2(U);
 void f3(U&);
 void f4(U*);
+W f5();
 
 void check()
 {
@@ -38,4 +43,6 @@ void check()
   f2(u); // OK
   f3(u); // OK
   f4(&u); // OK
+  void *ret = f5();
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: do not access members of unions; use (boost::)variant instead
 }

llvmbot · 2024-08-16T02:24:15Z

@llvm/pr-subscribers-clang-tidy

Author: Konstantin Romanov (ksromanov)

Changes

Fixes #102945

Full diff: https://github.com/llvm/llvm-project/pull/104540.diff

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp (+6-2)
(modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp (+7)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp
index 1ed444e630ec25..0e9185956b7aa8 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp
@@ -23,8 +23,12 @@ void ProTypeUnionAccessCheck::registerMatchers(MatchFinder *Finder) {
 
 void ProTypeUnionAccessCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Matched = Result.Nodes.getNodeAs<MemberExpr>("expr");
-  diag(Matched->getMemberLoc(),
-       "do not access members of unions; use (boost::)variant instead");
+  if (auto MemberLoc = Matched->getMemberLoc(); MemberLoc.isValid())
+    diag(MemberLoc,
+         "do not access members of unions; use (boost::)variant instead");
+  else
+    diag(Matched->getBeginLoc(),
+         "do not access members of unions; use (boost::)variant instead");
 }
 
 } // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp
index 6abc22b9e4345e..46bb06ba2c8fbe 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-union-access.cpp
@@ -5,6 +5,10 @@ union U {
   char union_member2;
 } u;
 
+union W {
+  template <class TP> operator TP *() const;
+};
+
 struct S {
   int non_union_member;
   union {
@@ -20,6 +24,7 @@ void f(char);
 void f2(U);
 void f3(U&);
 void f4(U*);
+W f5();
 
 void check()
 {
@@ -38,4 +43,6 @@ void check()
   f2(u); // OK
   f3(u); // OK
   f4(&u); // OK
+  void *ret = f5();
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: do not access members of unions; use (boost::)variant instead
 }

PiotrZSL

Missing release notes entry, about what's fixed in check.
For me this is an full-fix, not an workaround.

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp

5chmidti

This is still missing a release note, see

llvm-project/clang-tools-extra/docs/ReleaseNotes.rst

Line 104 in 40975da

Changes in existing checks

for where to add it and how they look like

Functionality-wise, this looks good

ksromanov · 2024-08-24T15:09:14Z

This is still missing a release note, see

Many thanks! This is my first PR to llvm, I didn't know about the file ReleaseNotes in clang-tools-extra, sorry.

…tion is invalid.

…essCheck.cpp Co-authored-by: Piotr Zegar <me@piotrzegar.pl>

clang-tools-extra/docs/ReleaseNotes.rst

ksromanov · 2024-09-16T14:17:15Z

@PiotrZSL would you please review once again?

ksromanov · 2024-10-21T15:45:28Z

All of the issues addressed long time ago.

nicovank

LGTM! Minus alphabetical ordering in release notes. I'll merge this tomorrow to give time to others just in case.

FWIW this issue actually triggers a crash/assertion failure when those are enabled:

clang-tidy: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:178: DiagnosticBuilder clang::tidy::ClangTidyContext::diag(StringRef, SourceLocation, StringRef, DiagnosticIDs::Level): Assertion `Loc.isValid()' failed.
[...]

clang-tools-extra/docs/ReleaseNotes.rst

nicovank · 2024-10-22T01:58:23Z

There's a CI failure but looks unrelated. If you have a minute please rebase/merge main to trigger the checks again and hopefully the problem disappears. We can also just keep an eye on the buildbots post-merge tomorrow.

EDIT: Turns out I can just do it, never knew I could press that button on the PR. Anyway should re-trigger things :).

ksromanov · 2024-10-22T03:27:06Z

EDIT: Turns out I can just do it, never knew I could press that button on the PR. Anyway should re-trigger things :).

Thank you!

github-actions · 2024-10-23T00:05:21Z

@ksromanov Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

ksromanov · 2024-10-23T02:33:24Z

Thanks a lot!

llvmbot added clang-tools-extra clang-tidy labels Aug 16, 2024

EugeneZelenko requested review from PiotrZSL, njames93, SimplyDanny and 5chmidti August 16, 2024 14:47

PiotrZSL requested changes Aug 16, 2024

View reviewed changes

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAccessCheck.cpp Outdated Show resolved Hide resolved

ksromanov force-pushed the fix-ProTypeUnionAccessCheck branch from 509a17d to 8f07de0 Compare August 16, 2024 18:04

ksromanov changed the title ~~[clang-tidy] Workaround for cppcoreguidelines-pro-type-union-access if memLoc is invalid~~ [clang-tidy] Fix for cppcoreguidelines-pro-type-union-access if memLoc is invalid Aug 16, 2024

ksromanov requested a review from PiotrZSL August 19, 2024 02:03

5chmidti requested changes Aug 24, 2024

View reviewed changes

ksromanov and others added 5 commits August 24, 2024 11:10

Workaround for cppcoreguidelines-pro-type-union-access if member loca…

31ed1cf

…tion is invalid.

Update clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeUnionAcc…

f271361

…essCheck.cpp Co-authored-by: Piotr Zegar <me@piotrzegar.pl>

Address comments.

87f1f4c

Update test.

c76eda3

Add note to ReleaseNotes.rst.

ed70da9

ksromanov force-pushed the fix-ProTypeUnionAccessCheck branch from aee44f9 to ed70da9 Compare August 24, 2024 15:14

5chmidti approved these changes Aug 24, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

ksromanov added 2 commits August 24, 2024 12:14

Add articles.

89360e5

Merge branch 'main' into fix-ProTypeUnionAccessCheck

443db74

Merge branch 'main' into fix-ProTypeUnionAccessCheck

bc2ed4f

nicovank approved these changes Oct 21, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

Put note in ReleaseNotes according to alphabetical order.

e0f3bae

Merge branch 'main' into fix-ProTypeUnionAccessCheck

568b002

nicovank merged commit 0fbf91a into llvm:main Oct 23, 2024
9 checks passed

ksromanov deleted the fix-ProTypeUnionAccessCheck branch October 23, 2024 02:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-tidy] Fix for cppcoreguidelines-pro-type-union-access if memLoc is invalid #104540

[clang-tidy] Fix for cppcoreguidelines-pro-type-union-access if memLoc is invalid #104540

ksromanov commented Aug 16, 2024 •

edited

Loading

github-actions bot commented Aug 16, 2024

llvmbot commented Aug 16, 2024

llvmbot commented Aug 16, 2024

PiotrZSL left a comment

5chmidti left a comment

ksromanov commented Aug 24, 2024 •

edited

Loading

ksromanov commented Sep 16, 2024 •

edited

Loading

ksromanov commented Oct 21, 2024

nicovank left a comment

nicovank commented Oct 22, 2024 •

edited

Loading

ksromanov commented Oct 22, 2024

github-actions bot commented Oct 23, 2024

ksromanov commented Oct 23, 2024

[clang-tidy] Fix for cppcoreguidelines-pro-type-union-access if memLoc is invalid #104540

[clang-tidy] Fix for cppcoreguidelines-pro-type-union-access if memLoc is invalid #104540

Conversation

ksromanov commented Aug 16, 2024 • edited Loading

github-actions bot commented Aug 16, 2024

llvmbot commented Aug 16, 2024

llvmbot commented Aug 16, 2024

PiotrZSL left a comment

Choose a reason for hiding this comment

5chmidti left a comment

Choose a reason for hiding this comment

ksromanov commented Aug 24, 2024 • edited Loading

ksromanov commented Sep 16, 2024 • edited Loading

ksromanov commented Oct 21, 2024

nicovank left a comment

Choose a reason for hiding this comment

nicovank commented Oct 22, 2024 • edited Loading

ksromanov commented Oct 22, 2024

github-actions bot commented Oct 23, 2024

ksromanov commented Oct 23, 2024

ksromanov commented Aug 16, 2024 •

edited

Loading

ksromanov commented Aug 24, 2024 •

edited

Loading

ksromanov commented Sep 16, 2024 •

edited

Loading

nicovank commented Oct 22, 2024 •

edited

Loading