Rust: Add `LiteralExpr` sub classes #19475

hvitved · 2025-05-12T18:20:50Z

In preparation for #19474.

Eventually we will also want to have predicates for retrieving the literal values, e.g. int IntegerLiteralExpr.getValue(), but that is left as future work.

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

paldepind

A few minor comments and I haven't dug into all the specifics of the regular expressions — otherwise looks good to me :)

paldepind · 2025-05-13T09:36:04Z

rust/ql/lib/codeql/rust/elements/LiteralExprExt.qll

@@ -0,0 +1,15 @@
+/** Provides sub classes of literal expressions. */


What does the Ext suffix in the filename indicate? I can't find any existing files with that suffix.

My first thought was it meant that these could be _ext_ended, but that's not the case.

I think it just means the file is an extension to the functionality provided in LiteralExpr.qll.

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

paldepind · 2025-05-13T10:00:32Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+        s = this.getTextValue() and
+        reg = IntegerLiteralRegexs::integerLiteral() and
+        last = strictcount(reg.indexOf("(")) and
+        result = s.regexpCapture(reg, last)


What about a helper predicate for getting the last capture group? Something like:

bindingset[s, reg] string regexpCaptureLast(string s, string reg) { exists(int i | result = s.regexpCapture(reg, i) and not exists(s.regexpCapture(reg, i + 1))) }

I don't think that will work; it will give you the last group that matches, but the suffix part is optional.

paldepind · 2025-05-13T10:02:56Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+        reg =
+          [
+            FloatLiteralRegexs::floatLiteralSuffix1(), FloatLiteralRegexs::floatLiteralSuffix2(),
+            FloatLiteralRegexs::integerSuffixLiteral()
+          ] and


Using the helper predicate above we could instead do a disjunction here:

Suggested change

reg =

[

FloatLiteralRegexs::floatLiteralSuffix1(), FloatLiteralRegexs::floatLiteralSuffix2(),

FloatLiteralRegexs::integerSuffixLiteral()

] and

reg =

FloatLiteralRegexs::floatLiteralSuffix1() + "|" +

FloatLiteralRegexs::floatLiteralSuffix2() + "|" +

FloatLiteralRegexs::integerSuffixLiteral()

and

geoffw0

Looks good but I have some questions and comments...

geoffw0 · 2025-05-13T08:37:10Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+   */
+  class CharLiteralExpr extends LiteralExpr {
+    CharLiteralExpr() {
+      // todo: proper implementation


What needs to be done to make this a "proper implementation"?

I was initially concerned about escaped quote characters, but I think with lazy matching they may just work. Testing will confirm.

The logic is a lot simpler than the official spec, which is why I added the comment.

I suggest we remove the comment, or rephrase it away from "todo", if there is nothing specific we want to do here.

rust/ql/test/extractor-tests/literal/literal.rs

geoffw0 · 2025-05-13T08:52:14Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+  class StringLiteralExpr extends LiteralExpr {
+    StringLiteralExpr() {
+      // todo: proper implementation
+      this.getTextValue().regexpMatch("r?#*\".*\"#*")


What needs to be done to make this a "proper implementation"?

I think it's OK that this matches some invalid string literals (e.g. r##"foo"#) as long as it matches all valid ones and there's no overlap with matching for literals of other types.

Same as for char literals, the spec is more involved, but perhaps as you say the above is good enough.

geoffw0 · 2025-05-13T09:06:54Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+        s = this.getTextValue() and
+        reg = IntegerLiteralRegexs::integerLiteral() and
+        last = strictcount(reg.indexOf("(")) and
+        result = s.regexpCapture(reg, last)


This is a bit of a scary approach, I'm not convinced it will be reliable.

Can't we just match against ".*(" + IntegerLiteralRegexs::suffix() + ")" here and grab the first group?

I would really like to be able to just reuse the existing regexes; one way this could be made better is if QL supported named groups.

In general the number of opening brackets isn't going to be equal to the number of capture groups, for example the regex (?is)(\\() has three ( but only one group. You might be able to stick to simpler regexs where the relationship holds but it feels a bit fragile to me.

Agree. Would it be possible to write the regex'es such that the thing we want is always in a fixed capture group? Then we could write something like s.regexpCapture(reg, 3) (or whatever the number it would be).

Sure, I can hard-code the numbers instead.

geoffw0 · 2025-05-13T10:01:07Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+            FloatLiteralRegexs::integerSuffixLiteral()
+          ] and
+        last = strictcount(reg.indexOf("(")) and
+        result = s.regexpCapture(reg, last)


Same thoughts as for IntegerLiteralExpr::getSuffix.

geoffw0 · 2025-05-13T10:10:00Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+          .regexpMatch([
+              FloatLiteralRegexs::floatLiteral(), FloatLiteralRegexs::integerSuffixLiteral()
+            ]) and
+      // E.g. `0x01_f32` is an integer, not a float


I'm confused by the cases 0x01_f32 and 0x01_e3. I understand they're supposed to be understood as integers, I'm not sure why this is so (in the Rust language) and I'm not sure how it happens (in your QL).

It is exactly the restriction on this line that makes it an integer only; otherwise it would be consider a float as well.

I don't see how IntegerLiteralExpr accepts 0x01_f32 when its suffix() function doesn't include f32.

0x01_f32 is just a HEX number, underscores are allowed as separators.

paldepind

🎉

paldepind

Nice refactor and TIL about non-capturing groups ;)

paldepind · 2025-05-13T13:34:02Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

+            IntegerLiteralRegexs::paren(FloatLiteralRegexs::floatLiteralSuffix2()) + "|" +
+            IntegerLiteralRegexs::paren(FloatLiteralRegexs::integerSuffixLiteral()) and
+        s = this.getTextValue() and
+        result = s.regexpCapture(reg, [1, 2, 3])


How does [1, 2, 3] not end up mixing things up? Do they all three always correspond to the suffix for all the three disjuncts?

They each correspond to a suffix capturing group for one of the disjuncts, so all should be good.

Sorry for being dense, but if 3 is the right capture group for one of the disjuncts why won't trying 1 or 2 give the wrong thing for that disjunct?

Trying 1 for, say, the third disjunct will simply not give a result, since capture group 1 does not exist in that disjunct, it exists in the first disjunct.

github-actions bot added the Rust Pull requests that update Rust code label May 12, 2025

github-advanced-security bot found potential problems May 12, 2025

View reviewed changes

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll Fixed Show fixed Hide fixed

Rust: Add LiteralExpr sub classes

cd01bd0

hvitved force-pushed the rust/literal-sub-classes branch from f93c86f to cd01bd0 Compare May 12, 2025 18:35

hvitved marked this pull request as ready for review May 13, 2025 07:12

hvitved requested a review from a team as a code owner May 13, 2025 07:12

paldepind requested changes May 13, 2025

View reviewed changes

geoffw0 reviewed May 13, 2025

View reviewed changes

Address review comments

7494eac

hvitved force-pushed the rust/literal-sub-classes branch from 6ec1565 to 7494eac Compare May 13, 2025 12:26

paldepind previously approved these changes May 13, 2025

View reviewed changes

Simplify using non-capturing groups

ae54c62

hvitved dismissed paldepind’s stale review via ae54c62 May 13, 2025 13:07

hvitved requested a review from paldepind May 13, 2025 13:32

paldepind approved these changes May 13, 2025

View reviewed changes

hvitved merged commit 3fc9da7 into github:main May 13, 2025
17 checks passed

hvitved deleted the rust/literal-sub-classes branch May 13, 2025 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: Add `LiteralExpr` sub classes #19475

Rust: Add `LiteralExpr` sub classes #19475

hvitved commented May 12, 2025 •

edited

Loading

paldepind left a comment

paldepind May 13, 2025

geoffw0 May 13, 2025 •

edited

Loading

hvitved May 13, 2025

paldepind May 13, 2025

hvitved May 13, 2025

paldepind May 13, 2025

geoffw0 left a comment

geoffw0 May 13, 2025

hvitved May 13, 2025

geoffw0 May 13, 2025

geoffw0 May 13, 2025

hvitved May 13, 2025

geoffw0 May 13, 2025

hvitved May 13, 2025

geoffw0 May 13, 2025

paldepind May 13, 2025

hvitved May 13, 2025

geoffw0 May 13, 2025

geoffw0 May 13, 2025

hvitved May 13, 2025

geoffw0 May 13, 2025

hvitved May 13, 2025

paldepind left a comment

paldepind left a comment

paldepind May 13, 2025

hvitved May 13, 2025

paldepind May 14, 2025

hvitved May 14, 2025

		@@ -0,0 +1,15 @@
		/** Provides sub classes of literal expressions. */

Rust: Add LiteralExpr sub classes #19475

Rust: Add LiteralExpr sub classes #19475

Conversation

hvitved commented May 12, 2025 • edited Loading

paldepind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffw0 May 13, 2025 • 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

geoffw0 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

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

paldepind left a comment

Choose a reason for hiding this comment

paldepind 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

Rust: Add `LiteralExpr` sub classes #19475

Rust: Add `LiteralExpr` sub classes #19475

hvitved commented May 12, 2025 •

edited

Loading

geoffw0 May 13, 2025 •

edited

Loading