C++: Define an extractor version table and use in IR generation

[jketema]

No description provided.

[MathiasVP]

The code changes LGTM! As far as I can tell this brings us back to a pre-frontend upgrade state when not exists(getExtractorVersion()) holds, and to a post-frontend upgrade state when exists(getExtractorVersion()) holds. Let's see what DCA says 🤞

I guess CI is failing because we're in the strange position where:

• The frontend has been upgraded, and
• There isn't a result for getExtractorVersion()

[jketema]

I guess CI is failing because we're in the strange position where:

• The frontend has been upgraded, and
• There isn't a result for getExtractorVersion()

Correct. It's the internal CI that'll give the real verdict here, but that was failing because of a missing stats file.

[redsun82]

wouldn't it get simpler to use by avoiding having to write lots of exists(getExtractorVersion()) if we had a default return value when the version is not in the DB?

Suggested change

	int getExtractorVersion() { extractor_version(result) }
	int getExtractorVersion() {
	extractor_version(result) or (not exists(int v | extractor_version(v)) and result = -1)
	}

(or 0 if actual versions start from 1).

Then checks like getExtractorVersion() < n would work as expected.

[jketema]

That seems like a very imperative way of thinking about this. Is that what we want?

[MathiasVP]

I agree with Jeroen, I think. I don't see why we should have a default value here (default values is not a thing we tend to use in QL since a predicate with "no value" "propagates outwards" so that whatever clause that uses getExtractorVersion() also produces no values).

With that said, @andersfugmann had a less controversial suggestion: Maybe the TranslatedExpr.qll should define a isBefore605FrontendUpgrade() predicate so that the exists(getExtractorVersion()) could be replaced with something slightly more meaningful.

I don't think we should do ^ in this PR since I'd like to avoid unnecessary submodule bumps wherever possible.

[jketema]

With that said, @andersfugmann had a less controversial suggestion: Maybe the TranslatedExpr.qll should define a isBefore605FrontendUpgrade() predicate so that the exists(getExtractorVersion()) could be replaced with something slightly more meaningful.

That makes sense to me. I can do that in this PR, since I need to rebase anyway, ...

[redsun82]

Yeah, a more explicit predicate name like isBefore605FrontendUpgrade sounds a very good alternative. Maybe we should make getExtractorVersion private within ExtractorVersion.qll and only expose such predicates with descriptive names from it?

About the default value, as a less experienced QL writer I know I could be bitten by writing for example getExtractorVersion() < 5. And that even if I was not bitten by it, I would have to rewrite the logic to something like getExtractorVersion() >= 5 or a worse not exists(getExtractorVersion()) or getExtractorVersion() < 5. But I am a less experienced QL writer 🙂

Btw, getLocation() does define a default in QL with UknownLocation, so I wasn't aware defaults are frowned upon.

[MathiasVP]

getLocation() is special because it interacts with non-QL things (like the VSCode extension)

[jketema]

Maybe we should make getExtractorVersion private within ExtractorVersion.qll and only expose such predicates with descriptive names from it?

Note that the file is marked as internal, so as a QL query writer you shouldn't even come near it.

[jketema]

I've updated the PR with a somewhat more declarative predicate name. Do I need to re-run DCA for this?

[MathiasVP]

LGTM!