Merge pull request #365 from github/lcartey/a7-3-1-improvements

`A7-3-1`: Improve performance, reduce false positives

Author: rvermeulen

change_notes/2023-09-28-a7-3-1-updates.md

@@ -0,0 +1,7 @@
+ * `A7-3-1` - `HiddenInheritedNonOverridableMemberFunction.ql`:
+   - Reduce duplication by reporting only a single location for each declaration of a problematic element.
+   - Reduce duplication when reporting the hidden function by reporting only one declaration entry.
+   - Improve performance by eliminating a number of bad join orders.
+   - Fix false positives where the using declaration occurred after the function declaration.
+   - Exclude special member functions, which cannot be inherited.
+   - Exclude private member functions, which cannot be inherited.

cpp/autosar/src/rules/A7-3-1/HiddenInheritedNonOverridableMemberFunction.ql

@@ -15,24 +15,45 @@
 
 import cpp
 import codingstandards.cpp.autosar
+import codingstandards.cpp.Class
 
-from FunctionDeclarationEntry overridingDecl, FunctionDeclarationEntry hiddenDecl
+/**
+ * Holds if the class has a non-virtual member function with the given name.
+ */
+pragma[noinline, nomagic]
+predicate hasNonVirtualMemberFunction(Class clazz, MemberFunction mf, string name) {
+  mf.getDeclaringType() = clazz and
+  mf.getName() = name and
+  not mf.isVirtual() and
+  // Exclude private member functions, which cannot be inherited.
+  not mf.isPrivate()
+}
+
+/**
+ * Holds if the member function is in a class with the given base class, and has the given name.
+ */
+pragma[noinline, nomagic]
+predicate hasDeclarationBaseClass(MemberFunction mf, Class baseClass, string functionName) {
+  baseClass = mf.getDeclaringType().getABaseClass() and
+  functionName = mf.getName()
+}
+
+from MemberFunction overridingDecl, MemberFunction hiddenDecl, Class baseClass, string name
 where
   not isExcluded(overridingDecl, ScopePackage::hiddenInheritedNonOverridableMemberFunctionQuery()) and
   // Check if we are overriding a non-virtual inherited member function
-  overridingDecl.getName() = hiddenDecl.getName() and
-  overridingDecl.getDeclaration().getDeclaringType().getABaseClass() =
-    hiddenDecl.getDeclaration().getDeclaringType() and
-  not hiddenDecl.getDeclaration().isVirtual() and
+  hasNonVirtualMemberFunction(baseClass, hiddenDecl, name) and
+  hasDeclarationBaseClass(overridingDecl, baseClass, name) and
   // Where the hidden member function isn't explicitly brought in scope through a using declaration.
   not exists(UsingDeclarationEntry ude |
-    ude.getDeclaration() = hiddenDecl.getDeclaration() and
-    ude.getEnclosingElement() = overridingDecl.getDeclaration().getDeclaringType() and
-    ude.getLocation().getStartLine() < overridingDecl.getLocation().getStartLine()
+    ude.getDeclaration() = hiddenDecl and
+    ude.getEnclosingElement() = overridingDecl.getDeclaringType()
   ) and
   // Exclude compiler generated member functions which include things like copy constructor that hide base class
   // copy constructors.
-  not overridingDecl.getDeclaration().isCompilerGenerated()
+  not overridingDecl.isCompilerGenerated() and
+  // Exclude special member functions, which cannot be inherited.
+  not overridingDecl instanceof SpecialMemberFunction
 select overridingDecl,
-  "Declaration for member '" + overridingDecl.getName() +
-    "' hides non-overridable inherited member function $@", hiddenDecl, hiddenDecl.getName()
+  "Declaration for member '" + name + "' hides non-overridable inherited member function $@",
+  hiddenDecl, hiddenDecl.getName()

cpp/autosar/src/rules/A7-3-1/HiddenInheritedOverridableMemberFunction.ql

@@ -21,6 +21,8 @@ where
   not isExcluded(overridingDecl, ScopePackage::hiddenInheritedOverridableMemberFunctionQuery()) and
   // Check if we are overriding a virtual inherited member function
   hiddenDecl.getDeclaration().isVirtual() and
+  // Exclude private member functions, which cannot be inherited.
+  not hiddenDecl.getDeclaration().(MemberFunction).isPrivate() and
   // The overriding declaration hides the hidden declaration if:
   (
     // 1. the overriding declaration overrides a function in a base class that is an overload of the hidden declaration
@@ -36,8 +38,7 @@ where
     // and the hidden declaration isn't explicitly brought in scope through a using declaration.
     not exists(UsingDeclarationEntry ude |
       ude.getDeclaration() = hiddenDecl.getDeclaration() and
-      ude.getEnclosingElement() = overridingDecl.getDeclaration().getDeclaringType() and
-      ude.getLocation().getStartLine() < overridingDecl.getLocation().getStartLine()
+      ude.getEnclosingElement() = overridingDecl.getDeclaration().getDeclaringType()
     )
     or
     // 2. if the overriding declaration doesn't override a base member function but has the same name

cpp/autosar/test/rules/A7-3-1/DefinitionNotConsideredForUnqualifiedLookup.expected

@@ -1 +1 @@
-| test.cpp:42:6:42:7 | declaration of f1 | Definition for 'f1' is not available for unqualified lookup because it is declared after $@ | test.cpp:39:12:39:13 | using f1 | using-declaration |
+| test.cpp:46:6:46:7 | declaration of f1 | Definition for 'f1' is not available for unqualified lookup because it is declared after $@ | test.cpp:43:12:43:13 | using f1 | using-declaration |

cpp/autosar/test/rules/A7-3-1/HiddenInheritedNonOverridableMemberFunction.expected

@@ -1 +1 @@
-| test.cpp:16:8:16:9 | declaration of f1 | Declaration for member 'f1' hides non-overridable inherited member function $@ | test.cpp:7:8:7:9 | declaration of f1 | f1 |
+| test.cpp:20:8:20:9 | f1 | Declaration for member 'f1' hides non-overridable inherited member function $@ | test.cpp:7:8:7:9 | f1 | f1 |

cpp/autosar/test/rules/A7-3-1/HiddenInheritedOverridableMemberFunction.expected

@@ -1,5 +1,5 @@
-| test.cpp:18:8:18:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:9:16:9:17 | declaration of f2 | f2 |
-| test.cpp:18:8:18:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:11:16:11:17 | declaration of f2 | f2 |
-| test.cpp:23:8:23:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:9:16:9:17 | declaration of f2 | f2 |
-| test.cpp:23:8:23:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:10:16:10:17 | declaration of f2 | f2 |
-| test.cpp:23:8:23:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:11:16:11:17 | declaration of f2 | f2 |
+| test.cpp:22:8:22:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:9:16:9:17 | declaration of f2 | f2 |
+| test.cpp:22:8:22:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:11:16:11:17 | declaration of f2 | f2 |
+| test.cpp:27:8:27:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:9:16:9:17 | declaration of f2 | f2 |
+| test.cpp:27:8:27:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:10:16:10:17 | declaration of f2 | f2 |
+| test.cpp:27:8:27:9 | declaration of f2 | Declaration for member 'f2' hides overridable inherited member function $@ | test.cpp:11:16:11:17 | declaration of f2 | f2 |

cpp/autosar/test/rules/A7-3-1/test.cpp

@@ -9,6 +9,10 @@ class C1 {
   virtual void f2(int);
   virtual void f2(double);
   virtual void f2(S1);
+
+private:
+  void f3(int);
+  void f4(int);
 };
 
 class C2 : public C1 {
@@ -47,7 +51,7 @@ void f1() {
   l1.f1(0); // calls C2::f1(double) instead of C1::f1(int)
   l1.f2(0); // calls C2::f2(double) instead of C1::f2(int)
   // S1 s1;
-  // l1.f2(s1); Won't compile because there is no suitable conversion fro S1 to
+  // l1.f2(s1); Won't compile because there is no suitable conversion from S1 to
   // double.
   C1 &l2{l1};
   l2.f1(0); // calls C1::f1(int)
@@ -60,3 +64,31 @@ void f1() {
   S1 l4;
   l3.f2(l4); // calls C1:f2(S1)
 }
+
+class C5 : public C1 {
+public:
+  void f1(double); // COMPLIANT
+  using C1::f1;    // order of using and f1 declaration is not relevant
+
+  void f2(double) override; // COMPLIANT
+  using C1::f2;             // order of using and f2 declaration is not relevant
+};
+
+void f2() {
+  C5 c5;
+  c5.f1(0);   // calls C1::f1(int)
+  c5.f1(0.0); // calls C5::f1(double)
+  c5.f2(0);   // calls C1::f2(int)
+  c5.f2(0.0); // calls C5::f2(double)
+}
+
+class C6 : public C1 {
+public:
+  C6 &operator=(const C6 &); // COMPLIANT
+};
+
+class C7 : public C1 {
+  void f3(int); // COMPLIANT
+
+  void f4(int); // COMPLIANT
+};