bug-2031036: Failure to remove arguments in signature with "(anonymous namespace)"

[Haseeb702]

Because:

• bug-2031036

This PR:

• replaces the (anonymous namespace) with a placeholder value and removes it from the exceptions, thus allowing the argument with the (anonymous namespace) to be removed

Example:

• Initial signature: mozilla::SpinEventLoopUntil(nsTSubstring<T> const&, (anonymous namespace)::ParentImpl::ShutdownBackgroundThread::<T>&&, nsIThread*)
• Post fix signature: mozilla::SpinEventLoopUntil

[Haseeb702]

Here I replace the "(anonymous namespace)" with a placeholder and remove it from the exceptions so that it's removed if it appears in an argument. In addition, when the signature is leading with "(anonymous namespace)" it is not removed since we replace it with a placeholder value. After the collapse() is done, we replace the placeholder with the "(anonymous namespace)" to get the correct signature.

[smarnach]

Thanks for this pull request! This is a clever way of avoiding the problem.

I still have some concerns about the approach. The current implementation of collapse() is a bit confusing, and the meaning of the exceptions parameter to that function is ambiguous. Your fix simply side-steps these problems by adding yet another special case to the code.

In the long run, this makes a codebase almost impossible to reason about. Part of the codebase are already quite difficult to understand. We should aim to improve that situation, and leave every bit of code that we should behind in a slightly better shape than it was in before.

Here is an idea what you could do instead. Instead of accepting a list of strings as exceptions, the collapse() function could accept a function is_exception that receives the string before a section that would be replaced and the section that would be replaced itself as parameters and returns a Boolean value indicating whether this is an exception. This would allow simplifying the body of collapse(), and also make the meaning of the parameter much clearer than it is now.

There are certainly better approaches than this suggestion I just made up without putting much thought into it. My main point is – let's try and make this code better while we are here, rather than just fixing the bug.

In some situations, "proper" fixes are not really possible. They may be unreasonably complex, or you simply may not have time for them, and then you have to settle for a hack and move on. However, I think that in this particular case there is a way to simplify the code while fixing the bug that can be implemented with reasonable effort.

[Haseeb702]

Another solution I tried was to edit the _is_exceptions() function in socorro/signature/utils.py by changing line 203 to if token == s: so that it checks the exact string and doesn't ignore the (anonymous namespace) that is embedded in the arguments. This worked for cpp normalization but failed at the rust because of the " as " exception. Now i'm going to look into the solution that @smarnach recommended.

[biancadanforth]

Since I was answering a question in Slack, and I don't see it mentioned in Sven's review: The issue is that collapse considers it an exception in both the case that 1) the exception string occurs before the start token or 2) somewhere in between the start and end token. The reason why "anonymous namespace" was included as an exception in the first place was because we do want to keep it in the function string if it occurs in case 1. The bug reported here is that we don't want to keep it if it's case 2.

So, the patch as currently written breaks the intended behavior to leave (anonymous namespace) in tact for case 1. We can check with amccreight (the reporter in the bug) to confirm that's still desired, but it so, then whatever solution we go with should not regress that.

Edit: Nevermind, your solution doesn't break that (as the tests indicate).