refactor(lsp): use deno_ast and cache swc ASTs #11780
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…at would be better.
…dency and it may be added to the standard library in the future.
dsherret
commented
Aug 19, 2021
| @@ -91,14 +89,10 @@ pub async fn format( | |||
|
|
|||
| /// Formats markdown (using https://github.com/dprint/dprint-plugin-markdown) and its code blocks | |||
| /// (ts/tsx, js/jsx). | |||
| fn format_markdown( | |||
| file_text: &str, | |||
| ts_config: dprint_plugin_typescript::configuration::Configuration, | |||
There was a problem hiding this comment.
Config objects were being needlessly passed around.
dsherret
commented
Aug 19, 2021
|
@kitsonk this is in draft because of the failing build due to problems I encountered integrating, but it's good enough for a review. (see the PR description—I shouldn't have updated swc in the crates so that I wouldn't need to bump deno_doc) |
kitsonk
reviewed
Aug 20, 2021
…ppened in refactoring.
dsherret
commented
Sep 6, 2021
| @@ -488,7 +488,7 @@ fn syntax_error() { | |||
| Some(vec![("NO_COLOR".to_owned(), "1".to_owned())]), | |||
| false, | |||
| ); | |||
| assert!(out.ends_with("parse error: Expected ';', '}' or <eof> at 1:7\n2\n")); | |||
| assert!(out.ends_with("parse error: Expected ';', '}' or <eof> at 1:8\n2\n")); | |||
There was a problem hiding this comment.
The column in parse errors is now 1-indexed for display (matches line number and what is shown in editors).
| @@ -52,7 +52,7 @@ pub struct File { | |||
| /// The resolved media type for the file. | |||
| pub media_type: MediaType, | |||
| /// The source of the file as a string. | |||
| pub source: String, | |||
| pub source: Arc<String>, | |||
There was a problem hiding this comment.
This PR should reduce memory usage and clones by using Arc<String> for the file text as much as possible (I may have still missed some areas). I didn't profile this or anything, but seems like something we should proactively do and is what's done in deno_ast.
bartlomieju
reviewed
Sep 6, 2021
| // reuse by other processes | ||
| let (_, lint_diagnostics) = | ||
| linter.lint(specifier.to_string(), source_code.to_string())?; | ||
| let lint_diagnostics = linter.lint_with_ast(parsed_source); |
bartlomieju
reviewed
Sep 6, 2021
bartlomieju
reviewed
Sep 6, 2021
Comment on lines
-386
to
-397
| let source = language_server | ||
| .get_text_content(specifier) | ||
| .ok_or_else(|| anyhow!("Missing text content: {}", specifier))?; | ||
| let media_type = language_server | ||
| .get_media_type(specifier) | ||
| .ok_or_else(|| anyhow!("Missing media type: {}", specifier))?; | ||
| // we swallow parsed errors, as they are meaningless here. | ||
| // TODO(@kitsonk) consider caching previous code_lens results to return if | ||
| // there is a parse error to avoid issues of lenses popping in and out | ||
| if let Ok(parsed_module) = | ||
| analysis::parse_module(specifier, &source, &media_type) | ||
| { |
bartlomieju
reviewed
Sep 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change integrates
deno_astand lazily caches/reuses swc AST in the language server.I've yet to come up with a benchmark I think is worthwhile to have... given that swc is already so fast this change is only noticable with very large files. I would say this isn't so much a performance improvement, but rather just a refactoring.
Closes #11345
Closes #11854