C++: Add support for C++ requires expressions #17740

jketema · 2024-10-11T12:35:41Z

Pull Request checklist

All query authors

A change note is added if necessary. See the documentation in this repository.
All new queries have appropriate .qhelp. See the documentation in this repository.
QL tests are added if necessary. See Testing custom queries in the GitHub documentation.
New and changed queries have correct query metadata. See the documentation in this repository.

Internal query authors only

Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
Changes are validated at scale (internal access required).
Adding a new query? Consider also adding the query to autofix.

cpp/ql/lib/semmle/code/cpp/Concept.qll

cpp/ql/lib/semmle/code/cpp/internal/QualifiedName.qll

calumgrant · 2024-10-14T08:54:09Z

What is the IR generated for requires expressions? My feeling is that we should exclude requires from IR because it isn't code that is actually executed, and any alerts that are surfaced in the requires block are likely to be of a completely different character. It doesn't have any kind of memory representation for example.

jketema · 2024-10-14T08:55:55Z

What is the IR generated for requires expressions?

There isn't any, because these are constant expressions, and they also have a value child. The value will occur in the IR, because that may be assigned to something.

jketema · 2024-10-14T09:04:53Z

Note that the IR for the test I added in #17693 is not changed by this PR.

calumgrant

A few minor things but overall LGTM.

calumgrant · 2024-10-14T09:29:40Z

cpp/ql/lib/semmlecode.cpp.dbscheme

@@ -1909,6 +1912,10 @@ case @expr.kind of
            | @istriviallyrelocatable
            ;

+compound_requirement_is_noexcept(
+    unique int expr: @compound_requirement ref


unique is implicit here. Could an expression be possible here, similar to fun_decl_noexcept? In that case, this predicate is compound_requirement_empty_noexcept.

Another strategy could be to unite this predicate with fun_decl_noexcept/fun_decl_empty_noexcept, but that might be more work.

unique is implicit here

Do you mean that I can omit it?

Could an expression be possible here, similar to fun_decl_noexcept?

No, that's not allowed. It's just a keyword without any arguments.

unique is implicit here

Do you mean that I can omit it?

Yes I would think so. You can't have duplicated rows in a tuple.

Could an expression be possible here, similar to fun_decl_noexcept?

No, that's not allowed. It's just a keyword without any arguments.

Ok.

Dropped the unique.

calumgrant · 2024-10-14T09:31:02Z

cpp/ql/lib/semmlecode.cpp.dbscheme

@@ -2168,11 +2175,11 @@ stmt_decl_entry_bind(
    int decl_entry: @element ref
 );

-@functionorblock = @function | @stmt_block;
+@functionorblockorrequiresexpr = @function | @stmt_block | @requires_expr;


Nit: could rename this to @block_parent or something descriptive.

Note that "block" has a special meaning here: https://clang.llvm.org/docs/BlockLanguageSpec.html

I'm open to a more sensible name though, just not sure what it should be.

How about parameterized_element?

Replaced by parameterized_element

calumgrant · 2024-10-14T09:47:09Z

cpp/ql/lib/semmle/code/cpp/internal/QualifiedName.qll

-  int index;
-
-  Parameter() { params(this, function, index, _) }
+  Parameter() { params(this, _, _, _) }


Is this line needed?

I guess not. I mostly simplified this, because ql-for-ql started explaining, but didn't think beyond that.

calumgrant · 2024-10-14T10:10:37Z

cpp/ql/lib/semmle/code/cpp/Concept.qll


  override string getAPrimaryQlClass() { result = "RequiresExpr" }
+
+  /**
+   * Gets a parameter of this requires expression.


Suggested change

* Gets a parameter of this requires expression.

* Gets a parameter of this requires expression, if any.

calumgrant · 2024-10-14T10:12:35Z

cpp/ql/lib/semmle/code/cpp/Concept.qll

+
+  /** Gets the number of parameters of this requires expression. */
+  int getNumberOfParameters() { result = count(this.getAParameter()) }
+}


I would expect there to be a predicate to access the body as well. - getBlock()

Oh, good point.

getRequirement/1 getARequirement/0 and getNumberOfRequirements/0 added.

calumgrant · 2024-10-14T10:16:33Z

cpp/ql/lib/semmle/code/cpp/Concept.qll

+}
+
+/**
+ * A C++ concept id expression


Suggested change

* A C++ concept id expression

* A C++ concept id expression.

calumgrant · 2024-10-14T10:21:17Z

cpp/ql/test/library-tests/ir/ir/PrintAST.expected

 #-----|               Type = [BoolType] bool
 #-----|               Value = [RequiresExpr] 1
 #-----|               ValueCategory = prvalue
+#-----|             <params>: 
+# 2692|             getChild(0): [GTExpr,SimpleRequirementExpr] ... > ...


Accessing the body via getBlock() might be nicer.

Done, see above for the actual predicate name I used.

calumgrant · 2024-10-14T10:28:02Z

cpp/ql/lib/semmle/code/cpp/Concept.qll

+  Expr getExpr() { result = this.getChild(0) }
+
+  /**
+   * Gets the return type requirement from the compound requirement (if any).


Suggested change

* Gets the return type requirement from the compound requirement (if any).

* Gets the return type requirement from the compound requirement, if any.

calumgrant · 2024-10-14T10:30:26Z

cpp/ql/lib/change-notes/2014-10-11-requires-expressions.md

+---
+category: feature
+---
+* Added classes `RequiresExpr`, `SimpleRequirementExpr`, `TypeRequirementExpr`, `CompoundRequirementExpr`, and `NestedRequirementExpr` to represent C++20 requires expressions and the simple, type, compound, and nested requirements that can occur in requires expressions.


Suggested change

* Added classes `RequiresExpr`, `SimpleRequirementExpr`, `TypeRequirementExpr`, `CompoundRequirementExpr`, and `NestedRequirementExpr` to represent C++20 requires expressions and the simple, type, compound, and nested requirements that can occur in requires expressions.

* Added classes `RequiresExpr`, `SimpleRequirementExpr`, `TypeRequirementExpr`, `CompoundRequirementExpr`, and `NestedRequirementExpr` to represent C++20 requires expressions and the simple, type, compound, and nested requirements that can occur in `require` expressions.

I assume you mean "requires expressions"?

calumgrant · 2024-10-14T11:10:15Z

cpp/ql/lib/semmle/code/cpp/PrintAST.qll

-  override AstNode getChildInternal(int childIndex) {
-    result.getAst() = expr.getChild(childIndex)
+  override PrintAstNode getChildInternal(int childIndex) {
+    result.(AstNode).getAst() = expr.getChild(childIndex)


Nit: Consider adding a predicatePrintAst::getAst() { none() }.

This in consistent with how this is done in other places in this library, so I'm going to leave this as-is for now.

Left as-is.

calumgrant · 2024-10-14T11:54:55Z

cpp/ql/lib/semmle/code/cpp/Concept.qll

 */
 class RequiresExpr extends Expr, @requires_expr {
-  override string toString() { result = "requires ..." }
+  override string toString() { result = "requires(...) { ... }" }


Nit: Consider something like if exists(this.getAParameter()) then result = "requires(...) { ... }" else result = "requires { ... }" }

Yes, I thought about that. However, I cannot really differentiate between empty and missing parameter lists, so I wasn't really sure about that.

jketema · 2024-10-14T18:38:14Z

Apologies for force pushing the fixes. The schema changes made it very difficult to do something else and still end up with a reasonable history.

jketema · 2024-10-14T18:42:05Z

cpp/ql/lib/semmle/code/cpp/Parameter.qll

- * A C/C++ function parameter or catch block parameter. For example the
- * function parameter `p` and the catch block parameter `e` in the following
+ * A C/C++ function parameter, catch block parameter, or requires expression parameter.
+ * For example the function parameter `p` and the catch block parameter `e` in the following


Note that these changes are new.

jketema · 2024-10-14T18:42:18Z

cpp/ql/lib/semmle/code/cpp/Parameter.qll

- * For catch block parameters, there is a one-to-one correspondence between
- * the `Parameter` and its `ParameterDeclarationEntry`.
+ * For catch block parameters and expression , there is a one-to-one
+ * correspondence between the `Parameter` and its `VariableDeclarationEntry`.


These changes are new.

calumgrant

Great work!

github-actions bot added C++ documentation labels Oct 11, 2024

jketema added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Oct 11, 2024

jketema force-pushed the requires branch from 005d8f4 to 0606fc1 Compare October 11, 2024 12:39

github-advanced-security bot found potential problems Oct 11, 2024

View reviewed changes

cpp/ql/lib/semmle/code/cpp/Concept.qll Fixed Show fixed Hide fixed

cpp/ql/lib/semmle/code/cpp/internal/QualifiedName.qll Fixed Show fixed Hide fixed

jketema force-pushed the requires branch 8 times, most recently from ed70f88 to db98433 Compare October 12, 2024 12:18

jketema marked this pull request as ready for review October 14, 2024 05:58

jketema requested a review from a team as a code owner October 14, 2024 05:58

calumgrant reviewed Oct 14, 2024

View reviewed changes

jketema force-pushed the requires branch 2 times, most recently from 3acd474 to 0df5305 Compare October 14, 2024 18:36

jketema commented Oct 14, 2024

View reviewed changes

calumgrant approved these changes Oct 15, 2024

View reviewed changes

C++: Add support for C++ requires expressions

168f7f5

jketema force-pushed the requires branch from 0df5305 to 168f7f5 Compare October 15, 2024 11:44

jketema merged commit 50ec254 into github:main Oct 15, 2024
15 of 16 checks passed

jketema deleted the requires branch October 15, 2024 12:32

jketema mentioned this pull request Oct 16, 2024

C++: Improve parameter naming #17778

Merged

7 tasks

	* Gets a parameter of this requires expression.
	* Gets a parameter of this requires expression, if any.

	* Gets the return type requirement from the compound requirement (if any).
	* Gets the return type requirement from the compound requirement, if any.

	* Added classes `RequiresExpr`, `SimpleRequirementExpr`, `TypeRequirementExpr`, `CompoundRequirementExpr`, and `NestedRequirementExpr` to represent C++20 requires expressions and the simple, type, compound, and nested requirements that can occur in requires expressions.
	* Added classes `RequiresExpr`, `SimpleRequirementExpr`, `TypeRequirementExpr`, `CompoundRequirementExpr`, and `NestedRequirementExpr` to represent C++20 requires expressions and the simple, type, compound, and nested requirements that can occur in `require` expressions.

C++: Add support for C++ requires expressions #17740

C++: Add support for C++ requires expressions #17740

Conversation

jketema commented Oct 11, 2024

Pull Request checklist

All query authors

Internal query authors only

calumgrant commented Oct 14, 2024

jketema commented Oct 14, 2024

jketema commented Oct 14, 2024

calumgrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant left a comment

Choose a reason for hiding this comment

jketema Oct 14, 2024 •

edited

Loading

calumgrant Oct 14, 2024 •

edited

Loading

calumgrant Oct 14, 2024 •

edited

Loading