Go: Check more things while running tests by owen-mc · Pull Request #19491 · github/codeql

owen-mc · 2025-05-14T10:01:35Z

This PR adds the following flags when we run tests: --fail-on-trap-errors, --check-undefined-labels, --check-unused-labels, --check-repeated-labels, --check-redefined-labels, --check-use-before-definition. There are also two extractor fixes to make tests pass with these flags:

Avoid making unused labels in certain situations.
Fix a db inconsistency on Windows by making the Go extractor use "C:/" for the drive root instead of "C:", so it matches up with the XML extractor (which we use for HTML files).

owen-mc · 2025-05-15T09:51:11Z

The CI has highlighted a db inconsistency that only happens on windows: the container parent of "C:/a" is recorded as both "C:/" and "C:". This only happens in tests with html files, hence it must be related to the xml extractor.

Copilot

Pull Request Overview

Adds more validation flags to the CodeQL test command and refactors the Go extractor’s path handling and nil checks.

Introduces rawPath/canonicalPath in extractFileInfo to normalize file paths.
Simplifies nil checks in extractExpr, extractStmt, and extractDecl by moving them to the top.
Updates test runner flags in Makefile and adjusts GitHub Actions Windows runner configuration.
Aligns greet.go import alias with updated definitions expected output.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
go/ql/test/query-tests/definitions/greet.go	Renamed import to `myfmt` and updated print call
go/ql/test/query-tests/definitions/definitions.expected	Added new definition entry for `myfmt`
go/extractor/extractor.go	Introduced `rawPath`/`canonicalPath`, refactored nil checks
go/Makefile	Added new `--check-*` flags to `codeql test run`
.github/workflows/go-tests-other-os.yml	Changed `runs-on` from `windows-latest-xl` to `windows-latest`

Comments suppressed due to low confidence (1)

go/Makefile:57

Remove the --check-unused-labels flag, as noted in the PR description, to prevent unintended test failures caused by trap errors and ensure consistency with the intended test configuration.

codeql test run -j0 ql/test --search-path .. --consistency-queries ql/test/consistency --compilation-cache=$(cache) --dynamic-join-order-mode=$(rtjo) --check-databases --fail-on-trap-errors --check-undefined-labels --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition

Copilot · 2025-05-20T12:01:39Z

-		if path != "/" {
-			parentPath = path
-		}
+		parentPath = rawPath


Unconditionally assigning parentPath = rawPath will make the next iteration build rawPath differently when rawPath is empty (root). Consider guarding this assignment for root paths (e.g. only set parentPath when rawPath is non-empty) to preserve correct canonical paths.

Suggested change

parentPath = rawPath

if rawPath != "" {

parentPath = rawPath

}

This makes the test slightly more thorough.

In go, an interface with value nil does not compare equal to nil. This is known as "typed nils". So our existing nil checks weren't working, which shows why we needed more nil checks inside the type switches. The solution is to explicitly check for each type we care about.

smowton

Path-formatting and test flag changes LGTM. Suggest not making the nil-check changes.

smowton · 2025-05-21T10:03:40Z

 // extractExpr extracts AST information for the given expression and all its subexpressions
 func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int, skipExtractingValue bool) {
-	if expr == nil {
+	if expr == nil || expr == (*ast.Ident)(nil) || expr == (*ast.BasicLit)(nil) ||


TIL: this makes sense. It does, though! Weak preference for keeping the old code, since as/when there are new AST nodes it is now easier to accidentally not add the nil check.

as a Go neophyte, after doing a quick google search, would something like expr == nil || reflect.ValueOf(i).IsNil() work and avoid the problem?

It's a definite footgun in the language, which I think they probably should have a better solution for. Btw there is an alternative which I saw suggested when I searched for this online, which is to use reflection to check if the interface type has a nil value. I thought this was too ugly, but it would address the concern that we need to remember to add any new types to the check.

You want to leave in the nil checks in the branches of the type switch? I don't really agree, but I'll do it as it isn't worth debating.

My comment above was a response to @smowton 's comment. I'm not sure about the exact formulation of using reflection. Since we know it will be an interface and not a map or channel or anything like that, it shouldn't be too hard.

If the reflection isn't too costly that's fine. After that I'd say a check in each branch is slightly better the a mass of checks up front, because the up-front ones can drift out of sync with the arms of the switch too easily.

It's a definite footgun in the language

So true! TIL, I'll definitely keep that in mind for next time I might dabble with Go.

Another idea occurred to me: we just move the two lines that are above the type switch into every case clause. Then the nil check works as expected. There is duplication, but it isn't too bad, and anyone adding another case in future will be able to copy from other case clauses without any risk of error. I think if reflection works and is performant then that is probably my first choice, followed by this duplication approach.

redsun82 · 2025-05-21T10:26:28Z

 // extractExpr extracts AST information for the given expression and all its subexpressions
 func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int, skipExtractingValue bool) {
-	if expr == nil {
+	if expr == nil || expr == (*ast.Ident)(nil) || expr == (*ast.BasicLit)(nil) ||


as a Go neophyte, after doing a quick google search, would something like expr == nil || reflect.ValueOf(i).IsNil() work and avoid the problem?

owen-mc · 2025-05-22T09:54:13Z

I have switched to using reflection for the nil check on interfaces. DCA showed no significant changes in build time - on the two largest repos it reduces it, if anything. I will do a QA run to be sure.

owen-mc · 2025-05-22T21:17:21Z

QA showed no negative consequences. The build step was took on average 1.68% less time and the analysis step took on average 1% less time. It's tempting to think that this PR caused that, but it may well just be noise.

There was one alert difference - I'll double check it locally.

owen-mc · 2025-05-23T14:36:06Z

I wasn't able to reproduce the alert difference locally. My best guess was that the repo was updated between the left run and the right run, but actually it hasn't been updated for two years.

owen-mc added the no-change-note-required This PR does not need a change note label May 14, 2025

github-actions Bot added the Go label May 14, 2025

owen-mc force-pushed the go/add-test-flags branch 3 times, most recently from c42710a to afc4bf3 Compare May 20, 2025 02:24

owen-mc marked this pull request as ready for review May 20, 2025 12:00

Copilot AI review requested due to automatic review settings May 20, 2025 12:00

owen-mc requested a review from a team as a code owner May 20, 2025 12:00

Copilot AI reviewed May 20, 2025

View reviewed changes

owen-mc added 6 commits May 20, 2025 13:13

Check more things while running tests

14af921

Added named import to definitions test

d39e7c2

This makes the test slightly more thorough.

Deal better with Windows paths

d5044fd

fix previous commit

47dac64

Change variable name and add comment

83cd349

owen-mc force-pushed the go/add-test-flags branch from 85d5922 to 83cd349 Compare May 20, 2025 12:13

smowton reviewed May 21, 2025

View reviewed changes

redsun82 reviewed May 21, 2025

View reviewed changes

Use reflection for interface nil check instead

463a711

owen-mc requested a review from smowton May 22, 2025 21:17

smowton approved these changes May 27, 2025

View reviewed changes

owen-mc merged commit 6f71e3b into github:main May 28, 2025
13 checks passed

owen-mc deleted the go/add-test-flags branch May 28, 2025 09:12

Conversation

owen-mc commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owen-mc commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc commented May 22, 2025

Uh oh!

owen-mc commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owen-mc commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

owen-mc commented May 14, 2025 •

edited

Loading

owen-mc commented May 15, 2025 •

edited

Loading

owen-mc commented May 22, 2025 •

edited

Loading

owen-mc commented May 23, 2025 •

edited

Loading