Skip to content

Commit

Permalink
[move][move-ide][move-compiler] Swap FilesSourceText to `MappedFile…
Browse files Browse the repository at this point in the history
…s` in most places (#18270)

## Description 

This swaps out `FilesSourceText` for `MappedFiles` whenever possible,
along with the following changes:

- The internal implementation of `MappedFiles` is a little more
complicated to support some `move-analyzer` behaviors.
- `MappedFiles` now lives in `move-compiler/shared/files.rs` to reflect
it being standalone from the diagnostics.
- This also updates the `move-analyzer` to start using the `MappedFiles`
instead of maintaining a separate file state.
- The `Position` form had its fields reverted to non-`pub` with getters
to better-clarify the value being returned.

## Test plan 

All tests still run.

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
  • Loading branch information
cgswords authored Jun 17, 2024
1 parent ff05210 commit 842cd17
Show file tree
Hide file tree
Showing 22 changed files with 606 additions and 493 deletions.
5 changes: 3 additions & 2 deletions crates/sui-move-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ use move_binary_format::{
use move_bytecode_utils::{layout::SerdeLayoutBuilder, module_cache::GetModule};
use move_compiler::{
compiled_unit::AnnotatedCompiledModule,
diagnostics::{report_diagnostics_to_buffer, report_warnings, Diagnostics, FilesSourceText},
diagnostics::{report_diagnostics_to_buffer, report_warnings, Diagnostics},
editions::Edition,
linters::LINT_WARNING_PREFIX,
shared::files::MappedFiles,
};
use move_core_types::{
account_address::AccountAddress,
Expand Down Expand Up @@ -184,7 +185,7 @@ impl BuildConfig {

/// There may be additional information that needs to be displayed after diagnostics are reported
/// (optionally report diagnostics themselves if files argument is provided).
pub fn decorate_warnings(warning_diags: Diagnostics, files: Option<&FilesSourceText>) {
pub fn decorate_warnings(warning_diags: Diagnostics, files: Option<&MappedFiles>) {
let any_linter_warnings = warning_diags.any_with_prefix(LINT_WARNING_PREFIX);
let (filtered_diags_num, unique) =
warning_diags.filtered_source_diags_with_prefix(LINT_WARNING_PREFIX);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,15 @@ use crate::{
expansion_mod_ident_to_map_key, type_def_loc, DefInfo, DefLoc, LocalDef, ModuleDefs,
UseDef, UseDefMap, UseLoc,
},
utils::{get_start_position_opt, ignored_function, to_lsp_position},
utils::{ignored_function, loc_start_to_lsp_position_opt},
};

use move_command_line_common::files::FileHash;
use move_compiler::{
diagnostics::{self as diag, PositionInfo},
diagnostics as diag,
expansion::ast::{self as E, ModuleIdent},
naming::ast as N,
parser::ast as P,
shared::{ide::MacroCallInfo, Identifier, Name},
shared::{files::MappedFiles, ide::MacroCallInfo, Identifier, Name},
typing::{
ast as T,
visitor::{LValueKind, TypingVisitorContext},
Expand All @@ -26,21 +25,18 @@ use move_compiler::{
use move_ir_types::location::{sp, Loc};
use move_symbol_pool::Symbol;

use codespan_reporting::files::SimpleFiles;
use im::OrdMap;
use lsp_types::Position;
use std::collections::{BTreeMap, BTreeSet, HashMap};
use std::collections::{BTreeMap, BTreeSet};

/// Data used during anlysis over typed AST
pub struct TypingAnalysisContext<'a> {
/// Outermost definitions in a module (structs, consts, functions), keyd on a ModuleIdent
/// string so that we can access it regardless of the ModuleIdent representation
/// (e.g., in the parsing AST or in the typing AST)
pub mod_outer_defs: &'a BTreeMap<String, ModuleDefs>,
/// A mapping from file names to file content (used to obtain source file locations)
pub files: &'a SimpleFiles<Symbol, String>,
/// A mapping from file hashes to file IDs (used to obtain source file locations)
pub file_id_mapping: &'a HashMap<FileHash, usize>,
/// Mapped file information for translating locations into positions
pub files: &'a MappedFiles,
pub references: &'a mut BTreeMap<DefLoc, BTreeSet<UseLoc>>,
/// Additional information about definitions
pub def_info: &'a mut BTreeMap<DefLoc, DefInfo>,
Expand All @@ -66,13 +62,13 @@ impl TypingAnalysisContext<'_> {
/// Returns the `lsp_types::Position` start for a location, but may fail if we didn't see the
/// definition already.
fn lsp_start_position_opt(&self, loc: &Loc) -> Option<lsp_types::Position> {
get_start_position_opt(loc, self.files(), self.file_mapping())
loc_start_to_lsp_position_opt(self.files, loc)
}

/// Returns the `lsp_types::Position` start for a location, but may fail if we didn't see the
/// definition already. This should only be used on things we already indexed.
fn lsp_start_position(&self, loc: &Loc) -> lsp_types::Position {
to_lsp_position(self.start_position(loc))
loc_start_to_lsp_position_opt(self.files, loc).unwrap()
}

fn reset_for_module_member(&mut self) {
Expand Down Expand Up @@ -289,8 +285,7 @@ impl TypingAnalysisContext<'_> {
let result = add_fun_use_def(
fun_def_name,
self.mod_outer_defs,
self.files(),
self.file_mapping(),
self.files,
mod_ident_str,
mod_defs,
use_name,
Expand Down Expand Up @@ -345,8 +340,7 @@ impl TypingAnalysisContext<'_> {
let mut refs = std::mem::take(self.references);
add_struct_use_def(
self.mod_outer_defs,
self.files(),
self.file_mapping(),
self.files,
mod_ident_str,
mod_defs,
&use_name.value(),
Expand Down Expand Up @@ -607,7 +601,7 @@ impl<'a> TypingVisitorContext for TypingAnalysisContext<'a> {
}
let loc = function_name.loc();
// first, enter self-definition for function name (unwrap safe - done when inserting def)
let name_start = to_lsp_position(self.start_position(&loc));
let name_start = self.lsp_start_position(&loc);
let fun_info = self
.def_info
.get(&DefLoc::new(loc.file_hash(), name_start))
Expand Down Expand Up @@ -915,15 +909,3 @@ impl<'a> TypingVisitorContext for TypingAnalysisContext<'a> {
}
}
}

impl diag::PositionInfo for TypingAnalysisContext<'_> {
type FileContents = String;

fn files(&self) -> &SimpleFiles<Symbol, Self::FileContents> {
self.files
}

fn file_mapping(&self) -> &HashMap<FileHash, move_compiler::diagnostics::FileId> {
self.file_id_mapping
}
}
23 changes: 13 additions & 10 deletions external-crates/move/crates/move-analyzer/src/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,7 @@ fn dot(symbols: &Symbols, use_fpath: &Path, position: &Position) -> Vec<Completi
let Some(fhash) = symbols.file_hash(use_fpath) else {
return completions;
};
let Some(byte_idx) =
utils::get_byte_idx(*position, fhash, &symbols.files, &symbols.file_id_mapping)
else {
let Some(byte_idx) = utils::lsp_position_to_byte_index(&symbols.files, fhash, position) else {
return completions;
};
let loc = Loc::new(fhash, byte_idx, byte_idx);
Expand Down Expand Up @@ -547,17 +545,17 @@ fn completion_items(pos: Position, path: &Path, symbols: &Symbols) -> Vec<Comple
let Some(fhash) = symbols.file_hash(path) else {
return items;
};
let Some(file_id) = symbols.file_id_mapping.get(&fhash) else {
let Some(file_id) = symbols.files.file_mapping().get(&fhash) else {
return items;
};
let Ok(file) = symbols.files.get(*file_id) else {
let Ok(file) = symbols.files.files().get(*file_id) else {
return items;
};

let buffer = file.source().clone();
if !buffer.is_empty() {
let mut only_custom_items = false;
let cursor = get_cursor_token(buffer.as_str(), &pos);
let cursor = get_cursor_token(&buffer, &pos);
match cursor {
Some(Tok::Colon) => {
items.extend_from_slice(&primitive_types());
Expand Down Expand Up @@ -585,7 +583,7 @@ fn completion_items(pos: Position, path: &Path, symbols: &Symbols) -> Vec<Comple
// offer them context-specific autocompletion items as well as
// Move's keywords, operators, and builtins.
let (custom_items, custom) =
context_specific_no_trigger(symbols, path, buffer.as_str(), &pos);
context_specific_no_trigger(symbols, path, &buffer, &pos);
only_custom_items = custom;
items.extend_from_slice(&custom_items);
if !only_custom_items {
Expand All @@ -595,7 +593,7 @@ fn completion_items(pos: Position, path: &Path, symbols: &Symbols) -> Vec<Comple
}
}
if !only_custom_items {
let identifiers = identifiers(buffer.as_str(), symbols, path);
let identifiers = identifiers(&buffer, symbols, path);
items.extend_from_slice(&identifiers);
}
} else {
Expand Down Expand Up @@ -682,8 +680,13 @@ fn completion_dot_test() {
character: 10,
};
let items = completion_items(pos, &cpath, &symbols);
let loc = format!("{:?} (line: {}, col: {}", cpath, pos.line, pos.character);
assert!(items.len() == 3, "wrong number of items at {}", loc.clone());
let loc = format!("{:?} (line: {}, col: {})", cpath, pos.line, pos.character);
assert!(
items.len() == 3,
"wrong number of items at {} (3 vs {})",
loc.clone(),
items.len()
);
validate_item(
loc.clone(),
&items,
Expand Down
33 changes: 11 additions & 22 deletions external-crates/move/crates/move-analyzer/src/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
// Copyright (c) The Move Contributors
// SPDX-License-Identifier: Apache-2.0

use crate::utils::get_loc;
use codespan_reporting::{diagnostic::Severity, files::SimpleFiles};
use crate::utils::{loc_end_to_lsp_position_opt, loc_start_to_lsp_position_opt};
use codespan_reporting::diagnostic::Severity;
use lsp_types::{Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, Location, Range};
use move_command_line_common::files::FileHash;
use move_compiler::shared::files::MappedFiles;
use move_ir_types::location::Loc;
use move_symbol_pool::Symbol;
use std::{
collections::{BTreeMap, HashMap},
path::PathBuf,
};
use std::{collections::BTreeMap, path::PathBuf};
use url::Url;

/// Converts diagnostics from the codespan format to the format understood by the language server.
Expand All @@ -22,15 +19,13 @@ pub fn lsp_diagnostics(
Vec<(Loc, String)>,
Vec<String>,
)>,
files: &SimpleFiles<Symbol, String>,
file_id_mapping: &HashMap<FileHash, usize>,
file_name_mapping: &BTreeMap<FileHash, PathBuf>,
files: &MappedFiles,
) -> BTreeMap<PathBuf, Vec<Diagnostic>> {
let mut lsp_diagnostics = BTreeMap::new();
for (s, _, (loc, msg), labels, _) in diagnostics {
let fpath = file_name_mapping.get(&loc.file_hash()).unwrap();
if let Some(start) = get_loc(&loc.file_hash(), loc.start(), files, file_id_mapping) {
if let Some(end) = get_loc(&loc.file_hash(), loc.end(), files, file_id_mapping) {
let fpath = files.file_path(&loc.file_hash());
if let Some(start) = loc_start_to_lsp_position_opt(files, loc) {
if let Some(end) = loc_end_to_lsp_position_opt(files, loc) {
let range = Range::new(start, end);
let related_info_opt = if labels.is_empty() {
None
Expand All @@ -39,15 +34,9 @@ pub fn lsp_diagnostics(
labels
.iter()
.filter_map(|(lloc, lmsg)| {
let lstart = get_loc(
&lloc.file_hash(),
lloc.start(),
files,
file_id_mapping,
)?;
let lend =
get_loc(&lloc.file_hash(), lloc.end(), files, file_id_mapping)?;
let lpath = file_name_mapping.get(&lloc.file_hash()).unwrap();
let lstart = loc_start_to_lsp_position_opt(files, lloc)?;
let lend = loc_end_to_lsp_position_opt(files, lloc)?;
let lpath = files.file_path(&lloc.file_hash());
let lpos = Location::new(
Url::from_file_path(lpath).unwrap(),
Range::new(lstart, lend),
Expand Down
Loading

0 comments on commit 842cd17

Please sign in to comment.