Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Don't insert imports when method calls are created through searcher #1209

Merged
merged 18 commits into from
Mar 8, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ read the notes of the `Enso 2.0.0-alpha.1` release.
dealing with incompatible node metadata (that store node visual positions, history of picked
searcher suggestions, etc.). This will allow IDE to correctly open projects that were created
using a different IDE version and prevent unnecessary lose of metadata information.
- [Not adding spurious imports][1209]. Fixed cases when IDE was adding unnecessary library
imports when selecting hints from node searcher. This makes the generated textual code easier to
read and reduces likelihood of accidental name collision.

#### EnsoGL
- New multi-camera management system, allowing the same shape systems be rendered on different
Expand Down Expand Up @@ -60,6 +63,8 @@ read the notes of the `Enso 2.0.0-alpha.1` release.
[1190]: https://github.com/enso-org/ide/pull/1190
[1187]: https://github.com/enso-org/ide/pull/1187
[1214]: https://github.com/enso-org/ide/pull/1214
[1067]: https://github.com/enso-org/ide/issues/1067
[1209]: https://github.com/enso-org/ide/pull/1209
<br/>


Expand Down
10 changes: 0 additions & 10 deletions src/rust/ide/src/controller/graph/executed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,30 +325,20 @@ pub mod tests {

impl MockData {
pub fn controller(&self) -> Handle {
info!(DefaultDebugLogger::new("Test"),"-- 1");
let parser = parser::Parser::new_or_panic();
info!(DefaultDebugLogger::new("Test"),"-- 2");
let module = self.module.plain(&parser);
info!(DefaultDebugLogger::new("Test"),"-- 3");
let method = self.graph.method();
info!(DefaultDebugLogger::new("Test"),"-- 4");
let mut project = model::project::MockAPI::new();
info!(DefaultDebugLogger::new("Test"),"-- 5");
let ctx = Rc::new(self.ctx.create());
info!(DefaultDebugLogger::new("Test"),"-- 6");
model::project::test::expect_name(&mut project,test::mock::data::PROJECT_NAME);
model::project::test::expect_parser(&mut project,&parser);
model::project::test::expect_module(&mut project,module);
model::project::test::expect_execution_ctx(&mut project,ctx);
info!(DefaultDebugLogger::new("Test"),"-- 7");
// Root ID is needed to generate module path used to get the module.
model::project::test::expect_root_id(&mut project,crate::test::mock::data::ROOT_ID);
// Both graph controllers need suggestion DB to provide context to their span trees.
info!(DefaultDebugLogger::new("Test"),"-- 8");
let suggestion_db = self.graph.suggestion_db();
info!(DefaultDebugLogger::new("Test"),"-- 9");
model::project::test::expect_suggestion_db(&mut project,suggestion_db);
info!(DefaultDebugLogger::new("Test"),"-- 10");
let project = Rc::new(project);
Handle::new(Logger::new("test"),project.clone_ref(),method).boxed_local().expect_ok()
}
Expand Down
91 changes: 62 additions & 29 deletions src/rust/ide/src/controller/searcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::double_representation::node::NodeInfo;
use crate::model::module::MethodId;
use crate::model::module::NodeMetadata;
use crate::model::module::Position;
use crate::model::suggestion_database::entry::CodeToInsert;
use crate::notification;

use data::text::TextLocation;
Expand Down Expand Up @@ -362,8 +363,13 @@ impl FragmentAddedByPickingSuggestion {
/// Check if the picked fragment is still unmodified by user.
fn is_still_unmodified
(&self, input:&ParsedInput, current_module:&QualifiedName) -> bool {
let expected_code = &self.picked_suggestion.code_to_insert(Some(current_module));
input.completed_fragment(self.id).contains(expected_code)
let expected = self.code_to_insert(current_module,&None);
input.completed_fragment(self.id).contains(&expected.code)
}

fn code_to_insert(&self, current_module:&QualifiedName, this_node:&Option<ThisNode>) -> CodeToInsert {
let generate_this = self.id != CompletedFragmentId::Function || this_node.is_none();
self.picked_suggestion.code_to_insert(Some(&current_module),generate_this)
}
}

Expand Down Expand Up @@ -536,14 +542,8 @@ impl Searcher {
///
/// Code depends on the location, as the first fragment can introduce `this` variable access,
/// and then we don't want to put any module name.
fn code_to_insert
(&self, suggestion:&action::Suggestion, loc:CompletedFragmentId) -> String {
let current_module = self.module_qualified_name();
if loc == CompletedFragmentId::Function && self.this_arg.is_some() {
suggestion.code_to_insert_skip_module()
} else {
suggestion.code_to_insert(Some(&current_module))
}
fn code_to_insert(&self, fragment:&FragmentAddedByPickingSuggestion) -> CodeToInsert {
fragment.code_to_insert(&self.module_qualified_name(), self.this_arg.as_ref())
}

/// Pick a completion suggestion.
Expand All @@ -555,9 +555,9 @@ impl Searcher {
(&self, picked_suggestion: action::Suggestion) -> FallibleResult<String> {
info!(self.logger, "Picking suggestion: {picked_suggestion:?}");
let id = self.data.borrow().input.next_completion_id();
let code_to_insert = self.code_to_insert(&picked_suggestion,id);
let added_ast = self.parser.parse_line(&code_to_insert)?;
let picked_completion = FragmentAddedByPickingSuggestion {id,picked_suggestion};
let code_to_insert = self.code_to_insert(&picked_completion).code;
let added_ast = self.parser.parse_line(&code_to_insert)?;
let pattern_offset = self.data.borrow().input.pattern_offset;
let new_expression = match self.data.borrow_mut().input.expression.take() {
None => {
Expand Down Expand Up @@ -730,10 +730,11 @@ impl Searcher {
);
}


fn add_required_imports(&self) -> FallibleResult {
let data_borrowed = self.data.borrow();
let fragments = data_borrowed.fragments_added_by_picking.iter();
let imports = fragments.map(|frag| &frag.picked_suggestion.module);
let imports = fragments.map(|frag| self.code_to_insert(frag).imports).flatten();
let mut module = self.module();
let here = self.module_qualified_name();
for import in imports {
Expand Down Expand Up @@ -994,6 +995,7 @@ pub mod test {
use super::*;

use crate::executor::test_utils::TestWithLocalPoolExecutor;
use crate::model::SuggestionDatabase;
use crate::model::suggestion_database::entry::Argument;
use crate::model::suggestion_database::entry::Kind;
use crate::model::suggestion_database::entry::Scope;
Expand All @@ -1005,7 +1007,6 @@ pub mod test {
use json_rpc::expect_call;
use utils::test::traits::*;
use enso_protocol::language_server::SuggestionId;
use crate::model::SuggestionDatabase;

pub fn completion_response(results:&[SuggestionId]) -> language_server::response::Completion {
language_server::response::Completion {
Expand Down Expand Up @@ -1067,39 +1068,26 @@ pub mod test {
entry3 : action::Suggestion,
entry4 : action::Suggestion,
entry9 : action::Suggestion,
entry10 : action::Suggestion,
}

impl Fixture {
fn new_custom<F>(client_setup:F) -> Self
where F : FnOnce(&mut MockData,&mut language_server::MockClient) {
info!(DefaultDebugLogger::new("Test"),"1");
let test = TestWithLocalPoolExecutor::set_up();
info!(DefaultDebugLogger::new("Test"),"2");
let mut data = MockData::default();
info!(DefaultDebugLogger::new("Test"),"3");
let mut client = language_server::MockClient::default();
info!(DefaultDebugLogger::new("Test"),"4");
client.require_all_calls();
info!(DefaultDebugLogger::new("Test"),"5");
client_setup(&mut data,&mut client);
info!(DefaultDebugLogger::new("Test"),"6");
let end_of_code = TextLocation::at_document_end(&data.graph.module.code);
let code_range = TextLocation::at_document_begin()..=end_of_code;
info!(DefaultDebugLogger::new("Test"),"7");
let graph = data.graph.controller();
info!(DefaultDebugLogger::new("Test"),"8");
let node = &graph.graph().nodes().unwrap()[0];
info!(DefaultDebugLogger::new("Test"),"9");
let this = ThisNode::new(vec![node.info.id()],&graph.graph());
info!(DefaultDebugLogger::new("Test"),"10");
let this = data.selected_node.and_option(this);
info!(DefaultDebugLogger::new("Test"),"11");
let logger = Logger::new("Searcher");// new_empty
info!(DefaultDebugLogger::new("Test"),"12");
let database = Rc::new(SuggestionDatabase::new_empty(&logger));
info!(DefaultDebugLogger::new("Test"),"13");
let module_name = QualifiedName::from_segments(PROJECT_NAME, &[MODULE_NAME]).unwrap();
info!(DefaultDebugLogger::new("Test"),"14");
let searcher = Searcher {
graph,logger,database,
data : default(),
Expand Down Expand Up @@ -1190,6 +1178,12 @@ pub mod test {
],
..entry1.clone()
};
let entry10 = model::suggestion_database::Entry {
name : "testFunction3".to_string(),
module : "Test.Other".to_owned().try_into().unwrap(),
scope : Scope::Everywhere,
..entry9.clone()
};

searcher.database.put_entry(1,entry1);
let entry1 = searcher.database.lookup(1).unwrap();
Expand All @@ -1201,7 +1195,9 @@ pub mod test {
let entry4 = searcher.database.lookup(4).unwrap();
searcher.database.put_entry(9,entry9);
let entry9 = searcher.database.lookup(9).unwrap();
Fixture{data,test,searcher,entry1,entry2,entry3,entry4,entry9}
searcher.database.put_entry(10,entry10);
let entry10 = searcher.database.lookup(10).unwrap();
Fixture{data,test,searcher,entry1,entry2,entry3,entry4,entry9,entry10}
}

fn new() -> Self {
Expand Down Expand Up @@ -1601,6 +1597,43 @@ pub mod test {
}
}

#[wasm_bindgen_test]
fn adding_imports_with_nodes() {
fn expect_inserted_import_for(entry:&action::Suggestion, expected_import:Vec<&QualifiedName>) {
let Fixture{test:_test,mut searcher,..} = Fixture::new();
let module = searcher.graph.graph().module.clone_ref();
let parser = searcher.parser.clone_ref();

let picked_method = FragmentAddedByPickingSuggestion {
id : CompletedFragmentId::Function,
picked_suggestion : entry.clone(),
};
with(searcher.data.borrow_mut(), |mut data| {
data.fragments_added_by_picking.push(picked_method);
data.input = ParsedInput::new(entry.name.to_string(),&parser).unwrap();
});

// Add new node.
searcher.mode = Immutable(Mode::NewNode {position:None});
searcher.commit_node().unwrap();

let module_info = module.info();
let imported_names = module_info.iter_imports()
.map(|import| import.qualified_name().unwrap())
.collect_vec();

let expected_import = expected_import.into_iter().cloned().collect_vec();
assert_eq!(imported_names,expected_import);
}

let Fixture{entry1,entry2,entry3,entry4,entry10,..} = Fixture::new();
expect_inserted_import_for(&entry1, vec![]);
expect_inserted_import_for(&entry2, vec![]);
expect_inserted_import_for(&entry3, vec![]);
expect_inserted_import_for(&entry4, vec![&entry4.module]);
expect_inserted_import_for(&entry10, vec![&entry10.module]);
}

#[wasm_bindgen_test]
fn committing_node() {
let Fixture{test:_test,mut searcher,entry4,..} = Fixture::new();
Expand Down
8 changes: 7 additions & 1 deletion src/rust/ide/src/double_representation/identifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,12 @@ pub struct NotReferentName(String);

// === Definition ===

#[derive(Clone,Debug,Display,Shrinkwrap,PartialEq,Eq,Hash)]
/// The name segment is a string that starts with an upper-cased character.
///
/// It is used for naming modules, module path segments and projects.
///
/// This value corresponds to contents of the `Cons` AST shape.
#[derive(Clone,Debug,Display,Shrinkwrap,PartialEq,Eq,PartialOrd,Ord,Hash)]
pub struct ReferentName(String);

impl ReferentName {
Expand Down Expand Up @@ -245,6 +245,12 @@ impl From<ReferentName> for String {
}
}

impl From<&ReferentName> for String {
fn from(name:&ReferentName) -> Self {
name.0.clone()
}
}

impl PartialEq<String> for ReferentName {
fn eq(&self, other:&String) -> bool {
&self.0 == other
Expand Down
9 changes: 7 additions & 2 deletions src/rust/ide/src/double_representation/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub struct NotDirectChild(ast::Crumbs);
/// Example: `["Parent","Module_Name"]`
///
/// Includes segments of module path but *NOT* the project name (see: `QualifiedName`).
#[derive(Clone,Debug,Shrinkwrap,PartialEq,Eq,Hash)]
#[derive(Clone,Debug,Shrinkwrap,PartialEq,Eq,PartialOrd,Ord,Hash)]
pub struct Id {
/// The vector is non-empty.
segments:Vec<ReferentName>
Expand Down Expand Up @@ -122,6 +122,11 @@ impl Id {
// Safe, as the invariant guarantees segments to be non-empty.
self.segments.iter().last().unwrap()
}

/// Access module name segments.
pub fn segments(&self) -> &Vec<ReferentName> {
&self.segments
}
}


Expand All @@ -139,7 +144,7 @@ impl Id {
///
/// See https://dev.enso.org/docs/distribution/packaging.html for more information about the
/// package structure.
#[derive(Clone,Debug,Deserialize,Serialize,PartialEq,Eq,Hash)]
#[derive(Clone,Debug,Deserialize,Serialize,PartialEq,Eq,PartialOrd,Ord,Hash)]
#[serde(into="String")]
#[serde(try_from="String")]
pub struct QualifiedName {
Expand Down
12 changes: 12 additions & 0 deletions src/rust/ide/src/double_representation/tp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ impl QualifiedName {
QualifiedName{project_name,module_segments,name}
}

/// Create from the module's qualified name.
pub fn new_module_member(module:module::QualifiedName, name:String) -> Self {
let module::QualifiedName{project_name,id} = module;
let module_segments = id.into_segments();
QualifiedName{project_name,module_segments,name}
}

/// Create from a text representation. May fail if the text is not valid Qualified name of any
/// type.
pub fn from_text(text:impl Str) -> FallibleResult<Self> {
Expand All @@ -74,6 +81,11 @@ impl QualifiedName {
}
Ok(QualifiedName {project_name,module_segments,name})
}

/// Check if the name is defined directly in the given module.
pub fn in_module(&self, module:&module::QualifiedName) -> bool {
self.project_name == module.project_name && &self.module_segments == module.id.segments()
}
}


Expand Down
3 changes: 2 additions & 1 deletion src/rust/ide/src/ide/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1324,7 +1324,8 @@ impl DataProviderForView {
suggestion_database::entry::Kind::Local => "Local variable",
suggestion_database::entry::Kind::Method => "Method",
};
format!("{} `{}`\n\nNo documentation available", title,suggestion.code_to_insert(None))
let code = suggestion.code_to_insert(None,true).code;
format!("{} `{}`\n\nNo documentation available", title,code)
}
}

Expand Down
Loading