added a comment - - edited
Summarizing some cases here from before/after the patch:
1) top-level fn (always has name)
1.6 - namespace$name
patch - namespace$name
2) non-top-level fn with name
1.6 - namespace$name (collides with prior case)
patch - namespace$topname__x__name <-- CHANGED
3) anonymous fn (no name)
1.6 - namespace$name$fn__x
patch - namespace$name$fn__x
4) top-level anonymous fn (no name, not at all useful :)
1.6 - namespace$fn__x
patch - namespace$fn__x
The key problem is that the first 2 cases produce the identical class name on 1.6. The patch alters the non-top-level named fn so there is no conflict.
Prior to the referenced old commit, I believe cases 1 and 2 would both produce namespace$name__x (where x is unique) so they would not collide. The change was made to prevent the top-level name from changing ("don't append numbers on top-level fn class names"). While the similar change was made on non-top-level fn names, I do not think it needed to be.
I've thought through (and tried) a bunch of the implications of this with the help of Nicola's comments above and I do not see an issue with any of the things I've considered. From a binary compatibility point of view with existing AOT code, old code compiled together should be self-consistent and continue to work. New compiled code will also be consistent. I can't think of a way that new code would expect to know the old name of a non-top-level function such that there could be an issue.
One question - why change the code such that the new class name is namespace$name$topname__x__name instead of namespace$name$topname_name__x (or something else?). And relatedly, while the diff is small, could we refactor a couple more lines to make the intent and cases clearer?
I am 90% ok with this patch but want a little thought into that question before I mark screened.