Comparator issues in I20240918-0950

[akurtakov]

Build https://download.eclipse.org/eclipse/downloads/drops4/I20240918-0950/ is unstable.
Comparator log: https://download.eclipse.org/eclipse/downloads/drops4/I20240918-0950/buildlogs/comparatorlogs/buildtimeComparatorUnanticipated.log.txt
Comparator zip: https://download.eclipse.org/eclipse/downloads/drops4/I20240918-0950/buildlogs/comparatorlogs/artifactcomparisons.zip

[akurtakov]

@jarthana @mpalat Please inspect whether the changes are wanted.

[akurtakov]

What I see in the zip is extra CHECKCAST instructions.

[jarthana]

Will take a look tomorrow. Also copying @stephan-herrmann

[stephan-herrmann]

Looking at AnnotationCodeMiningProvider we are talking about this code:

		if (getAdapter(IAnnotationAccess.class) instanceof IAnnotationAccessExtension annotationAccessExtension) {
			this.annotationAccess= annotationAccessExtension;
		} else {
			throw new IllegalStateException("annotationAccess must implement IAnnotationAccessExtension"); //$NON-NLS-1$
		}

Here getAdapter(Class<T>) is declared to return <T>. With T=IAnnotationAccess a checkcast to the statically known type is correct. I only wonder, if previously implementation of instanceof patterns took care to avoid unnecessary casts, when the only use of that value would undergo a second checkcast immediately after. @mpalat @srikanth-sankaran was any such optimization intended at some point?

[stephan-herrmann]

Essentially the same story in p2's ValidationDialogServiceUI.OriginTreeNode:

		public <T> T getAdapter(Class<T> adapter) {
			if (adapter == X509Certificate.class && !certificates.isEmpty()) {
				if (certificates.get(0) instanceof X509Certificate certificate) {
					return adapter.cast(certificate);
				}
			}

The result of certificates.get(0) gets casted twice.

[mpalat]

Looking at AnnotationCodeMiningProvider we are talking about this code:

		if (getAdapter(IAnnotationAccess.class) instanceof IAnnotationAccessExtension annotationAccessExtension) {
			this.annotationAccess= annotationAccessExtension;
		} else {
			throw new IllegalStateException("annotationAccess must implement IAnnotationAccessExtension"); //$NON-NLS-1$
		}

Here getAdapter(Class<T>) is declared to return <T>. With T=IAnnotationAccess a checkcast to the statically known type is correct. I only wonder, if previously implementation of instanceof patterns took care to avoid unnecessary casts, when the only use of that value would undergo a second checkcast immediately after. @mpalat @srikanth-sankaran was any such optimization intended at some point?

Thanks @stephan-herrmann for the analysis - agree that the newly introduced cast (the first cast after invoke is necessary - Commit 213967c106f77c8baf689ff315bc59bf5094f2bc that sets the valueCast via computeConversion() which causes the cast). And no, there was no such check previously - Am good with the newly generated code - probably an issue filed to see whether we can avoid duplicated cast.
updating to the new baseline eclipse-jdt/eclipse.jdt.core#2984

[jarthana]

Manoj, is this final? If so, we can touch the affected bundles (org.eclipse.ui.editors and org.eclipse.equinox.p2.ui) and respin. I don't think the changes to the jdt.core bundles are necessary.

[laeubi]

Essentially the same story in p2's ValidationDialogServiceUI.OriginTreeNode:
The result of certificates.get(0) gets casted twice.

I always wondered what is the best approach here to make the compiler "happy"... due to the first check if (adapter == SomeType.class) we "know" that now in this block <T> must be a subtype of SomeType, but the returning something that implements SomeType is not allowed without an explicit (T)objectImplementingSomeType or the adapter.cast(objectImplementingSomeType) both look a bit redundant....

[mpalat]

Manoj, is this final? If so, we can touch the affected bundles (org.eclipse.ui.editors and org.eclipse.equinox.p2.ui) and respin. I don't think the changes to the jdt.core bundles are necessary.

yes - this is final. We can take a look via a separate issue - eclipse-jdt/eclipse.jdt.core#2992
ah yes, the jdt.core bundles touching was not required. [[sorry - was in a whole day meeting - just got to check the updates before closing for the day]@akurtakov - can you please touch the other two bundles if it not too late for a Friday evening request?

[akurtakov]

I would not be able to do that. It would have to wait for Tue as it's long weekend in Bulgaria or someone else does it before that.

[mpalat]

I would not be able to do that. It would have to wait for Tue as it's long weekend in Bulgaria or someone else does it before that.

ok. no issues. will check - @MohananRahul if you see this can you touch

[merks]

@mpalat @MohananRahul

I believe the above two PR touch the affected bundles, correct? I didn't overlook something?

[mpalat]

@mpalat @MohananRahul

I believe the above two PR touch the affected bundles, correct? .

Yes - thank you @merks

[merks]

The verification build for eclipse-platform/eclipse.platform.ui#2311 appears to stall.

[merks]

Much later, still stuck here:

I'm sure it will timeout again.

[stephan-herrmann]

Essentially the same story in p2's ValidationDialogServiceUI.OriginTreeNode:
The result of certificates.get(0) gets casted twice.

I always wondered what is the best approach here to make the compiler "happy"... due to the first check if (adapter == SomeType.class) we "know" that now in this block <T> must be a subtype of SomeType, but the returning something that implements SomeType is not allowed without an explicit (T)objectImplementingSomeType or the adapter.cast(objectImplementingSomeType) both look a bit redundant....

FWIW I wasn't talking about the explicit adapter.cast(certificate); (which is fine), but about two checkcast instructions generated by the compiler, which are not visible in the source code :)

[merks]

FYI. I replayed the pr verification build with tycho.version 4.0.8. My impression is that API baseline checks hang or fail with 4.0.9. There are logged messages about unsaved workspace changes with 4.0.9 during this check.

[merks]

Failed with 403 errors. Try again.

[merks]

Through some miracle the build completed. I will make the optimistic assumption that this issue is resolved.