[Python AST] AST Viewer shows [Str] but no such type exists

[fcasal]

Description of the issue

I expect the type names present on the AST viewer to match a real CodeQL type, but this is not the case here. The type could be either Str_ or StrConst, both printing "Str" as their toString().

The Str_ class also mentions the Str class which I cannot find: https://github.com/github/codeql/blob/d50816a284e71f50d0c937928ec5a8521c9dba57/python/ql/lib/semmle/python/AstGenerated.qll#L1240

[aeisenberg]

What is the code snippet that produces this AST?

[fcasal]

Hi @aeisenberg, you can reproduce with a simple file such as

class A:
    NAME = "A"

I created the database with codeql database create test.db --language=python and the AST for that file looks like

[aeisenberg]

Thanks. I am asking our python team for a detailed answer.

[tausbn]

Thank you for your question. In this case, StrConst is the correct class. It's not immediately obvious why this class isn't called Str, and at this point the reason is probably lost to the mists of time.

Unfortunately, because AstGenerated.qll is generated automatically, it's not trivial to make it refer to StrConst instead of Str in the corresponding QLDoc.

In the short term, we'll probably just override the toString method on StrConst to say StrConst (instead of inheriting the string Str from the Str_ class).

[fcasal]

In the short term, we'll probably just override the toString method on StrConst to say StrConst (instead of inheriting the string Str from the Str_ class).

This makes total sense to me, and if you'd like I can open a PR that does it.

[tausbn]

This makes total sense to me, and if you'd like I can open a PR that does it.

I mean, I certainly won't prevent you from doing that, but I think you'll find it's going to be quite a lot of hassle to update all of the tests that currently say Str to instead say StrConst. :slightly_smiling_face:

I'm wondering if a less painful solution would be to add a Str class so that StrConst extends Str, and Str extends Str_. Then the QLDoc on Str_ would make sense again, and the QLDoc for Str could just say "placeholder class -- see StrConst" and none of the tests would need to be updated.

That way, a user of the AST viewer would at least have a fighting chance of finding the right class to look at.

I'll make a PR for this.