-
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
Occurences in Java. Don't expose setters for Scope vars. #11464
Changes from all commits
3a04f78
2b8ad1e
275a181
ca01e11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
package org.enso.compiler.pass.analyse.alias.graph; | ||
|
||
import java.util.UUID; | ||
import org.enso.compiler.core.ExternalID; | ||
import org.enso.compiler.core.Identifier; | ||
|
||
/** | ||
* An occurrence of a given symbol in the aliasing graph. Note that this is not present in the | ||
* metadata attached to the [[org.enso.compiler.core.IR]] elements, but only in the alias [[Graph]]. | ||
*/ | ||
public sealed interface GraphOccurrence permits GraphOccurrence.Def, GraphOccurrence.Use { | ||
public abstract int id(); | ||
|
||
public abstract String symbol(); | ||
|
||
/** The definition of a symbol in the aliasing graph. */ | ||
public static final class Def implements GraphOccurrence { | ||
private final int id; | ||
private final String symbol; | ||
private final @Identifier UUID identifier; | ||
private final @ExternalID UUID externalId; | ||
private final boolean isLazy; | ||
|
||
/** | ||
* The definition of a symbol in the aliasing graph. | ||
* | ||
* @param id the identifier of the name in the graph | ||
* @param symbol the text of the name | ||
* @param identifier the identifier of the symbol | ||
* @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<UUID> externalId, boolean isLazy) { | ||
this.id = id; | ||
this.externalId = externalId.nonEmpty() ? externalId.get() : null; | ||
this.identifier = identifier; | ||
this.isLazy = isLazy; | ||
this.symbol = symbol; | ||
} | ||
|
||
public Def(int id, String symbol, UUID identifier, scala.Option<UUID> externalId) { | ||
this(id, symbol, identifier, externalId, false); | ||
} | ||
|
||
@Override | ||
public int id() { | ||
return this.id; | ||
} | ||
|
||
@Override | ||
public String symbol() { | ||
return this.symbol; | ||
} | ||
|
||
public UUID identifier() { | ||
return identifier; | ||
} | ||
|
||
public scala.Option<UUID> externalId() { | ||
return scala.Option.apply(externalId); | ||
} | ||
|
||
public boolean isLazy() { | ||
return isLazy; | ||
} | ||
|
||
public static scala.Option<scala.Tuple5<Integer, String, UUID, scala.Option<UUID>, Boolean>> | ||
unapply(Object obj) { | ||
if (obj instanceof Def d) { | ||
var extId = scala.Option.apply(d.externalId); | ||
var tuple = new scala.Tuple5<>(d.id, d.symbol, d.identifier, extId, d.isLazy); | ||
return scala.Option.apply(tuple); | ||
} | ||
return scala.Option.empty(); | ||
} | ||
} | ||
|
||
/** A usage of a symbol in the aliasing graph */ | ||
public static final class Use implements GraphOccurrence { | ||
private final int id; | ||
private final String symbol; | ||
private final @Identifier UUID identifier; | ||
private final @ExternalID UUID externalId; | ||
|
||
/** | ||
* A usage of a symbol in the aliasing graph | ||
* | ||
* <p>Name usages _need not_ correspond to name definitions, as dynamic symbol resolution means | ||
* that a name used at runtime _may not_ be statically visible in the scope. | ||
* | ||
* @param id the identifier of the name in the graph | ||
* @param symbol the text of the name | ||
* @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<UUID> externalId) { | ||
this.id = id; | ||
this.symbol = symbol; | ||
this.externalId = externalId.nonEmpty() ? externalId.get() : null; | ||
this.identifier = identifier; | ||
} | ||
|
||
@Override | ||
public int id() { | ||
return this.id; | ||
} | ||
|
||
@Override | ||
public String symbol() { | ||
return this.symbol; | ||
} | ||
|
||
public UUID identifier() { | ||
return identifier; | ||
} | ||
|
||
public scala.Option<UUID> externalId() { | ||
return scala.Option.apply(externalId); | ||
} | ||
|
||
public static scala.Option<scala.Tuple4<Integer, String, UUID, scala.Option<UUID>>> unapply( | ||
Object obj) { | ||
if (obj instanceof Use u) { | ||
var extId = scala.Option.apply(u.externalId); | ||
var tuple = new scala.Tuple4<>(u.id, u.symbol, u.identifier, extId); | ||
return scala.Option.apply(tuple); | ||
} | ||
return scala.Option.empty(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -318,12 +318,17 @@ object Graph { | |
* Note that there may not be a link for all these definitions. | ||
*/ | ||
sealed class Scope( | ||
var childScopes: List[Scope] = List(), | ||
var occurrences: Map[Id, GraphOccurrence] = HashMap(), | ||
var allDefinitions: List[GraphOccurrence.Def] = List() | ||
private[Graph] var _childScopes: List[Scope] = List(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the purpose of this refactoring. Why not simply define these fields as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm only guessing but it looks like he wanted to limit the scope of variables so that one does not accidentally mutate them? But yes, the motivation is unclear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The title of this PR is:
Renaming the fields to |
||
private[Graph] var _occurrences: Map[Id, GraphOccurrence] = HashMap(), | ||
private[Graph] var _allDefinitions: List[GraphOccurrence.Def] = List() | ||
) { | ||
|
||
var parent: Option[Scope] = None | ||
private[Graph] var _parent: Scope = null | ||
|
||
def childScopes = _childScopes | ||
def occurrences = _occurrences | ||
def allDefinitions = _allDefinitions | ||
def parent = if (this._parent eq null) None else Some(_parent) | ||
|
||
/** Counts the number of scopes from this scope to the root. | ||
* | ||
|
@@ -342,8 +347,10 @@ object Graph { | |
* @return this scope with parent scope set | ||
*/ | ||
final def withParent(parentScope: Scope): this.type = { | ||
org.enso.common.Asserts.assertInJvm(parent.isEmpty) | ||
this.parent = Some(parentScope) | ||
if (this._parent ne parentScope) { | ||
org.enso.common.Asserts.assertInJvm(parent.isEmpty) | ||
this._parent = parentScope | ||
} | ||
this | ||
} | ||
|
||
|
@@ -396,8 +403,8 @@ object Graph { | |
*/ | ||
final def addChild(): Scope = { | ||
val scope = new Scope() | ||
scope.parent = Some(this) | ||
childScopes ::= scope | ||
scope._parent = this | ||
_childScopes ::= scope | ||
|
||
scope | ||
} | ||
|
@@ -412,7 +419,7 @@ object Graph { | |
s"Multiple occurrences found for ID ${occurrence.id}." | ||
) | ||
} else { | ||
occurrences += ((occurrence.id, occurrence)) | ||
_occurrences += ((occurrence.id, occurrence)) | ||
} | ||
} | ||
|
||
|
@@ -422,7 +429,7 @@ object Graph { | |
* @param definition The definition to add. | ||
*/ | ||
final def addDefinition(definition: GraphOccurrence.Def): Unit = { | ||
allDefinitions = allDefinitions ++ List(definition) | ||
_allDefinitions = allDefinitions ++ List(definition) | ||
} | ||
|
||
/** Finds an occurrence for the provided ID in the current scope, if it | ||
|
@@ -489,7 +496,7 @@ object Graph { | |
case None => | ||
parent.flatMap(_.resolveUsage(occurrence, parentCounter + 1)) | ||
case Some(target) => | ||
Some(Graph.Link(occurrence.id, parentCounter, target.id)) | ||
Some(Graph.Link(occurrence.id, parentCounter, target.id())) | ||
} | ||
} | ||
|
||
|
@@ -641,7 +648,7 @@ object Graph { | |
} | ||
|
||
private def removeScopeFromParent(scope: Scope): Unit = { | ||
childScopes = childScopes.filter(_ != scope) | ||
_childScopes = childScopes.filter(_ != scope) | ||
} | ||
|
||
/** Disassociates this Scope from its parent. | ||
|
This file was deleted.
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.
Why not declare it as record? It used to be
sealed case class
in Scala.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.
The problem with
record
is that all (default) constructor fields are publicly accessible. I plan to control the encapsulation more in some subsequent changes. Thus I went the more verbose way and created aclass
.