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

Guard lookup in BoundSet::allSuperPairsWithCommonGenericTypeRecursive #3411

Merged
merged 2 commits into from
Dec 8, 2024
Merged

Guard lookup in BoundSet::allSuperPairsWithCommonGenericTypeRecursive #3411

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26a1f52
Guard lookup in BoundSet::allSuperPairsWithCommonGenericTypeRecursive
fedejeanne Dec 6, 2024
92015e9
Guard lookup in BoundSet::allSuperPairsWithCommonGenericTypeRecursive
stephan-herrmann Dec 6, 2024
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
39 changes: 22 additions & 17 deletions ...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/BoundSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1264,33 +1264,38 @@ private boolean superOnlyRaw(TypeBinding g, TypeBinding s, LookupEnvironment env
}

protected List<Pair<TypeBinding>> allSuperPairsWithCommonGenericType(TypeBinding s, TypeBinding t) {
return allSuperPairsWithCommonGenericTypeRecursive(s, t, new HashSet<>());
ArrayList<Pair<TypeBinding>> result = new ArrayList<>();
allSuperPairsWithCommonGenericTypeRecursive(s, t, result, new HashSet<>());
return result;
}

private List<Pair<TypeBinding>> allSuperPairsWithCommonGenericTypeRecursive(TypeBinding s, TypeBinding t, HashSet<TypeBinding> visited) {
private void allSuperPairsWithCommonGenericTypeRecursive(TypeBinding s, TypeBinding t, List<Pair<TypeBinding>> result, HashSet<Integer> visited) {
if (s == null || s.id == TypeIds.T_JavaLangObject || t == null || t.id == TypeIds.T_JavaLangObject)
return Collections.emptyList();
if (!visited.add(s.prototype()))
return Collections.emptyList();
List<Pair<TypeBinding>> result = new ArrayList<>();
if (s.isParameterizedType() && t.isParameterizedType() // optimization #1: clients of this method only want to compare type arguments
&& TypeBinding.equalsEquals(s.original(), t.original())) {
result.add(new Pair<>(s, t));
}
return;
if (!visited.add(s.id))
return;

// optimization: nothing interesting above equal types
if (TypeBinding.equalsEquals(s, t))
return result; // optimization #2: nothing interesting above equal types
TypeBinding tSuper = t.findSuperTypeOriginatingFrom(s);
if (tSuper != null && s.isParameterizedType() && tSuper.isParameterizedType()) { // optimization #1 again
result.add(new Pair<>(s, tSuper));
return;
Copy link
Contributor

@szarnekow szarnekow Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Previously we added (s, t) before checking equalsEquals. It's no longer the same algorithm, right?

Old:

s -> List<String>
t ->  Collection<String>
s.superInterfaces recurses into visiting (Collection<String>, Collection<String>) and adds that pair

Now:

s -> List<String>
t ->  Collection<String>
s.superInterfaces recurses into visiting (Collection<String>, Collection<String>) and exits immediately

Is that intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, would it be valid to exit after adding the pair in line 1278 of the old version without relying on equalsEquals(s, t)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we added (s, t) before checking equalsEquals. It's no longer the same algorithm, right?
...
Is that intentional?

Yes, adding a pair of equal types is unnecessary. This change avoids that.

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, would it be valid to exit after adding the pair in line 1278 of the old version without relying on equalsEquals(s, t)?

That might be difficult to show. At that point we only know that generic types (original()) are equal. How the differences in type arguments percolate into super types is a non-trivial question. As ecj now is more than twice as fast than javac (in my poor-man's measurements), I'd rather not invest more in this particular code section. There are probably areas more in need of optimization.


if (s.isParameterizedType()) { // optimization here and below: clients of this method only want to compare type arguments
if (TypeBinding.equalsEquals(s.original(), t.original())) {
if (t.isParameterizedType())
result.add(new Pair<>(s, t));
} else {
TypeBinding tSuper = t.findSuperTypeOriginatingFrom(s);
if (tSuper != null && tSuper.isParameterizedType())
result.add(new Pair<>(s, tSuper));
}
}
result.addAll(allSuperPairsWithCommonGenericTypeRecursive(s.superclass(), t, visited));
allSuperPairsWithCommonGenericTypeRecursive(s.superclass(), t, result, visited);
ReferenceBinding[] superInterfaces = s.superInterfaces();
if (superInterfaces != null) {
for (ReferenceBinding superInterface : superInterfaces) {
result.addAll(allSuperPairsWithCommonGenericTypeRecursive(superInterface, t, visited));
allSuperPairsWithCommonGenericTypeRecursive(superInterface, t, result, visited);
}
}
return result;
}

public TypeBinding getEquivalentOuterVariable(InferenceVariable variable, InferenceVariable[] outerVariables) {
Expand Down
Loading