-
Notifications
You must be signed in to change notification settings - Fork 291
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
serialize ContextItems in SerializedChatMessages #5910
base: main
Are you sure you want to change the base?
Changes from all commits
e3addfd
2d5bf53
26c0a2f
a4c1087
b41e769
d2d38d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
@file:Suppress("FunctionName", "ClassName", "unused", "EnumEntryName", "UnusedImport") | ||
package com.sourcegraph.cody.agent.protocol_generated; | ||
|
||
import com.google.gson.annotations.SerializedName; | ||
|
||
data class SerializedContextItem( | ||
val uri: String, | ||
val title: String? = null, | ||
val source: ContextItemSource? = null, // Oneof: user, editor, search, initial, priority, unified, selection, terminal, history, agentic | ||
val range: RangeData? = null, | ||
val repoName: String? = null, | ||
val revision: String? = null, | ||
val description: String? = null, | ||
val size: Long? = null, | ||
val isIgnored: Boolean? = null, | ||
val isTooLarge: Boolean? = null, | ||
val isTooLargeReason: String? = null, | ||
val provider: String? = null, | ||
val icon: String? = null, | ||
val metadata: List<String>? = null, | ||
val type: TypeEnum, // Oneof: repository, tree, symbol, openctx, file | ||
val remoteRepositoryName: String? = null, | ||
val ranges: List<Range>? = null, | ||
val repoID: String, | ||
val isWorkspaceRoot: Boolean, | ||
val name: String, | ||
val symbolName: String, | ||
val kind: SymbolKind, // Oneof: class, function, method | ||
val providerUri: String, | ||
val mention: MentionParams? = null, | ||
) { | ||
|
||
enum class TypeEnum { | ||
@SerializedName("repository") Repository, | ||
@SerializedName("tree") Tree, | ||
@SerializedName("symbol") Symbol, | ||
@SerializedName("openctx") Openctx, | ||
@SerializedName("file") File, | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
export const IGNORED_INFO_SYMBOL: ReadonlyArray<string> = [] | ||
|
||
export const IGNORED_PROPERTIES: ReadonlyArray<string> = [ | ||
'npm @sourcegraph/telemetry ', // Too many complicated types from this package | ||
'`inline-completion-item-provider-config-singleton.ts`/tracer0:', | ||
'`observable.d.ts`/Subscription#', | ||
'`provider.ts`/Provider#configSource', | ||
'`StatusBar.ts`/CodyStatusBar', | ||
'lexicalEditor/`nodes.ts`/content0', | ||
] | ||
|
||
export const IGNORED_TYPE_REFS: ReadonlyArray<string> = [ | ||
'`provider.ts`/Provider#', | ||
'npm @sourcegraph/telemetry', // Too many complicated types from this package | ||
'/TelemetryEventParameters#', | ||
' lib/`lib.es5.d.ts`/Omit#', | ||
] | ||
|
||
/* | ||
Types listed in this array will allow for loose subtyping of their members | ||
By default the type `Item` below would not be allowed | ||
interface HasTitle { | ||
kind: 'has-title' | ||
title: string | ||
} | ||
|
||
interface HasDescription { | ||
kind: 'has-description' | ||
description: string | ||
} | ||
|
||
type Item = { | ||
title?: string | ||
description?: string | ||
} & (HasTitle | HasDescription) | ||
|
||
but with `ALLOW_SUBTYPING_FOR_MEMBERS` set to ['Item'] the above type would be | ||
allowed and would be generated with both title and description as nullable | ||
even though one of them is required depending on the kind of the item | ||
*/ | ||
|
||
export const ALLOW_SUBTYPING_FOR_MEMBERS: ReadonlyArray<string> = [ | ||
'lexicalEditor/`nodes.ts`/SerializedContextItem#', | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,13 +107,16 @@ export abstract class BaseCodegen { | |
return JSON.stringify(msg.toObject(), null, 2) | ||
} | ||
|
||
public isStringType(type: scip.Type): boolean { | ||
public isStringType( | ||
type: scip.Type, | ||
options: TypecheckerOptions = { allowSubtyping: false } | ||
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. It is unclear to me what this means. Consider the case immediately below:
In that sense, 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. @jamesmcnamara wrote this. @jamesmcnamara, can you pls reply? 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. This option allows us consider the a union type of |
||
): boolean { | ||
if (type.has_constant_type) { | ||
return type.constant_type.constant.has_string_constant | ||
} | ||
|
||
if (type.has_union_type) { | ||
return type.union_type.types.every(type => this.isStringType(type)) | ||
return type.union_type.types.every(type => this.isStringType(type, options)) | ||
} | ||
|
||
if (type.has_intersection_type) { | ||
|
@@ -125,6 +128,13 @@ export abstract class BaseCodegen { | |
} | ||
|
||
if (type.has_type_ref) { | ||
const sym = type.type_ref.symbol | ||
if ( | ||
options.allowSubtyping && | ||
[typescriptKeyword('undefined'), typescriptKeyword('null')].includes(sym) | ||
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. Question about the TypeScript AST here: Is a type like 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. @jamesmcnamara wrote this. @jamesmcnamara, can you pls reply? 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. Yes, can confirm. |
||
) { | ||
return true | ||
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. If allow sub typing is true, this doesn't make sense to me, because Would it make sense to recast the function not 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. @jamesmcnamara wrote this. @jamesmcnamara, can you pls reply? |
||
} | ||
return ( | ||
type.type_ref.symbol === typescriptKeyword('string') || | ||
this.isStringTypeInfo(this.symtab.info(type.type_ref.symbol)) | ||
|
@@ -134,7 +144,7 @@ export abstract class BaseCodegen { | |
return false | ||
} | ||
|
||
public isBooleanTypeInfo(info: scip.SymbolInformation): boolean { | ||
public isBooleanTypeInfo(info: scip.SymbolInformation, options?: TypecheckerOptions): boolean { | ||
if (info.signature.has_value_signature && info.signature.value_signature.tpe.has_constant_type) { | ||
return info.signature.value_signature.tpe.constant_type.constant.has_boolean_constant | ||
} | ||
|
@@ -147,33 +157,37 @@ export abstract class BaseCodegen { | |
return false | ||
} | ||
|
||
public isStringTypeInfo(info: scip.SymbolInformation): boolean { | ||
public isStringTypeInfo(info: scip.SymbolInformation, options?: TypecheckerOptions): boolean { | ||
if (info.signature.has_value_signature) { | ||
return this.isStringType(info.signature.value_signature.tpe) | ||
return this.isStringType(info.signature.value_signature.tpe, options) | ||
} | ||
|
||
if ( | ||
info.signature.has_type_signature && | ||
info.signature.type_signature.type_parameters && | ||
info.signature.type_signature.type_parameters.symlinks.length === 0 | ||
) { | ||
return this.isStringType(info.signature.type_signature.lower_bound) | ||
return this.isStringType(info.signature.type_signature.lower_bound, options) | ||
} | ||
|
||
if (info.signature.has_class_signature && info.kind === scip.SymbolInformation.Kind.Enum) { | ||
return info.signature.class_signature.declarations.symlinks.every(member => | ||
this.isStringTypeInfo(this.symtab.info(member)) | ||
this.isStringTypeInfo(this.symtab.info(member), options) | ||
) | ||
} | ||
|
||
return false | ||
} | ||
|
||
public compatibleSignatures(a: scip.SymbolInformation, b: scip.SymbolInformation): boolean { | ||
if (this.isStringTypeInfo(a) && this.isStringTypeInfo(b)) { | ||
public compatibleSignatures( | ||
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 am uncertain about this approach. IF I understand it, you're saying that
is safe because And in practice the escape hatch works for SerializedContextItem because there's no discrimination happening... the consumer just gets that big union type. But would things blow up if we had a method with a type like SerializedContextItemTypeA | SerializedContextItemTypeB? 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. @jamesmcnamara wrote this. @jamesmcnamara, can you pls reply? 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. There shouldn't be any method descriptions in here because this is used to generate code from the agent protocol so it should only contain immediate data types. A type alias like The only issue we're trying to resolve here is the combination of a union type and an intersection type which is treated at the top level as an intersection type (as described in this comment). The current behavior is to check that every shared field in the sub-discriminated unions are identical and we throw an exception if they differ (even on nullability). (Note that there is a bug in the existing implementation in that the output intersection type has all fields from all discriminated union members included, so: type Intersect = {
name: string
} & ({title: string} | {description: string}) would produce: type Intersect = {
name: string
title: string
description: string
} instead of type Intersect = {
name: string
title: string | undefined
description: string | undefined
} and that is not changed in this implementation) The only case we're trying to handle here is where the the types of a field on the intersection type and the sub-union types differ, but they differ only in their nullability (as with the above However in the 1% of cases where it is intentional, as with
1 is the most practical and probably the best practice but introduces an annoying maintenance burden of maintaining two types that are semantically identical but duplicated. 3 feels to me the most correct way but would require a careful, non-trivial implementation. 4 is what the current implementation does only for white-listed types (and indeed we see there is exactly one instance of this in the entire protocol so it's not likely a valuable case to invest in.) @olafurpg's opinion was to use option 1 and in general to avoid allowing complex types to leak into the protocol definition. I implemented 4 because it was quick and allowed us to avoid the conversion types. But with this much pushback and the bug noted above, perhaps 1 is really the way we should go. |
||
a: scip.SymbolInformation, | ||
b: scip.SymbolInformation, | ||
options?: TypecheckerOptions | ||
): boolean { | ||
if (this.isStringTypeInfo(a, options) && this.isStringTypeInfo(b, options)) { | ||
return true | ||
} | ||
if (this.isBooleanTypeInfo(a) && this.isBooleanTypeInfo(b)) { | ||
if (this.isBooleanTypeInfo(a, options) && this.isBooleanTypeInfo(b, options)) { | ||
return true | ||
} | ||
// TODO: more optimized comparison? | ||
|
@@ -306,3 +320,7 @@ export abstract class BaseCodegen { | |
throw new TypeError(`type has no properties: ${this.debug(type)}`) | ||
} | ||
} | ||
|
||
export interface TypecheckerOptions { | ||
allowSubtyping: boolean | ||
} |
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.
What consumes this? Because if it looks for that
:
separator for line ranges, it is going to encounter a:
infile:///
now.