fix: Enable creation of packages with repeated simple names in qualified name

[slarse]

Fix #4764

The problem was a check that I can't quite make sense of, see comment in code.

[slarse]

The comment implies that the package A to which we are trying to add the package B are the same by comparing their qualified names. This simply does not make sense, as the act of add B as a subpackage to A changes its qualified name to A.B.

It's simply impossible for A and B to end up with the same qualified name after adding B to A as the qualified name of B will always be prefixed with A..

[I-Al-Istannen]

I was quite confused about the check as well when adjusting this class. It sounded like this was meant to merge equal packages. If I didn't miss anything, the two names can be the same at the time of the check, if you have two parallel package hierarchies. The linking is only done in the super call, so the parent isn't yet replaced.

I think this could happen with the module system (though the overlap is purely due to spoon's creation of synthetic packages).

I was afraid to touch it back then, but it doesn't seem particularly likely that code depends on this.

[slarse]

I was quite confused about the check as well when adjusting this class. It sounded like this was meant to merge equal packages. If I didn't miss anything, the two names can be the same at the time of the check, if you have two parallel package hierarchies.

Absolutely, the names can be the same (e.g. A = B, which is precisely the bug reported in #4764). It doesn't matter, however, because the semantics of this method is to add B as a subpackage of A. A subpackage of A cannot have the same qualified name as A, as A's qualified name is prepended to all of its supbackages. There simply cannot be a name collision; the action that this method performs makes that impossible.

It just.. doesn't make sense.

[I-Al-Istannen]

What I was trying to convey is, that you can hit this code path in existing code (e.g.):

		Launcher launcher = new Launcher();
		Factory factory = launcher.getFactory();
		CtModule mod = factory.Module().getOrCreate("second");
		CtPackage pack1 = factory.Package().getOrCreate("test", factory.Module().getUnnamedModule());
		CtPackage pack2 = factory.Package().getOrCreate("test", mod);

		pack1.addPackage(pack2);

or if you obtain a second package another way. The existing code would merge this, your code would nest them. This is an observable semantic difference. I just wanted to point out, that this can happen and is not impossible and why I left that code in there when refactoring the ModelSet.

I do agree though, that the new semantics are likely closer to what the user intended and probably won't do any harm. You just might break something, as you change observable behavior.

[slarse]

Ah, yes I totally get that, I didn't mean that the code path couldn't be hit. It's the source of the bug. The removal of this code path is the bug fix. It's unarguably a bug, because at this time it isn't possible to create packages on the from A.A, because when you add A as a subpackage of A the (now removed) code path just merges them.

[slarse]

For clarity, if you run Launcher().getFactory().Package().getOrCreate("A.A"), you get back a package with the qualified name A (before removal of the discussed code path).

[I-Al-Istannen]

Yea, I just wanted to make sure that you are aware of the ways this path could be hit in the wild by two valid hierarchies in e.g. different modules coinciding instead of just because they have the same simple name.

Sounds like I might have misunderstood your position then. Sorry about that :)

[slarse]

Oh no need to apologize, I appreciate your attention to detail :)

[MartinWitt]

Could you add either a comment or the GitHub issue linking to the issue of the bug? This could help to understand test cases in the future. Otherwise LGTM.

[slarse]

Good catch, I routinely forget that we have that new annotation. Fixed.

[MartinWitt]

Thanks @slarse