Skip to content
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

Expression update contains full multi-value info #12195

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/language-server/protocol-language-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,10 @@ interface ExpressionUpdate {
* - array with multiple values represents an intersetion type
*/
type: string[];
/**
* The list of types this expression can be converted to.
*/
hiddenType: string[];
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
/** The updated method call info. */
methodCall?: MethodCall;
/** Profiling information about the expression. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ final class ContextEventsListener(
val computedExpressions = expressionUpdates.map { update =>
ContextRegistryProtocol.ExpressionUpdate(
update.expressionId,
update.expressionTypes.getOrElse(Vector()),
update.expressionType.map(_.visibleType).getOrElse(Vector()),
update.expressionType.map(_.hiddenType).getOrElse(Vector()),
update.methodCall.map(toProtocolMethodCall),
update.profilingInfo.map(toProtocolProfilingInfo),
update.fromCache,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ object ContextRegistryProtocol {
*
* @param expressionId the id of updated expression
* @param type the updated type of expression
* @param hiddenType the list of types this expression can be converted to
* @param methodCall the updated method call
* @param profilingInfo profiling information about the expression
* @param fromCache whether the expression's value came from the cache
Expand All @@ -185,6 +186,7 @@ object ContextRegistryProtocol {
case class ExpressionUpdate(
expressionId: UUID,
`type`: Vector[String],
hiddenType: Vector[String],
methodCall: Option[MethodCall],
profilingInfo: Vector[ProfilingInfo],
fromCache: Boolean,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ class ContextEventsListenerSpec
Set(
Api.ExpressionUpdate(
Suggestions.method.externalId.get,
Some(Vector(Suggestions.method.returnType)),
Some(
Api.ExpressionType(
Vector(Suggestions.method.returnType),
Vector(Suggestions.method.selfType)
)
),
Some(methodCall),
Vector(),
false,
Expand All @@ -87,6 +92,7 @@ class ContextEventsListenerSpec
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
Vector(Suggestions.method.returnType),
Vector(Suggestions.method.selfType),
Some(toProtocolMethodCall(methodCall)),
Vector(),
false,
Expand Down Expand Up @@ -136,6 +142,7 @@ class ContextEventsListenerSpec
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
Vector(),
Vector(),
None,
Vector(),
false,
Expand Down Expand Up @@ -174,6 +181,7 @@ class ContextEventsListenerSpec
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
Vector(),
Vector(),
None,
Vector(),
false,
Expand Down Expand Up @@ -230,6 +238,7 @@ class ContextEventsListenerSpec
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
Vector(),
Vector(),
None,
Vector(),
false,
Expand All @@ -239,6 +248,7 @@ class ContextEventsListenerSpec
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.local.externalId.get,
Vector(),
Vector(),
None,
Vector(),
false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ object Runtime {
notAppliedArguments: Vector[Int]
)

/** The type of the expression.
*
* @param visibleType the public type of the expression visible to the user
* @param hiddenType the list of types this expression can be converted to
*/
case class ExpressionType(
visibleType: Vector[String],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I like the terminology:

  • maybe we use visible type somewhere
  • but we certainly never used conversion type!

The documentation is using following terms:

  • "has been cast to (the visible part)"
  • "hidden types .... can be cast to."

I understand that the current terminology isn't perfect. We can change it, but consistently. We shouldn't be inventing conversionType term in the protocol only. If we want new term, we should update (at least) docs as well.

hiddenType: Vector[String]
)

/** A representation of an executable position in code.
*/
sealed trait StackItem
Expand Down Expand Up @@ -108,7 +118,7 @@ object Runtime {
/** An update about the computed expression.
*
* @param expressionId the expression id
* @param expressionTypes the type of expression
* @param expressionType the type of expression
* @param methodCall the underlying method call of this expression
* @param profilingInfo profiling information about the execution of this expression
* @param fromCache whether the value for this expression came from the cache
Expand All @@ -119,7 +129,7 @@ object Runtime {
@named("expressionUpdate")
case class ExpressionUpdate(
expressionId: ExpressionId,
expressionTypes: Option[Vector[String]],
expressionType: Option[ExpressionType],
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
methodCall: Option[MethodCall],
profilingInfo: Vector[ProfilingInfo],
fromCache: Boolean,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
public final class RuntimeCache implements java.util.function.Function<String, Object> {
private final Map<UUID, Reference<Object>> cache = new HashMap<>();
private final Map<UUID, Reference<Object>> expressions = new HashMap<>();
private final Map<UUID, String[]> types = new HashMap<>();
private final Map<UUID, TypeInfo> types = new HashMap<>();
private final Map<UUID, ExecutionService.FunctionCallInfo> calls = new HashMap<>();
private CachePreferences preferences = CachePreferences.empty();
private Consumer<UUID> observer;
Expand Down Expand Up @@ -107,15 +107,15 @@ public Set<UUID> clear(CachePreferences.Kind kind) {
* @return the previously cached type.
*/
@CompilerDirectives.TruffleBoundary
public String[] putType(UUID key, String[] typeNames) {
return types.put(key, typeNames);
public TypeInfo putType(UUID key, TypeInfo typeInfo) {
return types.put(key, typeInfo);
}

/**
* @return the cached type of the expression
*/
@CompilerDirectives.TruffleBoundary
public String[] getType(UUID key) {
public TypeInfo getType(UUID key) {
return types.get(key);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package org.enso.interpreter.instrument;

import java.util.Arrays;

/**
* The type information observed by the instrumentation.
*
* <p>The list in the type definition represents an intersection type. A list with a single element
* represents a simple type.
*
* @param visibleType the public type of the value visible to the user
* @param hiddenType the list of types the value can be converted to
*/
public record TypeInfo(String[] visibleType, String[] hiddenType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also use plural?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

record is immutable structure, but it references mutable String[] arrays. Isn't it better to use List<String> and make it immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is close to runtime, so I decided that arrays are a better choice than lists


public static TypeInfo ofType(String typeName) {
return new TypeInfo(new String[] {typeName}, new String[0]);
}

public static TypeInfo ofIntersectionType(String[] intersectionType) {
return new TypeInfo(intersectionType, new String[0]);
}

@Override
public String toString() {
return "TypeInfo(" + Arrays.toString(visibleType) + "," + Arrays.toString(hiddenType) + ")";
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.enso.interpreter.service;

import com.oracle.truffle.api.CompilerDirectives;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.UUID;
Expand All @@ -9,6 +10,7 @@
import org.enso.interpreter.instrument.MethodCallsCache;
import org.enso.interpreter.instrument.OneshotExpression;
import org.enso.interpreter.instrument.RuntimeCache;
import org.enso.interpreter.instrument.TypeInfo;
import org.enso.interpreter.instrument.UpdatesSynchronizationState;
import org.enso.interpreter.instrument.VisualizationHolder;
import org.enso.interpreter.instrument.profiling.ExecutionTime;
Expand Down Expand Up @@ -98,16 +100,16 @@ public Object findCachedResult(IdExecutionService.Info info) {
@Override
public void updateCachedResult(IdExecutionService.Info info) {
Object result = info.getResult();
String[] resultTypes = typeOf(result);
TypeInfo resultType = typeOf(result);
UUID nodeId = info.getId();
String[] cachedTypes = cache.getType(nodeId);
TypeInfo cachedType = cache.getType(nodeId);
FunctionCallInfo call = functionCallInfoById(nodeId);
FunctionCallInfo cachedCall = cache.getCall(nodeId);
ProfilingInfo[] profilingInfo = new ProfilingInfo[] {new ExecutionTime(info.getElapsedTime())};

ExpressionValue expressionValue =
new ExpressionValue(
nodeId, result, resultTypes, cachedTypes, call, cachedCall, profilingInfo, false);
nodeId, result, resultType, cachedType, call, cachedCall, profilingInfo, false);
syncState.setExpressionUnsync(nodeId);
syncState.setVisualizationUnsync(nodeId);

Expand All @@ -119,7 +121,7 @@ public void updateCachedResult(IdExecutionService.Info info) {
cache.offer(nodeId, result);
cache.putCall(nodeId, call);
}
cache.putType(nodeId, resultTypes);
cache.putType(nodeId, resultType);

callOnComputedCallback(expressionValue);
executeOneshotExpressions(nodeId, result, info);
Expand Down Expand Up @@ -214,19 +216,32 @@ private FunctionCallInfo functionCallInfoById(UUID nodeId) {
return calls.get(nodeId);
}

private String[] typeOf(Object value) {
private TypeInfo typeOf(Object value) {
if (value instanceof UnresolvedSymbol) {
return new String[] {Constants.UNRESOLVED_SYMBOL};
return TypeInfo.ofType(Constants.UNRESOLVED_SYMBOL);
}

var typeOfNode = TypeOfNode.getUncached();
Type[] allTypes = value == null ? null : typeOfNode.findAllTypesOrNull(value, true);
if (allTypes != null) {
String[] result = new String[allTypes.length];
for (var i = 0; i < allTypes.length; i++) {
result[i] = getTypeQualifiedName(allTypes[i]);
if (value != null) {
final TypeOfNode typeOfNode = TypeOfNode.getUncached();
final Type[] publicTypes = typeOfNode.findAllTypesOrNull(value, false);

if (publicTypes != null) {
final Type[] allTypes = typeOfNode.findAllTypesOrNull(value, true);
assert Arrays.equals(publicTypes, Arrays.copyOfRange(allTypes, 0, publicTypes.length));
final Type[] hiddenTypes =
Arrays.copyOfRange(allTypes, publicTypes.length, allTypes.length);

final String[] publicTypeNames = new String[publicTypes.length];
for (var i = 0; i < publicTypes.length; i++) {
publicTypeNames[i] = getTypeQualifiedName(publicTypes[i]);
}
final String[] hiddenTypeNames = new String[hiddenTypes.length];
for (var i = 0; i < hiddenTypeNames.length; i++) {
hiddenTypeNames[i] = getTypeQualifiedName(hiddenTypes[i]);
}

return new TypeInfo(publicTypeNames, hiddenTypeNames);
}
return result;
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.enso.interpreter.instrument.ExpressionExecutionState;
import org.enso.interpreter.instrument.MethodCallsCache;
import org.enso.interpreter.instrument.RuntimeCache;
import org.enso.interpreter.instrument.TypeInfo;
import org.enso.interpreter.instrument.UpdatesSynchronizationState;
import org.enso.interpreter.instrument.VisualizationHolder;
import org.enso.interpreter.instrument.profiling.ProfilingInfo;
Expand Down Expand Up @@ -662,8 +663,8 @@ public FunctionCallInstrumentationNode.FunctionCall getCall() {
public static final class ExpressionValue {
private final UUID expressionId;
private final Object value;
private final String[] types;
private final String[] cachedTypes;
private final TypeInfo typeInfo;
private final TypeInfo cachedTypeInfo;
private final FunctionCallInfo callInfo;
private final FunctionCallInfo cachedCallInfo;
private final ProfilingInfo[] profilingInfo;
Expand All @@ -674,8 +675,8 @@ public static final class ExpressionValue {
*
* @param expressionId the id of the expression being computed.
* @param value the value returned by computing the expression.
* @param types the type of the returned value.
* @param cachedTypes the cached type of the value.
* @param typeInfo the type info of the returned value.
* @param cachedTypeInfo the cached type info of the value.
* @param callInfo the function call data.
* @param cachedCallInfo the cached call data.
* @param profilingInfo the profiling information associated with this node
Expand All @@ -684,16 +685,16 @@ public static final class ExpressionValue {
public ExpressionValue(
UUID expressionId,
Object value,
String[] types,
String[] cachedTypes,
TypeInfo typeInfo,
TypeInfo cachedTypeInfo,
FunctionCallInfo callInfo,
FunctionCallInfo cachedCallInfo,
ProfilingInfo[] profilingInfo,
boolean wasCached) {
this.expressionId = expressionId;
this.value = value;
this.types = types;
this.cachedTypes = cachedTypes;
this.typeInfo = typeInfo;
this.cachedTypeInfo = cachedTypeInfo;
this.callInfo = callInfo;
this.cachedCallInfo = cachedCallInfo;
this.profilingInfo = profilingInfo;
Expand All @@ -708,11 +709,11 @@ public String toString() {
+ expressionId
+ ", value="
+ (value == null ? "null" : new MaskedString(value.toString()).applyMasking())
+ ", types='"
+ Arrays.toString(types)
+ ", typeInfo='"
+ typeInfo
+ '\''
+ ", cachedTypes='"
+ Arrays.toString(cachedTypes)
+ ", cachedTypeInfo='"
+ cachedTypeInfo
+ '\''
+ ", callInfo="
+ callInfo
Expand All @@ -735,15 +736,15 @@ public UUID getExpressionId() {
/**
* @return the type of the returned value.
*/
public String[] getTypes() {
return types;
public TypeInfo getType() {
return typeInfo;
}

/**
* @return the cached type of the value.
*/
public String[] getCachedTypes() {
return cachedTypes;
public TypeInfo getCachedType() {
return cachedTypeInfo;
}

/**
Expand Down Expand Up @@ -785,7 +786,22 @@ public boolean wasCached() {
* @return {@code true} when the type differs from the cached value.
*/
public boolean isTypeChanged() {
return !Arrays.equals(types, cachedTypes);
String[] visibleType = null;
String[] hiddenType = null;
if (typeInfo != null) {
visibleType = typeInfo.visibleType();
hiddenType = typeInfo.hiddenType();
}

String[] cachedVisibleType = null;
String[] cachedHiddenType = null;
if (cachedTypeInfo != null) {
cachedVisibleType = cachedTypeInfo.visibleType();
cachedHiddenType = cachedTypeInfo.hiddenType();
}

return !Arrays.equals(visibleType, cachedVisibleType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this delegate to typeInfo.equals(...) method? Once properly implemented to deal with String[] or once we use List<String> instead?

|| !Arrays.equals(hiddenType, cachedHiddenType);
}

/**
Expand Down
Loading
Loading