Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 9, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75f3ef1
Fix bug in creating package with repeated simple names
slarse Jun 27, 2022
4ee020a
Replace Test annotation with GithubIssue
slarse Jul 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions src/main/java/spoon/support/reflect/declaration/CtPackageImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,7 @@ protected CtRole getRole() {

@Override
public CtPackage put(String simpleName, CtPackage pack) {
if (pack == null) {
return null;
}
// they are the same
if (CtPackageImpl.this.getQualifiedName().equals(pack.getQualifiedName())) {
addAllTypes(pack, CtPackageImpl.this);
addAllPackages(pack, CtPackageImpl.this);
Comment on lines -51 to -54
Copy link
Collaborator Author

@slarse slarse Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (pack == null || pack == CtPackageImpl.this) {
return null;
}

Expand Down
31 changes: 31 additions & 0 deletions src/test/java/spoon/reflect/factory/PackageFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package spoon.reflect.factory;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.sameInstance;
import static org.hamcrest.MatcherAssert.assertThat;

import spoon.Launcher;
import spoon.reflect.declaration.CtPackage;
import spoon.test.GitHubIssue;

class PackageFactoryTest {

@GitHubIssue(issueNumber = 4764, fixed = true)
void getOrCreate_returnsNestedPackageStructure_whenQualifiedNameRepeatsSimpleName() {
// contract: A qualified name that is simply repetitions of a single simple name results in the expected
// nested package structure

PackageFactory factory = new Launcher().getFactory().Package();
String topLevelPackageName = "spoon";
String nestedPackageName = topLevelPackageName + "." + topLevelPackageName;

CtPackage packageWithDuplicatedSimpleNames = factory.getOrCreate(nestedPackageName);
CtPackage topLevelPackage = factory.get(topLevelPackageName);

assertThat(topLevelPackage.getQualifiedName(), equalTo(topLevelPackageName));
assertThat(packageWithDuplicatedSimpleNames.getQualifiedName(), equalTo(nestedPackageName));

assertThat(topLevelPackage.getPackage(topLevelPackageName), sameInstance(packageWithDuplicatedSimpleNames));
assertThat(packageWithDuplicatedSimpleNames.getParent(), sameInstance(topLevelPackage));
}
}