From 55c5001ac75067488684ab0f4a19fe1f8b76a28f Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Fri, 26 Sep 2025 18:21:29 +0200 Subject: [PATCH] sort infos more correctly to guarantee stable order (found by IJPL-209403 Highlighting property tests) the previous order had several problems 1) was not stable enough, so similar infos were in random order, which led to duplicate highlighters 2) on highlighter invalidation the order could change, which led to mergeSortedLists() incorrect merging, meaning removeAtomically() was unable to remove info from data, meaning stray infos staying cached GitOrigin-RevId: 8857b3a81459c670e4a197b06c55daa84614c610 --- .../daemon/impl/HighlightInfoUpdaterImpl.java | 95 ++++++++++--------- 1 file changed, 49 insertions(+), 46 deletions(-) diff --git a/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightInfoUpdaterImpl.java b/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightInfoUpdaterImpl.java index b09e16f06b29..21b2f19d427d 100644 --- a/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightInfoUpdaterImpl.java +++ b/platform/analysis-impl/src/com/intellij/codeInsight/daemon/impl/HighlightInfoUpdaterImpl.java @@ -47,11 +47,9 @@ import com.intellij.psi.util.PsiUtilCore; import com.intellij.util.ArrayUtil; import com.intellij.util.ConcurrencyUtil; import com.intellij.util.ExceptionUtil; -import com.intellij.util.ObjectUtils; import com.intellij.util.concurrency.AppExecutorUtil; import com.intellij.util.containers.CollectionFactory; import com.intellij.util.containers.ContainerUtil; -import com.intellij.util.containers.FreezableArrayList; import com.intellij.util.containers.HashingStrategy; import it.unimi.dsi.fastutil.longs.Long2ObjectMap; import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap; @@ -274,12 +272,30 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme return "; progress=" + (indicator == original ? "" : "wrapped:")+ (indicator == null ? "null\n" + ExceptionUtil.getThrowableText(new Throwable()) : indicator); } + private static int compareNaturalNullable(@Nullable String o1, @Nullable String o2) { + //noinspection StringEquality + return o1 == o2 ? 0 : o1 == null ? -1 : o2 == null ? 1 : o1.compareTo(o2); + } + private static final Comparator BY_OFFSETS_AND_HASH_ERRORS_FIRST = (o1, o2) -> { + if (o1 == o2) return 0; + int r = Segment.BY_START_OFFSET_THEN_END_OFFSET.compare(o1, o2); + if (r != 0) return r; + r = o2.getSeverity().compareTo(o1.getSeverity()); // NB: reversed - errors first + if (r != 0) return r; + r = compareNaturalNullable(o1.getDescription(), o2.getDescription()); + if (r != 0) return r; + // have to compare highlighters because we don't want to remove otherwise equal HighlightInfo except for its (recreated) highlighter + RangeHighlighterEx h1 = o1.getHighlighter(); + RangeHighlighterEx h2 = o2.getHighlighter(); + return Integer.compare(System.identityHashCode(h1), System.identityHashCode(h2)); + }; // remove `psis` from `data` in one batch for all infos in the list because there can be a lot of them - private static void removeFromDataAtomically(@NotNull Map data, - @NotNull @Unmodifiable Collection psis, - @NotNull HighlightingSession session) { - if (psis.isEmpty()) return; + // return true if was removed + private static boolean removeFromDataAtomically(@NotNull Map data, + @NotNull @Unmodifiable Collection psis, + @NotNull HighlightingSession session) { + if (psis.isEmpty()) return true; Map>> byPsiElement = new HashMap<>(); for (InvalidPsi invalidPsi : psis) { Object toolId = invalidPsi.info().toolId; @@ -288,29 +304,28 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme .computeIfAbsent(invalidPsi.psiElement(), __ -> new ArrayList<>()); infos.add(invalidPsi.info()); } + boolean removed = true; for (Map.Entry>> entry : byPsiElement.entrySet()) { Object toolId = entry.getKey(); ToolHighlights toolHighlights = data.get(toolId); - if (toolHighlights == null) continue; + if (toolHighlights == null) { + removed = false; + continue; + } Map> byPsiMap = entry.getValue(); for (Map.Entry> byPsiEntry : byPsiMap.entrySet()) { PsiElement psiElement = byPsiEntry.getKey(); - List oldInfos = ObjectUtils.notNull(toolHighlights.elementHighlights.get(psiElement), List.of()); - List toRemove = ContainerUtil.sorted(byPsiEntry.getValue(), Segment.BY_START_OFFSET_THEN_END_OFFSET); + List oldL = toolHighlights.elementHighlights.get(psiElement); + List oldInfos = oldL == null ? List.of() : ContainerUtil.sorted(oldL, BY_OFFSETS_AND_HASH_ERRORS_FIRST); // need to-resort in case the range-highlighters invalidated and offsets are skewed + List toRemove = ContainerUtil.sorted(byPsiEntry.getValue(), BY_OFFSETS_AND_HASH_ERRORS_FIRST); List resultInfos = new ArrayList<>(); - ContainerUtil.processSortedListsInOrder(oldInfos, toRemove, (o1, o2) -> { - int r = Segment.BY_START_OFFSET_THEN_END_OFFSET.compare(o1, o2); - if (r != 0) return r; - // have to compare highlighters because we don't want to remove otherwise equal HighlightInfo except for its (recreated) highlighter - RangeHighlighterEx h1 = o1.getHighlighter(); - RangeHighlighterEx h2 = o2.getHighlighter(); - return System.identityHashCode(h1)-System.identityHashCode(h2); - }, true, (info, result) -> { + ContainerUtil.processSortedListsInOrder(oldInfos, toRemove, BY_OFFSETS_AND_HASH_ERRORS_FIRST, true, (info, result) -> { if (result == ContainerUtil.MergeResult.COPIED_FROM_LIST1) { resultInfos.add(info); } // in other cases when the info is either from toRemove or both, skip it }); + removed &= oldInfos.size() - toRemove.size() == resultInfos.size(); if (resultInfos.isEmpty()) { toolHighlights.elementHighlights.remove(psiElement); } @@ -324,6 +339,7 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme } } } + return removed; } private synchronized void recycleInvalidPsiElements(@NotNull PsiFile psiFile, @@ -579,7 +595,7 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme } } - private synchronized PsiElement findPsiElement(HighlightInfo info, @NotNull PsiFile psiFile) { + private synchronized PsiElement findPsiElement(@NotNull HighlightInfo info, @NotNull PsiFile psiFile) { Map data = getData(psiFile); ToolHighlights highlights = data.get(info.toolId); for (Map.Entry> entry : highlights.elementHighlights.entrySet()) { @@ -734,7 +750,7 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme .filter(e -> isInspectionToolId(e.getKey())) // inspections only .flatMap(e -> e.getValue().elementHighlights.values().stream()) .flatMap(l->l.stream()) - .sorted(UpdateHighlightersUtil.BY_ACTUAL_START_OFFSET_NO_DUPS) + .sorted(BY_OFFSETS_AND_HASH_ERRORS_FIRST) // sort by errors first, so that when the warning appears, the overlapping errors are already in the 'overlappingIntervals' queue .toList(); SweepProcessor.Generator generator = processor -> ContainerUtil.process(sorted, processor); SeverityRegistrar severityRegistrar = SeverityRegistrar.getSeverityRegistrar(session.getProject()); @@ -1213,13 +1229,11 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme SeverityRegistrar severityRegistrar = SeverityRegistrar.getSeverityRegistrar(session.getProject()); Long2ObjectMap range2markerCache = new Long2ObjectOpenHashMap<>(10); // infos which highlighters are recycled from `invalidElementRecycler` and which need to be removed from `data` to avoid its highlighter to be registered in two places - // this list must be sorted by Segment.BY_START_OFFSET_THEN_END_OFFSET - List recycledInvalidPsiHighlightersToBeRemovedFromData = new ArrayList<>(newInfos.size()); - List sorted = ContainerUtil.sorted(newInfos, Segment.BY_START_OFFSET_THEN_END_OFFSET); - FreezableArrayList newInfosToStore = new FreezableArrayList<>(sorted.size()); + // this list must be sorted by BY_OFFSETS_AND_HASH + List newInfosToStore = new ArrayList<>(newInfos.size()); //noinspection ForLoopReplaceableByForEach - for (int i = 0; i < sorted.size(); i++) { - HighlightInfo newInfo = sorted.get(i); + for (int i = 0; i < newInfos.size(); i++) { + HighlightInfo newInfo = newInfos.get(i); assert toolId.equals(newInfo.toolId) : "HighlightInfo generated by "+toolId + "(" + toolId.getClass() + ") must have consistent toolId, but got:"+ newInfo; boolean isFileLevel = newInfo.isFileLevelAnnotation(); long finalInfoRange = isFileLevel @@ -1239,9 +1253,6 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme String from; if (recycled == null) { recycled = invalidElementRecycler.pickupHighlighterFromGarbageBin(infoStartOffset, infoEndOffset, layer, newInfo.getDescription()); - if (recycled != null) { - recycledInvalidPsiHighlightersToBeRemovedFromData.add(recycled); - } from = "invalidElementRecycler"; } else { @@ -1251,14 +1262,16 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme if (LOG.isDebugEnabled()) { LOG.debug("assignRangeHighlighters: pickedup " + recycled + " from " + from+ " "+session.getProgressIndicator()); } + // remove highlighter from the data before changing its "errorStripe" to avoid having same highlighter for different Infos + removeFromDataAtomically(data, List.of(recycled), session); } - changeRangeHighlighterAttributes(session, psiFile, markup, newInfo, range2markerCache, finalInfoRange, recycled, isFileLevel, infoStartOffset, infoEndOffset, layer, - severityRegistrar); + changeRangeHighlighterAttributes(session, psiFile, markup, newInfo, range2markerCache, finalInfoRange, recycled, isFileLevel, infoStartOffset, infoEndOffset, layer, severityRegistrar); } - // this list must be sorted by Segment.BY_START_OFFSET_THEN_END_OFFSET - for (int i = 0; i < newInfosToStore.size(); i++) { - HighlightInfo info = newInfosToStore.get(i); + List sorted = ContainerUtil.sorted(newInfosToStore, BY_OFFSETS_AND_HASH_ERRORS_FIRST); + // this list must be sorted by BY_OFFSETS_AND_HASH + for (int i = 0; i < sorted.size(); i++) { + HighlightInfo info = sorted.get(i); //todo fails because of ProblemDescriptorWithReporterName //assert toolId.equals(info.toolId) : info + "; " + toolId + "(" + toolId.getClass() + ")"; assert info.getHighlighter() != null : info; @@ -1266,11 +1279,11 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme HighlightInfo assignedInfo = HighlightInfo.fromRangeHighlighter(info.getHighlighter()); assert assignedInfo == info : "from RH: " + assignedInfo + "(" + System.identityHashCode(assignedInfo)+ "); but expected: " + info+ "(" + System.identityHashCode(info)+ ")"; if (i>0) { - assert Segment.BY_START_OFFSET_THEN_END_OFFSET.compare(newInfosToStore.get(i-1), newInfosToStore.get(i)) <= 0 : "assignRangeHighlighters returned unsorted list: "+newInfosToStore; + int compare = BY_OFFSETS_AND_HASH_ERRORS_FIRST.compare(sorted.get(i - 1), sorted.get(i)); + assert compare <= 0 : "assignRangeHighlighters returned unsorted list: " + sorted +"; "+compare; } } - removeFromDataAtomically(data, recycledInvalidPsiHighlightersToBeRemovedFromData, session); - return newInfosToStore.emptyOrFrozen(); + return sorted; } private static void changeRangeHighlighterAttributes(@NotNull HighlightingSession session, @@ -1356,14 +1369,4 @@ public final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater impleme return highlighter; } - private void assertNoInfoInMarkup(@NotNull HighlightInfo info, @NotNull MarkupModelEx markup, @NotNull ManagedHighlighterRecycler recycler, @NotNull ManagedHighlighterRecycler invalidPsiRecycler) { - if (!isAssertInvariants()) return; - if (ContainerUtil.mapNotNull(markup.getAllHighlighters(), m-> HighlightInfo.fromRangeHighlighter(m)).contains(info) - && !ContainerUtil.mapNotNull(recycler.forAllInGarbageBin(), i->i.info()).contains(info) - && !ContainerUtil.mapNotNull(invalidPsiRecycler.forAllInGarbageBin(), i->i.info()).contains(info)) { - throw new AssertionError("Info " + info + " found in markup among "+ - "\n "+StringUtil.join(ContainerUtil.mapNotNull(markup.getAllHighlighters(), m-> HighlightInfo.fromRangeHighlighter(m)), Object::toString,"\n ")+ - " --, even though these recyclers do not contain it:\n "+recycler.forAllInGarbageBin()+";\n "+invalidPsiRecycler.forAllInGarbageBin()); - } - } }