Code cleanup in AnnotationModel, AnnotationMap, Javadoc inherit adjustment #3399

mehmet-karaman · 2025-10-13T09:44:40Z

split of the PR: Fix for multi threading issues in AnnotationMap #3397
Removed dead code in AnnotationModel.cleanup because mapLock can't be null.
Added new javadoc to IAnnotationMap.getLockObject()
added assert.isLegal check to AnnotationMAp.setLockObject()

mehmet-karaman · 2025-10-13T09:45:01Z

@laeubi @iloveeclipse this is the first split of the original PR.

iloveeclipse · 2025-10-13T09:47:08Z

Mehmet, please don't hesitate to link to original tickets or PR's. It is always an extra work to find out what is related.
In current case: original PR is #3397

laeubi

I miss a reasoning why we can change the API contract of these methods, it looks like a API breaking change.

Also commit message should clearly state that e.g.

Require non null LockObject in IAnnotationMap

- describe the reasoning here -

this is obviously not a cleanup / javadoc only thing!

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap.java

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java

github-actions · 2025-10-13T10:39:51Z

Test Results

3 018 files ±0 3 018 suites ±0 2h 25m 11s ⏱️ - 13m 48s
8 226 tests ±0 7 977 ✅ +1 249 💤 ±0 0 ❌ - 1
23 598 runs ±0 22 804 ✅ +1 794 💤 ±0 0 ❌ - 1

Results for commit 67ea81a. ± Comparison against base commit c521dfc.

♻️ This comment has been updated with latest results.

laeubi · 2025-10-13T15:27:35Z

In general I'm not sure it pays of to change anything here. Even if I can see that removing some dead code might be a good thing I wonder if it is worth the hassle here if it is "really dead".

In contrast we introduce some asymmetry here to other places that use the ISycnronizer API... so I'm not totally against this but would propose to simply leave it as is.

iloveeclipse · 2025-10-13T15:39:58Z

The point is: for the reader of the IAnnotationModel & AnnotationModel API's it is not clear at all what happens here. Why there is one piece of code checking lock for null andthe rest of the code not, when the ISynchronizable.getLockObject() explicitly says it can be null. Is all the code buggy? Surely not, something seem to work somehow. Can the new code rely on lock being not null or not? No idea.

So this PR clarifies that mess and helps the reader of next one (#3397) to assume we are always working with non null lock object.

iloveeclipse · 2025-10-13T15:45:14Z

I'm not totally against this but would propose to simply leave it as is.

Is there anything in this PRT to be changed? If yes, please point which exact changes are needed so we can continue.

Otherwise remove the negative review, because from our understanding this PR makes sense in the context of the next one.

laeubi · 2025-10-13T15:48:34Z

he point is: for the reader of the IAnnotationModel & AnnotationModel API's it is not clear at all what happens here. Why there is one piece of code checking lock for null andthe rest of the code not

This is I think what is mainly a confusing of different levels of code and it is claimed without a proof.

e.g. I have checked that all public access to IAnnotationModel is doing proper null checks... AnnotationModel is not API and do not do null checks because its implementation ensures null is never returned.

This is completely common way and I do not see why implementation details need to show up in the public API ... so this is my primary concern.

iloveeclipse · 2025-10-13T15:50:35Z

AnnotationModel is not API

Why that?

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java

mickaelistria · 2025-10-13T16:50:54Z

AnnotationModel is not API

Why that?

It's package-visible only.

iloveeclipse · 2025-10-13T16:55:51Z

AnnotationModel is not API

Why that?

It's package-visible only.

public class AnnotationModel

mickaelistria · 2025-10-13T16:59:26Z

My bad, I had (mis)read AnnotationMap...

iloveeclipse · 2025-10-14T13:10:05Z

This is I think what is mainly a confusing of different levels of code and it is claimed without a proof.

The proof is the current code in the AnnotationModel class that dereferences the "possible null" lock objects 10 times out of 11 without null check.

e.g. I have checked that all public access to IAnnotationModel is doing proper null checks...

You have most likely checked ISynchronizable.getLockObject() callers, not AnnotationModel.getLockObject() callers.
This calls aren't doing null checks:

org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.CompilationUnitAnnotationModel.addAnnotation(Annotation, Position, boolean)
org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.CompilationUnitAnnotationModel.getJavaMarkerAnnotations(Position)
org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.CompilationUnitAnnotationModel.internalBeginReporting(boolean)
org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.CompilationUnitAnnotationModel.internalEndReporting(ProblemRequestorState)
org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.CompilationUnitAnnotationModel.removeAllAnnotations(boolean)
org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.CompilationUnitAnnotationModel.removeAnnotation(Annotation, boolean)
org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.CompilationUnitAnnotationModel.reportProblems(List<IProblem>)
org.eclipse.pde.internal.ui.editor.text.XMLReconcilingStrategy.SpellingProblemCollector.endCollecting()
org.eclipse.ui.texteditor.spelling.SpellingReconcileStrategy.SpellingProblemCollector.endCollecting()
org.eclipse.cdt.internal.ui.editor.CDocumentProvider.TranslationUnitAnnotationModel.addAnnotation(Annotation, Position, boolean)
org.eclipse.cdt.dsf.debug.internal.ui.disassembly.model.BreakpointsAnnotationModel.breakpointChanged(IBreakpoint, IMarkerDelta)
org.eclipse.cdt.internal.ui.editor.CDocumentProvider.TranslationUnitAnnotationModel.getAnnotations(Position)
org.eclipse.cdt.internal.ui.editor.CDocumentProvider.TranslationUnitAnnotationModel.internalBeginReporting(boolean)
org.eclipse.cdt.internal.ui.editor.CDocumentProvider.TranslationUnitAnnotationModel.internalEndReporting(ProblemRequestorState)
org.eclipse.xtext.ui.editor.quickfix.XtextResourceMarkerAnnotationModel.queueAnnotationChanged(Annotation)
org.eclipse.cdt.internal.ui.editor.CDocumentProvider.TranslationUnitAnnotationModel.removeAllAnnotations(boolean)
org.eclipse.cdt.internal.ui.editor.CDocumentProvider.TranslationUnitAnnotationModel.removeAnnotation(Annotation, boolean)
org.eclipse.cdt.internal.ui.editor.CDocumentProvider.TranslationUnitAnnotationModel.reportProblems(List<IProblem>)

AnnotationModel is not API and do not do null checks because its implementation ensures null is never returned.

It is API and it should do null checks on IAnnotationMap getAnnotationMap() return value because it could be overridden, but it never did.

OK, I will push a PR update that will:

log warning instead of assert
Improve javadoc of the IAnnotationModel.getLockObject() method to mention that changing the lock object after setting it is not expected and mght lead to synchronization problems
Add javadoc to AnnotationModel.getLockObject() and AnnotationModel.setLockObject() methods matching the javadoc of IAnnotationModel.getLockObject().
Rebase on master
Update commit message

iloveeclipse · 2025-10-15T13:38:49Z

@laeubi , @mickaelistria : could you please re-review?

iloveeclipse · 2025-10-16T15:00:35Z

@laeubi , @mickaelistria : could you please re-review?

Ping.

laeubi

Give the limited scope of this it looks "safe" to do, I added some style comments that might or might not be considered.

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap.java

- Clarified contract of IAnnotationMap.getLockObject() and setLockObject(). - Clarified contract of AnnotationModel.getLockObject() and setLockObject(). - Added warning in AnnotationMap.setLockObject() if the lock is changed - Removed dead code in AnnotationModel.cleanup because mapLock can't be null. Co-authored-by: Andrey Loskutov <[email protected]>

laeubi requested changes Oct 13, 2025

View reviewed changes

mehmet-karaman force-pushed the code_cleanup_and_javadoc_adjustment branch from 88572d3 to 82ccd1e Compare October 13, 2025 11:20

mickaelistria reviewed Oct 13, 2025

View reviewed changes

iloveeclipse force-pushed the code_cleanup_and_javadoc_adjustment branch from 82ccd1e to 8050bb4 Compare October 14, 2025 13:10

iloveeclipse requested review from laeubi and mickaelistria October 14, 2025 13:11

laeubi approved these changes Oct 16, 2025

View reviewed changes

iloveeclipse force-pushed the code_cleanup_and_javadoc_adjustment branch from 8050bb4 to 67ea81a Compare October 16, 2025 15:30

iloveeclipse approved these changes Oct 16, 2025

View reviewed changes

iloveeclipse mentioned this pull request Oct 16, 2025

Fix for multi threading issues in AnnotationMap #3397

Open

iloveeclipse merged commit 4f771f0 into eclipse-platform:master Oct 16, 2025
18 checks passed

Code cleanup in AnnotationModel, AnnotationMap, Javadoc inherit adjustment #3399

Code cleanup in AnnotationModel, AnnotationMap, Javadoc inherit adjustment #3399

Conversation

mehmet-karaman commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mehmet-karaman commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

laeubi commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

laeubi commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mickaelistria commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

mickaelistria commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 14, 2025

Uh oh!

iloveeclipse commented Oct 15, 2025

Uh oh!

iloveeclipse commented Oct 16, 2025

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mehmet-karaman commented Oct 13, 2025 •

edited

Loading

github-actions bot commented Oct 13, 2025 •

edited

Loading

mickaelistria commented Oct 13, 2025 •

edited

Loading