-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(fmt): lsp find references #4934
Conversation
CodSpeed Performance ReportMerging #4934 will not alter performanceComparing Summary
|
WASM Query Engine file Size
|
test views on name and as type
Add AttributePosition::ArgumentValue(...)
… for completions (from engines) so we're now capable of offering completions based on partial text :)
let arr = arg.value.as_array().unwrap(); | ||
let expr = arr.0.iter().find(|expr| expr.span().contains(position)); | ||
if let Some(expr) = expr { | ||
return Self::ArgumentValue(arg.name(), expr.to_string()); |
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.
Problem: when using nested composite type fields in attributes, i.e.
model User {
id String @id @map("_id") @db.String
address Address
@@unique([address.poBox.name])
}
type Address {
street String @db.String
city String
poBox POBox
}
type SubType {
name String
}
We retrieve the whole string that is defined, i.e.
"address.poBox.name"
and dont have more granular understanding of if the cursor is inside address
, poBox
or name
.
This PR already supports finding fields from non-nested fields in block attributes, but I'm not sure what to do with this.
This seems like it would require modification to this pest grammar:
expression = { function_call | array_expression | numeric_literal | string_literal | path }
string_literal = ${ "\"" ~ string_content ~ "\"" }
string_content = @{ (string_escape | !("\"" | ASCII_CONTROL_CHARACTER) ~ ANY)* }
But I'm not fully sure of the ramifications of changing the pest grammar like this. If this is something we want to explore, I think it should be in a follow-up PR as a stretch goal.
We could maybe make string literal an option of string content or string array content 🤔
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 string literal rule is irrelevant here, it matches a quoted string literal in source code of the schema and there are no quotes here. The relevant rule is path
, and it is already defined as
identifier = @{ ASCII_ALPHANUMERIC ~ ( "_" | "-" | ASCII_ALPHANUMERIC)* }
path = @{ identifier ~ ("." ~ path?)* }
so no modifications are necessary in the grammar, it is already defined the way you need.
You need to modify the Rule::path
case in parse_expression
in psl/schema-ast/src/parser/parse_expression.rs
to recurse into path
and create a separate span for each identifier
token instead of converting the whole subtree to string as it does now.
/// `CodeActionParams` object and the response being a list of | ||
/// `CodeActionOrCommand` objects. | ||
#[wasm_bindgen] | ||
pub fn references(schema: String, params: String) -> String { |
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.
new wasm endpoint for references to be used by language-tools
Vec::new() | ||
} | ||
|
||
pub(crate) fn references(schema_files: Vec<(String, SourceFile)>, params: ReferenceParams) -> Vec<Location> { |
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.
This is where we actually handle finding all the references
@@ -495,7 +495,7 @@ impl Connector for PostgresDatamodelConnector { | |||
ast::ModelPosition::ModelAttribute( | |||
"index", | |||
attr_id, | |||
ast::AttributePosition::FunctionArgument(field_name, "ops"), |
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.
changed to line-up with changes in find_at_position
@@ -85,19 +86,47 @@ fn push_ast_completions(ctx: CompletionContext<'_>, completion_list: &mut Comple | |||
.relation_mode() | |||
.unwrap_or_else(|| ctx.connector().default_relation_mode()); | |||
|
|||
match ctx.db.ast(ctx.initiating_file_id).find_at_position(position) { | |||
let find_at_position = ctx.db.ast(ctx.initiating_file_id).find_at_position(position); |
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.
In the process of updating find_at_position
for references
I broke completions for referential actions. However, due to the changes in find_at_position
we are now able to offer completions on partial values:
Screen.Recording.2024-06-25.at.22.47.11.mov
@@ -86,7 +86,7 @@ pub fn preview_features() -> String { | |||
} | |||
|
|||
/// The API is modelled on an LSP [completion | |||
/// request](https://github.com/microsoft/language-server-protocol/blob/gh-pages/_specifications/specification-3-16.md#textDocument_completion). | |||
/// request](https://github.com/microsoft/language-server-protocol/blob/gh-pages/_specifications/specification-3-16.md#completion-request-leftwards_arrow_with_hook). |
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.
These docs links were outdated, they now link to the correct sections :)
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.
Nice work, there's a couple of things you can improve
find_where_used_* - return iterators instead of immediately collecting to vecs - Note: these then have to be immediately collected by the caller as rust otherwise complains about opaque types. Can be revisited later if we see fit. find_where_used_in_model - rename *_as_field_name - now returns an optional identifier instead of a vec - cut down iteration - finding the model / view is now infallible find_where_used_as_field_type - general clean up - remove fields allocation find_where_used_as_top_name - use top interner instead of iterating everything - return Option of identifier instead of iterator Co-authored-by: Flavian Desverne <flavian.dsv@gmail.com>
...a-fmt/tests/text_document_completion/scenarios/referential_actions_in_progress_2/result.json
Show resolved
Hide resolved
string interning seems to not support datasources
added find_source
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.
Great work 👍
* companion pr to prisma/prisma-engines#4934 * test references in LT --------- Co-authored-by: Serhii Tatarintsev <tatarintsev@prisma.io>
I left details on testing this locally in the language-tools pr here
closes https://github.com/prisma/team-orm/issues/1201
Screen.Recording.2024-06-26.at.14.29.33.mov
Screen.Recording.2024-06-26.at.14.27.22.mov
closes https://github.com/prisma/team-orm/issues/1196
Screen.Recording.2024-06-26.at.14.22.53.mov
These only apply to models and views. Composite types have uniques, indices, so on defined in their containing model / view.
closes https://github.com/prisma/team-orm/issues/1200