C++: Improve Element::toString() performance

[zlaski-semmle]

by removing recursion in TypeMention::toString().

[jbj]

A bunch of tests are failing because their expected file includes the output of TypeMention.toString(). The test of TypeMention itself remains correct but has become much less informative. Please update the ql file for that test to add a column with the type name.

All the other tests look fine, but you'll need to update their expected file to match the new output. @geoffw0, can you confirm that it's fine to change the dependency-tests output like this?

Now that we've seen how much this change can affect test output, I think the change deserves to be mentioned in the change note. Please add a change note to describe how this change affects qltest files that select every Element (which should be rare).

[pavgust]

Would it be sufficient to change this to result = "mention of " + getMentionedType().getName() to avoid the recursion? That would hopefully keep the more informative to-string.

[jbj]

Yes, that would turn this into a PR that we can merge right away, without changes to tests or change note. I prefer result = "type mention" because then we avoid the time and space overhead of creating a new string for each type.

Another option would be result = getMentionedType().getName(), which would make the string informative again. It would be in line with variable accesses, whose toString is just the name of the variable. But as far as I know, nothing benefits from TypeMention.toString being informative -- except for one test that's easily fixed.

[geoffw0]

@geoffw0, can you confirm that it's fine to change the dependency-tests output like this?

I would imagine they're fine but I failed to find the actual test failures in the logs. I suggest adding the .expected changes (and/or trying out @pavgust's suggestion) and I can review more easily at that point, with the expectation that they're likely to be fine.

[jbj]

The tests couldn't run because of conflicts in the change note, so you'll need to rebase. When the tests do run, I expect they'll fail because the expected files in the internal repo also need to be upgraded. You'll need to make a PR to the internal repo as well, making its submodule point to the head of this PR, which should be freshly rebased on ql:master (not on the current target of the submodule pointer). Here's the guide: https://docs.google.com/document/d/1zOra3INgr06hxeyH3AFHn9JmSm8yJNnpH3BVQOBL4-k/edit#heading=h.4dw5b4p0d86g

[jbj]

Typo: Expr should be Element.

Also, let's not overstate the performance implications of this change. I like to avoid mentioning performance improvements and regressions in the change notes because even though it varies between releases, performance stays within the same ballpark over time while the quality of our results goes up.

What's important for customers to know is that if they have qltests that query every Element, then those tests will need their expected output updated.

[zlaski-semmle]

Must be committed together with https://git.semmle.com/Semmle/code/pull/32371