Results whose location.endColumn == 0 are incorrectly linked

[kjcolley7]

When writing queries to explore YAML files (using Javascript's YAML.qll library), top-level YAMLMapping instances always have their endColumn set to zero. This is not an issue, as I'd expect it to have the selection region end at the beginning of endLine with no characters on that line selected, just the newline from the previous line and stuff before that. The problem is that endColumn > 0 is explicitly checked in the extension's source code, and zero is considered an invalid value for endColumn:

vscode-codeql/extensions/ql-vscode/src/pure/bqrs-utils.ts

Line 87 in 3e0ea1b

&& loc.endColumn > 0;

In all of these cases, endLine > startLine and endColumn == 0.

Version
CodeQL extension version: 1.5.6 CodeQL CLI version: 2.7.0 Platform: darwin x64

To reproduce
Run this query on any database from any language (tested on Python, should also work for Javascript at least):

// Just find some random file from the database with at least 3 lines of code
string getFilePath() {
	result = min(@location l, @file f, string fp, int el
		| locations_default(l, f, _, _, el, _) and files(f, fp) and el >= 3 | fp
	)
}

abstract class LocationBug extends string {
	bindingset[this] LocationBug() { any() }
	abstract predicate hasLocationInfo(string fp, int sl, int sc, int el, int ec);
}

class BadLocation extends LocationBug {
	BadLocation() { this = "BadLocation" }
	override predicate hasLocationInfo(string fp, int sl, int sc, int el, int ec) {
		fp = getFilePath() and sl = 2 and sc = 0 and el = 3 and ec = 0
	}
}

class GoodLocation extends LocationBug {
	GoodLocation() { this = "GoodLocation" }
	override predicate hasLocationInfo(string fp, int sl, int sc, int el, int ec) {
		fp = getFilePath() and sl = 2 and sc = 0 and el = 3 and ec = 1
	}
}

from LocationBug bug
select bug

Expected behavior
Clicking on either result in the table links to the source file with the entire second line highlighted. What actually happens is that the BadLocation result links just to the top of the file, whereas the GoodLocation result goes to the second line completely highlighted and the first character of the third line highlighted.

If the intent of that check is to ensure the end is past the start, then it would make more sense to do it like:

&& (
    loc.endLine > loc.startLine
    || loc.endLine == loc.startLine && loc.endColumn >= loc.startColumn
)

[aeisenberg]

Thanks for your bug report. I am chatting with the yaml extractor maintainers to determine a way forward.

[kjcolley7]

Regardless of whether the YAML extractor should emit locations whose endColumn == 0, since that logically makes sense and can be represented by a selection region in VSCode, I think the CodeQL extension should support this case. The YAML extractor is probably not the only thing that results in a location with endColumn == 0. Could be some other extractor, or even somebody's custom predicate hasLocationInfo.

[aeisenberg]

I chatted with the yaml extractor maintainers, and I now have a better idea of what is happening. 0-based column indexes are not explicitly supported in the CodeQL CLI, but they do mostly work in that they match the last character of the previous line. Similarly, specifying a column offset after the end of the line starts matching on the next line. This is all easy enough to do inside of the CLI since it has easy access to the full file contents.

To be clear, the docs indicate that both line and column offsets are 1-based. https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/ Using 0 or values greater than the end column work, but it is undocumented.

Several things to note here:

1. In the extension, we need to translate from CodeQL positions to vscode positions. vscode positions are 1-based.
2. Handling the undocumented behaviour will be difficult since the extension does not have easy access to the contents when it generates the request to open the file.

So, I don't think this is a bug in the YAML extractor. It's possibly a bug in the CLI, but it's probably too late to change. At the very least, the documentation page should change. Also, the vscode extension should avoid failing if it receives a surprising location. It should translate it into something it can handle even if it is not precisely what is requested. If it turns out there is a need to handle locations exactly the same way that the CLI does, we can look into that later.

[kjcolley7]

Sounds like a good approach to me!