From 988316f910cdb9271efc482f2150d0e47afbfdfd Mon Sep 17 00:00:00 2001 From: Jaroslav Tulach Date: Tue, 5 Nov 2024 05:21:10 +0100 Subject: [PATCH] Make Graph.nextId() private (#11486) --- .../pass/analyse/PassPersistance.java | 2 - .../analyse/alias/graph/GraphOccurrence.java | 12 ++-- .../compiler/pass/analyse/AliasAnalysis.scala | 61 +++++++--------- .../pass/analyse/alias/graph/Graph.scala | 25 ++++++- .../pass/analyse/test/AliasAnalysisTest.scala | 71 +++++++------------ .../persist/impl/PersistableProcessor.java | 18 +++-- 6 files changed, 92 insertions(+), 97 deletions(-) diff --git a/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java b/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java index b5ded69b40..8d758ce72e 100644 --- a/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java +++ b/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java @@ -61,8 +61,6 @@ import scala.Tuple2$; @Persistable(clazz = AliasMetadata.Occurrence.class, id = 1261, allowInlining = false) @Persistable(clazz = AliasMetadata.RootScope.class, id = 1262, allowInlining = false) @Persistable(clazz = AliasMetadata.ChildScope.class, id = 1263, allowInlining = false) -@Persistable(clazz = GraphOccurrence.Use.class, id = 1264, allowInlining = false) -@Persistable(clazz = GraphOccurrence.Def.class, id = 1265, allowInlining = false) @Persistable(clazz = Graph.Link.class, id = 1266, allowInlining = false) @Persistable(clazz = TypeInference.class, id = 1280) @Persistable(clazz = FramePointerAnalysis$.class, id = 1281) diff --git a/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/alias/graph/GraphOccurrence.java b/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/alias/graph/GraphOccurrence.java index 3d47576596..ecbf61953d 100644 --- a/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/alias/graph/GraphOccurrence.java +++ b/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/alias/graph/GraphOccurrence.java @@ -3,6 +3,7 @@ package org.enso.compiler.pass.analyse.alias.graph; import java.util.UUID; import org.enso.compiler.core.ExternalID; import org.enso.compiler.core.Identifier; +import org.enso.persist.Persistable; /** * An occurrence of a given symbol in the aliasing graph. Note that this is not present in the @@ -14,6 +15,7 @@ public sealed interface GraphOccurrence permits GraphOccurrence.Def, GraphOccurr public abstract String symbol(); /** The definition of a symbol in the aliasing graph. */ + @Persistable(id = 1265, allowInlining = false) public static final class Def implements GraphOccurrence { private final int id; private final String symbol; @@ -30,8 +32,7 @@ public sealed interface GraphOccurrence permits GraphOccurrence.Def, GraphOccurr * @param externalId the external identifier for the IR node defining the symbol * @param isLazy whether or not the symbol is defined as lazy */ - public Def( - int id, String symbol, UUID identifier, scala.Option externalId, boolean isLazy) { + Def(int id, String symbol, UUID identifier, scala.Option externalId, boolean isLazy) { this.id = id; this.externalId = externalId.nonEmpty() ? externalId.get() : null; this.identifier = identifier; @@ -39,10 +40,6 @@ public sealed interface GraphOccurrence permits GraphOccurrence.Def, GraphOccurr this.symbol = symbol; } - public Def(int id, String symbol, UUID identifier, scala.Option externalId) { - this(id, symbol, identifier, externalId, false); - } - @Override public int id() { return this.id; @@ -77,6 +74,7 @@ public sealed interface GraphOccurrence permits GraphOccurrence.Def, GraphOccurr } /** A usage of a symbol in the aliasing graph */ + @Persistable(id = 1264, allowInlining = false) public static final class Use implements GraphOccurrence { private final int id; private final String symbol; @@ -94,7 +92,7 @@ public sealed interface GraphOccurrence permits GraphOccurrence.Def, GraphOccurr * @param identifier the identifier of the symbol * @param externalId the external identifier for the IR node defining the symbol */ - public Use(int id, String symbol, UUID identifier, scala.Option externalId) { + Use(int id, String symbol, UUID identifier, scala.Option externalId) { this.id = id; this.symbol = symbol; this.externalId = externalId.nonEmpty() ? externalId.get() : null; diff --git a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala index ff4bec1cdb..93195fff35 100644 --- a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala +++ b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala @@ -420,9 +420,7 @@ case object AliasAnalysis extends IRPass { case Expression.Block(_, _, _, isSuspended, _) => isSuspended case _ => false } - val occurrenceId = graph.nextId() - val occurrence = new GraphOccurrence.Def( - occurrenceId, + val occurrence = graph.newDef( name.name, binding.getId(), binding.getExternalId, @@ -443,7 +441,7 @@ case object AliasAnalysis extends IRPass { .updateMetadata( new MetadataPair( this, - alias.AliasMetadata.Occurrence(graph, occurrenceId) + alias.AliasMetadata.Occurrence(graph, occurrence.id) ) ) } else { @@ -487,9 +485,7 @@ case object AliasAnalysis extends IRPass { case _ => parentScope.addChild() } - val labelId = graph.nextId() - val definition = new GraphOccurrence.Def( - labelId, + val definition = graph.newDef( label.name, label.getId, label.getExternalId @@ -505,7 +501,7 @@ case object AliasAnalysis extends IRPass { .updateMetadata( new MetadataPair( this, - alias.AliasMetadata.Occurrence(graph, labelId) + alias.AliasMetadata.Occurrence(graph, definition.id) ) ) case x => @@ -545,9 +541,7 @@ case object AliasAnalysis extends IRPass { ) => // Synthetic `self` must not be added to the scope, but it has to be added as a // definition for frame index metadata - val occurrenceId = graph.nextId() - val definition = new GraphOccurrence.Def( - occurrenceId, + val definition = graph.newDef( selfName.name, arg.getId(), arg.getExternalId @@ -557,7 +551,7 @@ case object AliasAnalysis extends IRPass { .updateMetadata( new MetadataPair( this, - alias.AliasMetadata.Occurrence(graph, occurrenceId) + alias.AliasMetadata.Occurrence(graph, definition.id) ) ) .copy( @@ -584,9 +578,7 @@ case object AliasAnalysis extends IRPass { analyseExpression(ir, graph, argScope) ) - val occurrenceId = graph.nextId() - val definition = new GraphOccurrence.Def( - occurrenceId, + val definition = graph.newDef( name.name, arg.getId(), arg.getExternalId, @@ -604,7 +596,7 @@ case object AliasAnalysis extends IRPass { .updateMetadata( new MetadataPair( this, - alias.AliasMetadata.Occurrence(graph, occurrenceId) + alias.AliasMetadata.Occurrence(graph, definition.id) ) ) } else { @@ -759,30 +751,29 @@ case object AliasAnalysis extends IRPass { graph: Graph, parentScope: Scope ): Name = { - val occurrenceId = graph.nextId() - - if (isInPatternContext && !isConstructorNameInPatternContext) { - val definition = new GraphOccurrence.Def( - occurrenceId, - name.name, - name.getId, - name.getExternalId - ) - parentScope.add(definition) - parentScope.addDefinition(definition) - } else { - val occurrence = - new GraphOccurrence.Use( - occurrenceId, + val occurrenceId = + if (isInPatternContext && !isConstructorNameInPatternContext) { + val definition = graph.newDef( name.name, name.getId, name.getExternalId ) - parentScope.add(occurrence) - if (!isConstructorNameInPatternContext && !name.isMethod) { - graph.resolveLocalUsage(occurrence) + parentScope.add(definition) + parentScope.addDefinition(definition) + definition.id + } else { + val occurrence = + graph.newUse( + name.name, + name.getId, + name.getExternalId + ) + parentScope.add(occurrence) + if (!isConstructorNameInPatternContext && !name.isMethod) { + graph.resolveLocalUsage(occurrence) + } + occurrence.id } - } name.updateMetadata( new MetadataPair( this, diff --git a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/graph/Graph.scala b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/graph/Graph.scala index fe7bf907f5..39bd5e31c1 100644 --- a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/graph/Graph.scala +++ b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/graph/Graph.scala @@ -26,7 +26,7 @@ sealed class Graph( /** @return the next counter value */ - def nextIdCounter: Int = _nextIdCounter + private[graph] def nextIdCounter: Int = _nextIdCounter /** @return a deep structural copy of `this` */ final def deepCopy( @@ -80,7 +80,7 @@ sealed class Graph( * * @return a unique identifier for this graph */ - final def nextId(): Graph.Id = { + private def nextId(): Graph.Id = { val nextId = _nextIdCounter if (nextId < 0) { throw new IllegalStateException("Cannot emit new IDs. Frozen!") @@ -89,6 +89,27 @@ sealed class Graph( nextId } + /** Factory method to create new [GraphOccurrence.Def]. + */ + final def newDef( + symbol: String, + identifier: java.util.UUID, + externalId: Option[java.util.UUID], + suspended: Boolean = false + ): GraphOccurrence.Def = { + new GraphOccurrence.Def(nextId(), symbol, identifier, externalId, suspended) + } + + /** Factory method to create new [GraphOccurrence.Use]. + */ + final def newUse( + symbol: String, + identifier: java.util.UUID, + externalId: Option[java.util.UUID] + ): GraphOccurrence.Use = { + new GraphOccurrence.Use(nextId(), symbol, identifier, externalId) + } + /** Resolves any links for the given usage of a symbol, assuming the symbol * is a local variable. * diff --git a/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/pass/analyse/test/AliasAnalysisTest.scala b/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/pass/analyse/test/AliasAnalysisTest.scala index b11103ede1..f66f3fc1d7 100644 --- a/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/pass/analyse/test/AliasAnalysisTest.scala +++ b/engine/runtime-integration-tests/src/test/scala/org/enso/compiler/pass/analyse/test/AliasAnalysisTest.scala @@ -94,20 +94,20 @@ class AliasAnalysisTest extends CompilerTest { val childOfChild = child1.addChild() val childOfChildOfChild = childOfChild.addChild() - val aDefId = graph.nextId() - val aDef = new GraphOccurrence.Def(aDefId, "a", genId, None) + val aDef = graph.newDef("a", genId, None) + val aDefId = aDef.id - val bDefId = graph.nextId() - val bDef = new GraphOccurrence.Def(bDefId, "b", genId, None) + val bDef = graph.newDef("b", genId, None) + val bDefId = bDef.id - val aUseId = graph.nextId() - val aUse = new GraphOccurrence.Use(aUseId, "a", genId, None) + val aUse = graph.newUse("a", genId, None) + val aUseId = aUse.id - val bUseId = graph.nextId() - val bUse = new GraphOccurrence.Use(bUseId, "b", genId, None) + val bUse = graph.newUse("b", genId, None) + val bUseId = bUse.id - val cUseId = graph.nextId() - val cUse = new GraphOccurrence.Use(cUseId, "c", genId, None) + val cUse = graph.newUse("c", genId, None) + val cUseId = cUse.id // Add occurrences to the scopes complexScope.add(aDef) @@ -232,20 +232,19 @@ class AliasAnalysisTest extends CompilerTest { val rootScope = graph.rootScope val childScope = rootScope.addChild() - val aDefId = graph.nextId() - val aDef = new GraphOccurrence.Def(aDefId, "a", genId, None) + val aDef = graph.newDef("a", genId, None) + val aDefId = aDef.id - val bDefId = graph.nextId() - val bDef = new GraphOccurrence.Def(bDefId, "b", genId, None) + val bDef = graph.newDef("b", genId, None) - val aUse1Id = graph.nextId() - val aUse1 = new GraphOccurrence.Use(aUse1Id, "a", genId, None) + val aUse1 = graph.newUse("a", genId, None) + val aUse1Id = aUse1.id - val aUse2Id = graph.nextId() - val aUse2 = new GraphOccurrence.Use(aUse2Id, "a", genId, None) + val aUse2 = graph.newUse("a", genId, None) + val aUse2Id = aUse2.id - val cUseId = graph.nextId() - val cUse = new GraphOccurrence.Use(cUseId, "c", genId, None) + val cUse = graph.newUse("c", genId, None) + val cUseId = cUse.id rootScope.add(aDef) rootScope.add(aUse1) @@ -264,16 +263,6 @@ class AliasAnalysisTest extends CompilerTest { graphCopy shouldEqual graph } - "generate monotonically increasing identifiers" in { - val ids = List.fill(100)(graph.nextId()) - var currentId = ids.head - 1 - - ids.forall(id => { - currentId += 1 - currentId == id - }) shouldEqual true - } - "correctly resolve usages where possible" in { use1Link shouldBe defined use2Link shouldBe defined @@ -354,32 +343,22 @@ class AliasAnalysisTest extends CompilerTest { val child2 = rootScope.addChild() val grandChild = child1.addChild() - val aDefInRootId = graph.nextId() - val aDefInRoot = new GraphOccurrence.Def(aDefInRootId, "a", genId, None) + val aDefInRoot = graph.newDef("a", genId, None) rootScope.add(aDefInRoot) - val aDefInChild1Id = graph.nextId() - val aDefInChild1 = - new GraphOccurrence.Def(aDefInChild1Id, "a", genId, None) + val aDefInChild1 = graph.newDef("a", genId, None) child1.add(aDefInChild1) - val aDefInChild2Id = graph.nextId() - val aDefInChild2 = - new GraphOccurrence.Def(aDefInChild2Id, "a", genId, None) + val aDefInChild2 = graph.newDef("a", genId, None) child2.add(aDefInChild2) - val aDefInGrandChildId = graph.nextId() - val aDefInGrandChild = - new GraphOccurrence.Def(aDefInGrandChildId, "a", genId, None) + val aDefInGrandChild = graph.newDef("a", genId, None) grandChild.add(aDefInGrandChild) - val bDefInRootId = graph.nextId() - val bDefInRoot = new GraphOccurrence.Def(bDefInRootId, "b", genId, None) + val bDefInRoot = graph.newDef("b", genId, None) rootScope.add(bDefInRoot) - val bDefInChild2Id = graph.nextId() - val bDefInChild2 = - new GraphOccurrence.Def(bDefInChild2Id, "b", genId, None) + val bDefInChild2 = graph.newDef("b", genId, None) child2.add(bDefInChild2) graph.knownShadowedDefinitions(aDefInGrandChild) shouldEqual Set( diff --git a/lib/java/persistance-dsl/src/main/java/org/enso/persist/impl/PersistableProcessor.java b/lib/java/persistance-dsl/src/main/java/org/enso/persist/impl/PersistableProcessor.java index e7ee6e05f4..933a3c1c54 100644 --- a/lib/java/persistance-dsl/src/main/java/org/enso/persist/impl/PersistableProcessor.java +++ b/lib/java/persistance-dsl/src/main/java/org/enso/persist/impl/PersistableProcessor.java @@ -108,10 +108,7 @@ public class PersistableProcessor extends AbstractProcessor { }; var constructors = typeElem.getEnclosedElements().stream() - .filter( - e -> - e.getModifiers().contains(Modifier.PUBLIC) - && e.getKind() == ElementKind.CONSTRUCTOR) + .filter(e -> isVisibleFrom(e, orig) && e.getKind() == ElementKind.CONSTRUCTOR) .sorted(richerConstructor) .collect(Collectors.toList()); @@ -124,7 +121,7 @@ public class PersistableProcessor extends AbstractProcessor { e -> e.getKind() == ElementKind.FIELD && e.getModifiers().contains(Modifier.STATIC) - && e.getModifiers().contains(Modifier.PUBLIC)) + && isVisibleFrom(e, orig)) .filter(e -> tu.isSameType(e.asType(), typeElem.asType())) .collect(Collectors.toList()); if (singletonFields.isEmpty()) { @@ -275,6 +272,17 @@ public class PersistableProcessor extends AbstractProcessor { return true; } + private boolean isVisibleFrom(Element e, Element from) { + if (e.getModifiers().contains(Modifier.PUBLIC)) { + return true; + } + if (e.getModifiers().contains(Modifier.PRIVATE)) { + return false; + } + var eu = processingEnv.getElementUtils(); + return eu.getPackageOf(e) == eu.getPackageOf(from); + } + private int countInlineRef(List parameters) { var tu = processingEnv.getTypeUtils(); var cnt = 0;