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

Deliver full intersection type of a value to the IDE #11583

Merged
merged 12 commits into from
Nov 22, 2024
2 changes: 1 addition & 1 deletion app/gui/src/project-view/stores/graph/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
profilingInfo: update.profilingInfo ?? [],
fromCache: update.fromCache ?? false,
payload: update.payload ?? { type: 'Value' },
...(update.type ? { type: update.type } : {}),
type: update.type ?? [],
...(update.methodCall ? { methodCall: update.methodCall } : {}),
}
proj.computedValueRegistry.processUpdates([update_])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ function updateInfo(info: ExpressionInfo, update: ExpressionUpdate) {

function combineInfo(info: ExpressionInfo | undefined, update: ExpressionUpdate): ExpressionInfo {
const isPending = update.payload.type === 'Pending'
const updateSingleValueType = update.type.at(0) // TODO: support multi-value (aka intersection) types
return {
typename: update.type ?? (isPending ? info?.typename : undefined),
typename: updateSingleValueType ?? (isPending ? info?.typename : undefined),
methodCall: update.methodCall ?? (isPending ? info?.methodCall : undefined),
payload: update.payload,
profilingInfo: update.profilingInfo,
Expand Down
2 changes: 1 addition & 1 deletion app/ydoc-shared/src/languageServerTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ export interface ExpressionUpdate {
/** The id of updated expression. */
expressionId: ExpressionId
/** The updated type of the expression. */
type?: string
type: string[]
Copy link
Member

Choose a reason for hiding this comment

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

This is an incompatible message change. Single values used to be represented as "string" and now they will be [ "string" ]. Are we sure we don't need compatibility?

/** The updated method call info. */
methodCall?: MethodCall
/** Profiling information about the expression. */
Expand Down
10 changes: 8 additions & 2 deletions docs/language-server/protocol-language-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,14 @@ An update about the computed expression.
interface ExpressionUpdate {
/** The id of updated expression. */
expressionId: ExpressionId;
/** The updated type of the expression. */
type?: string;
/** The updated type of the expression.
*
* Possible values:
* - empty array indicates no type information for this expression
* - array with a single value contains a value of this expression
* - array with multiple values represents an intersetion type
*/
type: 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 feel incompatible changes should be versioned and documented. We should:

  • increase the version of the protocol
  • describe what change has happened and why it is incompatible

Sure, I know we have no version of the protocol. However I asked for it a year ago. Moreover recently there were complains about incompatibilities between cloud/IDE and the conclusion was: we should at least detect the incompatibilities by versioning our protocols.

/** 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,7 @@ final class ContextEventsListener(
val computedExpressions = expressionUpdates.map { update =>
ContextRegistryProtocol.ExpressionUpdate(
update.expressionId,
update.expressionType,
update.expressionTypes.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 @@ -176,15 +176,15 @@ object ContextRegistryProtocol {
/** An update about computed expression.
*
* @param expressionId the id of updated expression
* @param `type` the updated type of expression
* @param type the updated type of expression
* @param methodCall the updated method call
* @param profilingInfo profiling information about the expression
* @param fromCache whether or not the expression's value came from the cache
* @param fromCache whether the expression's value came from the cache
* @param payload an extra information about the computed value
*/
case class ExpressionUpdate(
expressionId: UUID,
`type`: Option[String],
`type`: Vector[String],
methodCall: Option[MethodCall],
profilingInfo: Vector[ProfilingInfo],
fromCache: Boolean,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,6 @@ final class SuggestionsHandler(
config,
suggestionsRepo
)
context.system.eventStream
.subscribe(self, classOf[Api.ExpressionUpdates])
context.system.eventStream
.subscribe(self, classOf[Api.SuggestionsDatabaseModuleUpdateNotification])
context.system.eventStream.subscribe(
Expand Down Expand Up @@ -264,54 +262,6 @@ final class SuggestionsHandler(
)
)

case msg: Api.ExpressionUpdates if state.isSuggestionUpdatesRunning =>
state.suggestionUpdatesQueue.enqueue(msg)

case Api.ExpressionUpdates(_, updates) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? Seems rather unrelated. Or is that feature no longer used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, after the component browser is finished, and GUI can handle suggestions itself, we don't need to do this logic anymore

logger.debug(
"Received expression updates [{}]",
updates.map(u => (u.expressionId, u.expressionType))
)
val types = updates.toSeq
.filter(_.typeChanged)
.flatMap(update => update.expressionType.map(update.expressionId -> _))
suggestionsRepo
.updateAll(types)
.map { case (version, updatedIds) =>
val updates = types.zip(updatedIds).collect {
case ((_, typeValue), Some(suggestionId)) =>
SuggestionsDatabaseUpdate.Modify(
id = suggestionId,
returnType = Some(fieldUpdate(typeValue))
)
}
SuggestionsDatabaseUpdateNotification(version, updates)
}
.onComplete {
case Success(notification) =>
if (notification.updates.nonEmpty) {
clients.foreach { clientId =>
sessionRouter ! DeliverToJsonController(clientId, notification)
}
}
self ! SuggestionsHandler.SuggestionUpdatesCompleted
case Failure(ex) =>
logger.error(
"Error applying changes from computed values [{}]",
updates.map(_.expressionId),
ex
)
self ! SuggestionsHandler.SuggestionUpdatesCompleted
}
context.become(
initialized(
projectName,
graph,
clients,
state.suggestionUpdatesRunning()
)
)

case Api.BackgroundJobsStartedNotification() =>
self ! SuggestionLoadingCompleted
context.become(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class ContextEventsListenerSpec
Set(
Api.ExpressionUpdate(
Suggestions.method.externalId.get,
Some(Suggestions.method.returnType),
Some(Vector(Suggestions.method.returnType)),
Some(methodCall),
Vector(),
false,
Expand All @@ -86,7 +86,7 @@ class ContextEventsListenerSpec
Vector(
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
Some(Suggestions.method.returnType),
Vector(Suggestions.method.returnType),
Some(toProtocolMethodCall(methodCall)),
Vector(),
false,
Expand Down Expand Up @@ -135,7 +135,7 @@ class ContextEventsListenerSpec
Vector(
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
None,
Vector(),
None,
Vector(),
false,
Expand Down Expand Up @@ -173,7 +173,7 @@ class ContextEventsListenerSpec
Vector(
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
None,
Vector(),
None,
Vector(),
false,
Expand Down Expand Up @@ -229,7 +229,7 @@ class ContextEventsListenerSpec
Vector(
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.method.externalId.get,
None,
Vector(),
None,
Vector(),
false,
Expand All @@ -238,7 +238,7 @@ class ContextEventsListenerSpec
),
ContextRegistryProtocol.ExpressionUpdate(
Suggestions.local.externalId.get,
None,
Vector(),
None,
Vector(),
false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,20 +108,18 @@ object Runtime {
/** An update about the computed expression.
*
* @param expressionId the expression id
* @param expressionType the type of expression
* @param expressionTypes 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 or not the value for this expression came
* from the cache
* @param typeChanged whether or not the type of the value or method definition
* @param profilingInfo profiling information about the execution of this expression
* @param fromCache whether the value for this expression came from the cache
* @param typeChanged whether the type of the value or method definition
* has changed from the one that was cached, if any
* @param payload an extra information about the computed value
*/
@named("expressionUpdate")
case class ExpressionUpdate(
expressionId: ExpressionId,
expressionType: Option[String],
expressionTypes: Option[Vector[String]],
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, String[]> 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 typeName) {
return types.put(key, typeName);
public String[] putType(UUID key, String[] typeNames) {
return types.put(key, typeNames);
}

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,16 @@ public Object findCachedResult(IdExecutionService.Info info) {
@Override
public void updateCachedResult(IdExecutionService.Info info) {
Object result = info.getResult();
String resultType = typeOf(result);
String[] resultTypes = typeOf(result);
UUID nodeId = info.getId();
String cachedType = cache.getType(nodeId);
String[] cachedTypes = 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, resultType, cachedType, call, cachedCall, profilingInfo, false);
nodeId, result, resultTypes, cachedTypes, call, cachedCall, profilingInfo, false);
syncState.setExpressionUnsync(nodeId);
syncState.setVisualizationUnsync(nodeId);

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

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

private String typeOf(Object value) {
String resultType;
private String[] typeOf(Object value) {
if (value instanceof UnresolvedSymbol) {
resultType = Constants.UNRESOLVED_SYMBOL;
} else {
var typeOfNode = TypeOfNode.getUncached();
Object typeResult = value == null ? null : typeOfNode.findTypeOrError(value);
if (typeResult instanceof Type t) {
resultType = getTypeQualifiedName(t);
} else {
resultType = null;
return new String[] {Constants.UNRESOLVED_SYMBOL};
}

var typeOfNode = TypeOfNode.getUncached();
Type[] allTypes = value == null ? null : typeOfNode.findAllTypesOrNull(value);
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

if (allTypes != null) {
String[] result = new String[allTypes.length];
for (var i = 0; i < allTypes.length; i++) {
result[i] = getTypeQualifiedName(allTypes[i]);
}
return result;
}
return resultType;

return null;
}

@CompilerDirectives.TruffleBoundary
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -661,8 +661,8 @@ public FunctionCallInstrumentationNode.FunctionCall getCall() {
public static final class ExpressionValue {
private final UUID expressionId;
private final Object value;
private final String type;
private final String cachedType;
private final String[] types;
private final String[] cachedTypes;
private final FunctionCallInfo callInfo;
private final FunctionCallInfo cachedCallInfo;
private final ProfilingInfo[] profilingInfo;
Expand All @@ -673,8 +673,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 type the type of the returned value.
* @param cachedType the cached type of the value.
* @param types the type of the returned value.
* @param cachedTypes the cached type 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 @@ -683,16 +683,16 @@ public static final class ExpressionValue {
public ExpressionValue(
UUID expressionId,
Object value,
String type,
String cachedType,
String[] types,
String[] cachedTypes,
FunctionCallInfo callInfo,
FunctionCallInfo cachedCallInfo,
ProfilingInfo[] profilingInfo,
boolean wasCached) {
this.expressionId = expressionId;
this.value = value;
this.type = type;
this.cachedType = cachedType;
this.types = types;
this.cachedTypes = cachedTypes;
this.callInfo = callInfo;
this.cachedCallInfo = cachedCallInfo;
this.profilingInfo = profilingInfo;
Expand All @@ -707,11 +707,11 @@ public String toString() {
+ expressionId
+ ", value="
+ (value == null ? "null" : new MaskedString(value.toString()).applyMasking())
+ ", type='"
+ type
+ ", types='"
+ Arrays.toString(types)
+ '\''
+ ", cachedType='"
+ cachedType
+ ", cachedTypes='"
+ Arrays.toString(cachedTypes)
+ '\''
+ ", callInfo="
+ callInfo
Expand All @@ -734,15 +734,15 @@ public UUID getExpressionId() {
/**
* @return the type of the returned value.
*/
public String getType() {
return type;
public String[] getTypes() {
return types;
}

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

/**
Expand Down Expand Up @@ -784,7 +784,7 @@ public boolean wasCached() {
* @return {@code true} when the type differs from the cached value.
*/
public boolean isTypeChanged() {
return !Objects.equals(type, cachedType);
return !Arrays.equals(types, cachedTypes);
}

/**
Expand Down
Loading
Loading