Null-Analysis - The interface ... cannot be implemented more than once with different arguments ...

[stephan-herrmann]

fixes #2158

This was tricky: the "duplicate" supertype exists in two variants that differ only in type annotations on type arguments.

@srikanth-sankaran when we introduced TypeBinding.equalsEquals() and ..notEquals(), the intention was to ignore differences only regarding annotations (by comparing id), but it seems this works only for annotations on the main type? Should types that differ only regarding an annotation on a type argument share the same id, or is that outside the contract?

Anyway, working with those variants was wrong from the outset. It happened because a ParameterizedTypeBinding was created (as a super interface binding) before the underlying generic type was fully updated (CompleteTypeBindingsSteps.INTEGRATE_ANNOTATIONS_IN_HIERARCHY) which clearly is a bug.

While further untangling the order of processing steps doesn't look very feasible, I found a way to fix this super type after the fact (during TypeDeclaration.updateSupertypesWithAnnotations(), which already exists for this kind of udpate).
This is done by (a) detecting this situation (with help of SourceTypeBinding.supertypeAnnotationsUpdated) and if necessary (b) re-create the parameterized type from the updated generic type. This should be OK, since at that point we haven't yet started referencing such types from fields and method (STB.resolveType(s)For methods).

[stephan-herrmann]

I observed something suspicious regarding type identity, indeed:

• we have a TVB
• first a PTB is created with the TVB as one argument
  • this causes creation of a PTBKey to register the PTB in TypeSystem
  • the PTBKey stores this TVB
• later annotations of that TVB are resolved
  • this triggers TypeSystem.forceRegisterAsDerived()
    • before setting annotations, the TVB is cloned and from here on the clone is used as the 'naked' type (in types[id][0])
• later during another call to getParameterizedType() we look up the unannotated type of the same TVB and in types[id][0] we find the clone
  • using the clone for a new PTBKey we do not find the previously registered types, because the clone is not == the TVB stored in the previous PTBKey.

This seems to create some schizophrenia among types that should be "mostly equal" (ignoring annotations), but get different ids stamped on.

With the proposed change, this situation no longer causes harm (no longer occurs?), but I can't predict whether/when this will cause grief again. Thus I'm thinking of a way to update the PTBKey where the TVB is stored.

[srikanth-sankaran]

fixes #2158

This was tricky: the "duplicate" supertype exists in two variants that differ only in type annotations on type arguments.

@srikanth-sankaran when we introduced TypeBinding.equalsEquals() and ..notEquals(), the intention was to ignore differences only regarding annotations (by comparing id), but it seems this works only for annotations on the main type? Should types that differ only regarding an annotation on a type argument share the same id, or is that outside the contract?

AFAICR, it should disregard all type annotations anywhere - so yes types that differ only regarding an annotation on a type argument share the same id would be my expectation. Did you check if the behavior that is being observed dates to the original implementation ??

[stephan-herrmann]

AFAICR, it should disregard all type annotations anywhere - so yes types that differ only regarding an annotation on a type argument share the same id would be my expectation.

Thanks.

Did you check if the behavior that is being observed dates to the original implementation ??

I'd say that behaviour was latent for a long time, but it requires a very specific ordering of processing steps for it to surface.

Digging into history I find these stages:

• Original code: A TVB with declaration-site annotations registered the annotated variant where TypeSystem expects the naked type.
• In https://bugs.eclipse.org/440143 I started to clone the TVB so we do have an unannotated variant.
• In https://bugs.eclipse.org/456487 I detected that the naked type ended up in the wrong slot, fixed there.
• Found during debugging for this issue: a TVB could be used in a PTBKey before all the magic of setTypeAnnotations() and forceRegisterAsDerived() happens. This key will never be matched after a truly naked type has been registered.

In commit 3929efa I addressed this by setting up a protocol for swapping the TVB not only in TypeSystem.types[id] but also in the PTBKey.

Since the reported symptom was already fixed in TypeDeclaration.updateWithAnnotations(..), I added a flag by which the test can disable that particular fix, so we can observe the effect of the secondary fix which is implemented in commit 3929efa

---

Brainstorming an alternative solution, in case the above is considered too much magic piled up: when we create a TVB we may not yet know if annotations will be applied (either explicitly, or worse: from a @NonNullByDefault which may not yet be resolved), but still we want to establish two invariants:

• the identity of the TVB will not change when annotations are applied
• we need a TVB without annotations as the main type in TypeSystem.types[id][0].

Could we resolve this by always cloning all TVB right when created? We'd put the clone in slot 0 of TypeSystem.types[id] and consider the original TVB as potentially derived.

This would introduce a lot of cloning for TVB that are never annotated. Plus: any == tests for such TVB would be at risk, since interning would no longer be perfect, we'd admit duplicate identical TVB instances.

[stephan-herrmann]

@srikanth-sankaran with all the wizardry involved, may I ask for a review?

[srikanth-sankaran]

@srikanth-sankaran with all the wizardry involved, may I ask for a review?

Sure, I have cleared up the urgent items - I will review this tomorrow. Woefully rusty on type annotation related code, but I will take look.

I demand my pound of flesh, ;-) in the form of help in moving forward with #1181 - Manoj helped with some "stress testing" on this PR and things look good to go according to him too. I was hoping to dig up the old draft specs but that pursuit has not been effective. So we will have to rely on https://bugs.eclipse.org/bugs/show_bug.cgi?id=422489#c2 and other proof my argument attempted at #1181

[stephan-herrmann]

I demand my pound of flesh, ;-) in the form of help in moving forward with #1181 - Manoj helped with some "stress testing" on this PR and things look good to go according to him too. I was hoping to dig up the old draft specs but that pursuit has not been effective. So we will have to rely on https://bugs.eclipse.org/bugs/show_bug.cgi?id=422489#c2 and other proof my argument attempted at #1181

It's a deal! :)

[srikanth-sankaran]

@srikanth-sankaran with all the wizardry involved, may I ask for a review?

Hi Stephan, I have looked at the changes, but it requires way too much context to meaningfully critique it - way too much context than can be reasonably acquired during the review of a bug fix.

I think the key to unravelling all this really lies in "untangling the order of processing steps" - You may very well be correct in asserting "further untangling the order of processing steps doesn't look very feasible," - but I would like to make a very comprehensive study of where we stand wrt to the order of processing steps particularly in relation to what javac does (which itself would need reconstructing in my mind - IIRC, javac processes supertypes in two passes, in the first pass ignoring type arguments and in the subsequent one including them, likewise annotations get queued up for processing and get taken up at a specific time)

I am observing this "order of processing steps" perhaps not being precisely nailed showing up in other places too - the permits clauses appear to be resolved at the "wrong" time - I have a set of issues relating to that under investigation. Also in the case of records, various places avoiding reentrancy give me an uneasy feeling.

It is a sizeable project to comprehensively study all this and implement an alternate pipeline if/as deemed necessary - until then point fixes of the present kind are the only option. Please proceed to merge.

I will take up this study - but hard to put a date on it. The grammar changes for lambdas is in the queue too :)

Following on the pattern (pun intended) of 4.31 where I did a comprehensive review and rewrite of (a) JEP 394: Pattern Matching for instanceof (b) JEP 441: Pattern Matching for switch (c) JEP 440: Record Patterns and (d) JEP 456: Unnamed Variables & Patterns
for 4.32, my goal is a comprehensive review and rewrite as required of (a) JEP 409 sealed classes and (b) JEP 361: Switch Expressions.

So the present topic will have to wait until after this. But we can reopen the discussions and brainstorming at that point - what do you say ?

[srikanth-sankaran]

Also, when we open it to consider alternative approaches - we may want to evaluate whether it makes sense to come up with an implementation where org.eclipse.jdt.internal.compiler.lookup.TypeBinding.equalsEquals(TypeBinding, TypeBinding) will use org.eclipse.jdt.internal.compiler.lookup.TypeBinding.id comparisons only as a fast path i.e., if id's are == the underlying type is the same, but if id's are different, you need a slower path that strips type annotations and compares rather than just saying false.

That could allow for some "fault tolerance" so to speak and by taking away the current hard guarantee about underlying type identity that TypeSystem attempts to provide avoid situations of the present kind.

[srikanth-sankaran]

Also, when we open it to consider alternative approaches - we may want to evaluate whether it makes sense to come up with an implementation where org.eclipse.jdt.internal.compiler.lookup.TypeBinding.equalsEquals(TypeBinding, TypeBinding) will use org.eclipse.jdt.internal.compiler.lookup.TypeBinding.id comparisons only as a fast path i.e., if id's are == the underlying type is the same, but if id's are different, you need a slower path that strips type annotations and compares rather than just saying false.

That could allow for some "fault tolerance" so to speak and by taking away the current hard guarantee about underlying type identity that TypeSystem attempts to provide avoid situations of the present kind.

While a comprehensive study of the "order of processing steps" and implementation of an alternate pipeline is a sizeable project, do you think using org.eclipse.jdt.internal.compiler.lookup.TypeBinding.id comparisons in org.eclipse.jdt.internal.compiler.lookup.TypeBinding.equalsEquals(TypeBinding, TypeBinding)` only as a fast path and falling back on a backup path that implements a TypeVisitor of sorts structurally comparing two types ignoring type annotations is something that would help ? (and would you want to do it for this problem or in general keep as a future solution)

[srikanth-sankaran]

Also, when we open it to consider alternative approaches - we may want to evaluate whether it makes sense to come up with an implementation where org.eclipse.jdt.internal.compiler.lookup.TypeBinding.equalsEquals(TypeBinding, TypeBinding) will use org.eclipse.jdt.internal.compiler.lookup.TypeBinding.id comparisons only as a fast path i.e., if id's are == the underlying type is the same, but if id's are different, you need a slower path that strips type annotations and compares rather than just saying false.
That could allow for some "fault tolerance" so to speak and by taking away the current hard guarantee about underlying type identity that TypeSystem attempts to provide avoid situations of the present kind.

While a comprehensive study of the "order of processing steps" and implementation of an alternate pipeline is a sizeable project, do you think using org.eclipse.jdt.internal.compiler.lookup.TypeBinding.id comparisons in org.eclipse.jdt.internal.compiler.lookup.TypeBinding.equalsEquals(TypeBinding, TypeBinding)` only as a fast path and falling back on a backup path that implements a TypeVisitor of sorts structurally comparing two types ignoring type annotations is something that would help ? (and would you want to do it for this problem or in general keep as a future solution)

What would this buy us ? TypeSystem can undertake reasonable efforts to dole out unique id stamps for variants but doesn't have to bend over backwards to do so. And where it fails to stamp unique id stamps, we DON'T need various other parts of the compiler implementing their own "patch up's" for failings of the TypeSystem to standby its guarantee.

I posit the slow path need not actually be slow - structural comparisons between wildly different types would fail fast I think.

[srikanth-sankaran]

If such a change to org.eclipse.jdt.internal.compiler.lookup.TypeBinding.equalsEquals(TypeBinding, TypeBinding) were feasible and implemented would any of the past fixes to annotation handling get simplified ?

[stephan-herrmann]

As for the ordering of processing steps:

Originally connectTypeHierarchy was just a single phase, covering all of

• supertypes
• type parameters
• annotations

Only bound check during TypeReference.resolveType() has an option to defer checking (Scope.deferBoundCheck()), so since https://bugs.eclipse.org/bugs/show_bug.cgi?id=159851 (2009) the team was aware, that some actions could be triggered before required information is available.

In the context of type annotations we received various bug reports, that boiled down to the fact, that evaluating annotations right during connectTypeHierarchy can have ill effects. Initially we tried to handle individual bug patterns with more deferring (like https://bugs.eclipse.org/bugs/show_bug.cgi?id=434570) and more on-demand evaluation (like #630), but obviously this didn't produce a clean solution.

For that reason I gradually disentangled that aspect from connectTypeHierarchy (centrally: #1077 with follow-ups).

The net effect is that now we have one more top-level phase, and a somewhat more systematic approach to technically handle those phases that are driven by LookupEnvironment (see LookupEnvironment.CompleteTypeBindingsSteps).

The idea of phase INTEGRATE_ANNOTATIONS_IN_HIERARCHY is: after super types and their type parameters have been properly resolved, resolving annotations should not cause any harm. But since supertype bindings (possibly parameterized) already exist at that point in time, the new phase needs to take a deep dive into all these bindings and update them with annotations, where appropriate. That's already a lot of traversals, but the idea is: it should be enough to traverse supertypes, since types in signatures have not yet been resolved at that time (resolveTypesFor()).

I believe this is already significantly safer than the previous state, but surely discussing if ecj's phase-model is overly simplistic is a worthy topic. I see only one downside in introducing more phases: for every type that is incrementally attributed (with type parameters, annotations ...) it looks very tricky, into which derived bindings (in particular PTB) do we need to propagate all late added information?

As for the current bug I should note, that the conflict did not originate from any resolving happening too early or too late, but only from the internal structure PTBKey.

[stephan-herrmann]

It seems like https://bugs.eclipse.org/bugs/show_bug.cgi?id=418041 is also already relevant for our discussion.

[srikanth-sankaran]

As for the ordering of processing steps:

Thanks, this is very useful!

I have created #2209 and assigned it to myself.

[stephan-herrmann]

Also, when we open it to consider alternative approaches - we may want to evaluate whether it makes sense to come up with an implementation where org.eclipse.jdt.internal.compiler.lookup.TypeBinding.equalsEquals(TypeBinding, TypeBinding) will use org.eclipse.jdt.internal.compiler.lookup.TypeBinding.id comparisons only as a fast path i.e., if id's are == the underlying type is the same, but if id's are different, you need a slower path that strips type annotations and compares rather than just saying false.

That could allow for some "fault tolerance" so to speak and by taking away the current hard guarantee about underlying type identity that TypeSystem attempts to provide avoid situations of the present kind.

Thanks, this made me rethink our original invariants about type identity, derived types, prototypes etc. So I wrote a little essay as #2210 - to be picked up at our leisure.

[stephan-herrmann]

Hi Stephan, I have looked at the changes, but it requires way too much context to meaningfully critique it - way too much context than can be reasonably acquired during the review of a bug fix.

The interesting discussions resulting from your comments prove the opposite: this was indeed meaningful critique 😄 Thanks!

To be continued in #2209 and #2210 ...

Meanwhile I merged the current bug fix as the best thing we have today - if some day we come up with a more consistent story, I'll be happy to remove this and previous situational fixes.