-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use GraphBuilder to construct an alias Graph #11491
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would be even better if we could ensure that the build (toScope
and toGraph
) methods on the builder would be called just once.
} | ||
|
||
public Graph toGraph() { | ||
return graph; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ensure that toGraph
was called just once? Looking at AliasAnalysis
, it seems to me that toGraph
creates snapshots of a graph, i.e., the builder is reusable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that, but it is probably not going to work. For example processing arguments is trying to modify a builder that has already been queried for a graph/scope:
org.enso.compiler.pass.analyse.alias.graph.GraphBuilder.assertNotFreezed(GraphBuilder.java:124)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.alias.graph.GraphBuilder.newDef(GraphBuilder.java:97)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.AliasAnalysis$.$anonfun$analyseArgumentDefs$1(AliasAnalysis.scala:570)
at scala.library@2.13.11/scala.collection.immutable.List.map(List.scala:250)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.AliasAnalysis$.analyseArgumentDefs(AliasAnalysis.scala:521)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.AliasAnalysis$.$anonfun$analyseModuleDefinition$1(AliasAnalysis.scala:292)
at scala.library@2.13.11/scala.collection.immutable.List.map(List.scala:246)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.AliasAnalysis$.analyseModuleDefinition(AliasAnalysis.scala:287)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.AliasAnalysis$.$anonfun$runModule$1(AliasAnalysis.scala:100)
at scala.library@2.13.11/scala.collection.immutable.List.map(List.scala:246)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.AliasAnalysis$.runModule(AliasAnalysis.scala:100)
at org.enso.runtime/org.enso.compiler.pass.analyse.test.AliasAnalysisTest$AnalyseModule.analyse(AliasAnalysisTest.scala:58)
at org.enso.runtime/org.enso.compiler.pass.analyse.test.AliasAnalysisTest.$anonfun$new$37(AliasAnalysisTest.scala:414)
at org.scalatest.SuperEngine.registerNestedBranch(Engine.scala:609)
at org.scalatest.wordspec.AnyWordSpecLike.org$scalatest$wordspec$AnyWordSpecLike$$registerBranch(AnyWordSpecLike.scala:218)
at org.scalatest.wordspec.AnyWordSpecLike$$anon$1.apply(AnyWordSpecLike.scala:1158)
at org.scalatest.verbs.ShouldVerb$StringShouldWrapperForVerb.should(ShouldVerb.scala:194)
at org.scalatest.verbs.ShouldVerb$StringShouldWrapperForVerb.should$(ShouldVerb.scala:193)
at org.scalatest.matchers.should.Matchers$StringShouldWrapper.should(Matchers.scala:8242)
at org.enso.runtime/org.enso.compiler.pass.analyse.test.AliasAnalysisTest.<init>(AliasAnalysisTest.scala:407)
at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
at java.base/java.lang.reflect.ReflectAccess.newInstance(ReflectAccess.java:128)
at java.base/jdk.internal.reflect.ReflectionFactory.newInstance(ReflectionFactory.java:304)
at java.base/java.lang.Class.newInstance(Class.java:725)
at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:454)
at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:414)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.lang.AssertionError: Freezed here!
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.alias.graph.GraphBuilder.toGraph(GraphBuilder.java:113)
at org.enso.runtime.compiler/org.enso.compiler.pass.analyse.AliasAnalysis$.$anonfun$analyseArgumentDefs$1(AliasAnalysis.scala:583)
at scala.library@2.13.11/scala.collection.immutable.List.map(List.scala:246)
Maybe it doesn't matter. The goal of the GraphBuilder
was to identify the code that can modify the Graph.Scope
- which has been achieved. All such code needs the GraphBuilder
.
Jaroslav Tulach reports a new STANDUP for yesterday (2024-11-05): Progress: .
Next Day: Discussions and
|
Pull Request Description
Another refactoring in the line of #11486 to set us better up for #11365. Introducing
GraphBuilder
with methods to modify theGraph
and itsScope
. Keeping "getter-like methods" inGraph
andGraph.Scope
.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,