From ecedcd502a73c267e83c219d1fec7524f03844a3 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Tue, 13 Dec 2022 14:19:33 +0100 Subject: [PATCH 1/7] Initial fix --- lsp/nls/src/requests/completion.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 31bc438331..cbd8975c0d 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -164,7 +164,10 @@ fn find_fields_from_contract( ) -> Option> { let item = linearization.get_item(id)?; match &item.meta { - Some(meta_value) => Some(find_fields_from_meta_value(meta_value, path)), + Some(meta_value) => { + let result = find_fields_from_meta_value(meta_value, path); + (!result.is_empty()).then_some(result) + } None => match item.kind { TermKind::Declaration(_, _, ValueState::Known(new_id)) | TermKind::Usage(UsageState::Resolved(new_id)) => { From 21f54fe2021010bf44420e22c8ead4803340af68 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Thu, 15 Dec 2022 14:02:38 +0100 Subject: [PATCH 2/7] Aggregate contract & termkind --- lsp/nls/src/requests/completion.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index cbd8975c0d..58d1edabc8 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -164,10 +164,7 @@ fn find_fields_from_contract( ) -> Option> { let item = linearization.get_item(id)?; match &item.meta { - Some(meta_value) => { - let result = find_fields_from_meta_value(meta_value, path); - (!result.is_empty()).then_some(result) - } + Some(meta_value) => Some(find_fields_from_meta_value(meta_value, path)), None => match item.kind { TermKind::Declaration(_, _, ValueState::Known(new_id)) | TermKind::Usage(UsageState::Resolved(new_id)) => { @@ -331,9 +328,15 @@ fn collect_record_info( // Get record fields from static type info (_, Types(TypeF::Record(rrows))) => find_fields_from_type(&rrows, path), (TermKind::Declaration(_, _, ValueState::Known(body_id)), _) => { - find_fields_from_contract(linearization, *body_id, path) - .or_else(|| find_fields_from_term_kind(linearization, id, path)) - .unwrap_or_default() + // The path is mutable, so the first case would consume the path + // so we have to clone it so that it can be correctly used for the second case. + let mut p = path.clone(); + let mut fst = find_fields_from_contract(linearization, *body_id, path) + .unwrap_or_default(); + let snd = + find_fields_from_term_kind(linearization, id, &mut p).unwrap_or_default(); + fst.extend(snd); + fst } ( TermKind::RecordField { From 126e4918035ba612905efb0d3f90fb04d6d681d1 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Fri, 16 Dec 2022 16:16:52 +0100 Subject: [PATCH 3/7] Add tests --- lsp/nls/src/requests/completion.rs | 129 ++++++++++++++++++++--------- 1 file changed, 92 insertions(+), 37 deletions(-) diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 58d1edabc8..90d3fe0c1b 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -333,8 +333,8 @@ fn collect_record_info( let mut p = path.clone(); let mut fst = find_fields_from_contract(linearization, *body_id, path) .unwrap_or_default(); - let snd = - find_fields_from_term_kind(linearization, id, &mut p).unwrap_or_default(); + let snd = find_fields_from_term_kind(linearization, *body_id, &mut p) + .unwrap_or_default(); fst.extend(snd); fst } @@ -478,9 +478,34 @@ mod tests { use super::*; use crate::linearization::Environment; use codespan::Files; - use nickel_lang::position::TermPos; + use nickel_lang::{position::TermPos, term::MergePriority}; use std::collections::{HashMap, HashSet}; + fn make_lin_item( + id: ItemId, + kind: TermKind, + meta: Option, + ) -> LinearizationItem { + LinearizationItem { + env: Environment::new(), + id, + pos: TermPos::None, + ty: Types(TypeF::Dyn), + kind, + meta, + } + } + + fn make_completed(linearization: Vec>) -> Completed { + let id_to_index: HashMap<_, _> = linearization + .iter() + .map(|item| item.id) + .enumerate() + .map(|(index, id)| (id, index)) + .collect(); + Completed::new(linearization, id_to_index) + } + #[test] fn test_get_identifier_path() { let tests = [ @@ -609,26 +634,6 @@ mod tests { #[test] fn test_find_record_fields() { - fn make_linearization_item(id: ItemId, kind: TermKind) -> LinearizationItem { - LinearizationItem { - env: Environment::new(), - id, - pos: TermPos::None, - ty: Types(TypeF::Dyn), - kind, - meta: None, - } - } - fn make_completed(linearization: Vec>) -> Completed { - let id_to_index: HashMap<_, _> = linearization - .iter() - .map(|item| item.id) - .enumerate() - .map(|(index, id)| (id, index)) - .collect(); - Completed::new(linearization, id_to_index) - } - // ids is an array of the ids from this linearization // which would give the expected output fn single_case( @@ -654,34 +659,38 @@ mod tests { let mut files = Files::new(); let file_id = files.add("test", "test"); - let a = make_linearization_item( + let a = make_lin_item( ItemId { file_id, index: 0 }, TermKind::Declaration( Ident::from("a"), vec![ItemId { file_id, index: 3 }], ValueState::Known(ItemId { file_id, index: 1 }), ), + None, ); - let b = make_linearization_item( + let b = make_lin_item( ItemId { file_id, index: 1 }, TermKind::Record(HashMap::from([ (Ident::from("foo"), ItemId { file_id, index: 2 }), (Ident::from("bar"), ItemId { file_id, index: 2 }), (Ident::from("baz"), ItemId { file_id, index: 2 }), ])), + None, ); - let c = make_linearization_item(ItemId { file_id, index: 2 }, TermKind::Structure); - let d = make_linearization_item( + let c = make_lin_item(ItemId { file_id, index: 2 }, TermKind::Structure, None); + let d = make_lin_item( ItemId { file_id, index: 3 }, TermKind::Declaration( Ident::from("d"), Vec::new(), ValueState::Known(ItemId { file_id, index: 4 }), ), + None, ); - let e = make_linearization_item( + let e = make_lin_item( ItemId { file_id, index: 4 }, TermKind::Usage(UsageState::Resolved(ItemId { file_id, index: 0 })), + None, ); let linearization = vec![a, b, c, d, e]; let expected = vec![ @@ -695,15 +704,16 @@ mod tests { expected, ); - let a = make_linearization_item( + let a = make_lin_item( ItemId { file_id, index: 0 }, TermKind::Declaration( Ident::from("a"), Vec::new(), ValueState::Known(ItemId { file_id, index: 1 }), ), + None, ); - let b = make_linearization_item( + let b = make_lin_item( ItemId { file_id, index: 1 }, TermKind::Record(HashMap::from([ (Ident::from("one"), ItemId { file_id, index: 2 }), @@ -711,43 +721,50 @@ mod tests { (Ident::from("three"), ItemId { file_id, index: 2 }), (Ident::from("four"), ItemId { file_id, index: 2 }), ])), + None, ); - let c = make_linearization_item(ItemId { file_id, index: 2 }, TermKind::Structure); - let d = make_linearization_item( + let c = make_lin_item(ItemId { file_id, index: 2 }, TermKind::Structure, None); + let d = make_lin_item( ItemId { file_id, index: 3 }, TermKind::Declaration( Ident::from("d"), Vec::new(), ValueState::Known(ItemId { file_id, index: 13 }), ), + None, ); - let e = make_linearization_item( + let e = make_lin_item( ItemId { file_id, index: 4 }, TermKind::Declaration( Ident::from("e"), Vec::new(), ValueState::Known(ItemId { file_id, index: 14 }), ), + None, ); - let f = make_linearization_item( + let f = make_lin_item( ItemId { file_id, index: 5 }, TermKind::Declaration( Ident::from("f"), Vec::new(), ValueState::Known(ItemId { file_id, index: 15 }), ), + None, ); - let g = make_linearization_item( + let g = make_lin_item( ItemId { file_id, index: 13 }, TermKind::Usage(UsageState::Resolved(ItemId { file_id, index: 0 })), + None, ); - let h = make_linearization_item( + let h = make_lin_item( ItemId { file_id, index: 14 }, TermKind::Usage(UsageState::Resolved(ItemId { file_id, index: 3 })), + None, ); - let i = make_linearization_item( + let i = make_lin_item( ItemId { file_id, index: 15 }, TermKind::Usage(UsageState::Resolved(ItemId { file_id, index: 4 })), + None, ); let expected = vec![ IdentWithType::from("one"), @@ -767,4 +784,42 @@ mod tests { expected, ); } + + #[test] + fn test_collect_record_info_with_non_contract_meta() { + let mut files = Files::new(); + let file_id = files.add("test", ""); + let id = ItemId { file_id, index: 0 }; + let a = make_lin_item( + id, + TermKind::Declaration( + Ident::from("a"), + Vec::new(), + ValueState::Known(ItemId { file_id, index: 1 }), + ), + None, + ); + + let meta = MetaValue { + doc: Some("doc".to_string()), + types: None, + contracts: Vec::new(), + opt: false, + priority: MergePriority::Neutral, + value: None, + }; + let b = make_lin_item( + ItemId { file_id, index: 1 }, + TermKind::Record(HashMap::from([ + (Ident::from("one"), ItemId { file_id, index: 2 }), + (Ident::from("two"), ItemId { file_id, index: 2 }), + ])), + Some(meta), + ); + let lin = make_completed(vec![a, b]); + + let actual = collect_record_info(&lin, id, &mut Vec::new()); + let expected = vec![Ident::from("one"), Ident::from("two")]; + assert_eq!(actual, expected) + } } From ec3e6b85ff3366bee1d270880bb8a2c4c1aed4d2 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Mon, 19 Dec 2022 12:12:28 +0100 Subject: [PATCH 4/7] Fix tests --- lsp/nls/src/requests/completion.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 90d3fe0c1b..45619a29cb 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -808,6 +808,11 @@ mod tests { priority: MergePriority::Neutral, value: None, }; + let c = make_lin_item( + ItemId { file_id, index: 2 }, + TermKind::Structure, + None, + ); let b = make_lin_item( ItemId { file_id, index: 1 }, TermKind::Record(HashMap::from([ @@ -816,10 +821,11 @@ mod tests { ])), Some(meta), ); - let lin = make_completed(vec![a, b]); + let lin = make_completed(vec![a, b, c]); let actual = collect_record_info(&lin, id, &mut Vec::new()); let expected = vec![Ident::from("one"), Ident::from("two")]; + let actual = actual.iter().map(|iwm| iwm.ident).collect::>(); assert_eq!(actual, expected) } } From 972ff486e8e6f2040b7901d80e0efcae374532e1 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Mon, 19 Dec 2022 12:17:08 +0100 Subject: [PATCH 5/7] Format --- lsp/nls/src/requests/completion.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 45619a29cb..0d68b3bcb0 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -808,11 +808,7 @@ mod tests { priority: MergePriority::Neutral, value: None, }; - let c = make_lin_item( - ItemId { file_id, index: 2 }, - TermKind::Structure, - None, - ); + let c = make_lin_item(ItemId { file_id, index: 2 }, TermKind::Structure, None); let b = make_lin_item( ItemId { file_id, index: 1 }, TermKind::Record(HashMap::from([ From 0036a74c96a0c223f3190c68ec46e97c4dbe7e98 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Tue, 20 Dec 2022 16:29:05 +0100 Subject: [PATCH 6/7] Return a `Vec` --- lsp/nls/src/requests/completion.rs | 106 +++++++++++++++-------------- 1 file changed, 54 insertions(+), 52 deletions(-) diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 0d68b3bcb0..21def49694 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -115,31 +115,35 @@ fn find_fields_from_term_kind( linearization: &Completed, id: ItemId, path: &mut Vec, -) -> Option> { - let item = linearization.get_item(id)?; +) -> Vec { + let Some(item) = linearization.get_item(id) else { + return Vec::new() + }; match item.kind { TermKind::Record(ref fields) => { if path.is_empty() { - Some( - fields - .iter() - .map(|(&ident, &id)| { - // This unwrap is safe because, `id` is the field of the record - // we're currently analyzing. We're sure that the linearization - // phase doesn't produce wrong or invalid ids. - let item = linearization.get_item(id).unwrap(); - let (ty, _) = linearization.resolve_item_type_meta(item); - IdentWithType { - ident, - ty, - item: Some(item.clone()), - } - }) - .collect(), - ) + fields + .iter() + .map(|(&ident, &id)| { + // This unwrap is safe because, `id` is the field of the record + // we're currently analyzing. We're sure that the linearization + // phase doesn't produce wrong or invalid ids. + let item = linearization.get_item(id).unwrap(); + let (ty, _) = linearization.resolve_item_type_meta(item); + IdentWithType { + ident, + ty, + item: Some(item.clone()), + } + }) + .collect() } else { - let name = path.pop()?; - let new_id = fields.get(&name)?; + let Some(name) = path.pop() else { + return Vec::new() + }; + let Some(new_id) = fields.get(&name) else { + return Vec::new() + }; find_fields_from_term_kind(linearization, *new_id, path) } } @@ -151,7 +155,7 @@ fn find_fields_from_term_kind( | TermKind::Usage(UsageState::Resolved(new_id)) => { find_fields_from_term_kind(linearization, new_id, path) } - _ => None, + _ => Vec::new(), } } @@ -161,16 +165,18 @@ fn find_fields_from_contract( linearization: &Completed, id: ItemId, path: &mut Vec, -) -> Option> { - let item = linearization.get_item(id)?; +) -> Vec { + let Some(item) = linearization.get_item(id) else { + return Vec::new() + }; match &item.meta { - Some(meta_value) => Some(find_fields_from_meta_value(meta_value, path)), + Some(meta_value) => find_fields_from_meta_value(meta_value, path), None => match item.kind { TermKind::Declaration(_, _, ValueState::Known(new_id)) | TermKind::Usage(UsageState::Resolved(new_id)) => { find_fields_from_contract(linearization, new_id, path) } - _ => None, + _ => Vec::new(), }, } } @@ -187,7 +193,7 @@ fn find_fields_from_meta_value( .chain(meta_value.types.iter()) .flat_map(|contract| match &contract.types { Types(TypeF::Record(row)) => find_fields_from_type(row, path), - Types(TypeF::Flat(term)) => find_fields_from_term(term, path).unwrap_or_default(), + Types(TypeF::Flat(term)) => find_fields_from_term(term, path), _ => Vec::new(), }) .collect() @@ -205,9 +211,7 @@ fn find_fields_from_type(rrows: &RecordRows, path: &mut Vec) -> Vec { find_fields_from_type(&rrows_current, path) } - Some(Types(TypeF::Flat(term))) => { - find_fields_from_term(&term, path).unwrap_or_default() - } + Some(Types(TypeF::Flat(term))) => find_fields_from_term(&term, path), _ => Vec::new(), } } else { @@ -227,31 +231,32 @@ fn find_fields_from_type(rrows: &RecordRows, path: &mut Vec) -> Vec) -> Option> { +fn find_fields_from_term(term: &RichTerm, path: &mut Vec) -> Vec { let current = path.pop(); match (term.as_ref(), current) { - (Term::Record(data) | Term::RecRecord(data, ..), None) => Some( - data.fields - .keys() - .copied() - .map(|ident| IdentWithType { - ident, - ty: Types(TypeF::Flat(term.clone())), - item: None, - }) - .collect(), - ), + (Term::Record(data) | Term::RecRecord(data, ..), None) => data + .fields + .keys() + .copied() + .map(|ident| IdentWithType { + ident, + ty: Types(TypeF::Flat(term.clone())), + item: None, + }) + .collect(), (Term::Record(data) | Term::RecRecord(data, ..), Some(name)) => { - let term = data.fields.get(&name)?; + let Some(term) = data.fields.get(&name) else { + return Vec::new() + }; find_fields_from_term(term, path) } (Term::MetaValue(meta_value), Some(ident)) => { // We don't need to pop here, as the metavalue wraps the actual record path.push(ident); - Some(find_fields_from_meta_value(meta_value, path)) + find_fields_from_meta_value(meta_value, path) } - (Term::MetaValue(meta_value), None) => Some(find_fields_from_meta_value(meta_value, path)), - _ => None, + (Term::MetaValue(meta_value), None) => find_fields_from_meta_value(meta_value, path), + _ => Vec::new(), } } @@ -331,10 +336,8 @@ fn collect_record_info( // The path is mutable, so the first case would consume the path // so we have to clone it so that it can be correctly used for the second case. let mut p = path.clone(); - let mut fst = find_fields_from_contract(linearization, *body_id, path) - .unwrap_or_default(); - let snd = find_fields_from_term_kind(linearization, *body_id, &mut p) - .unwrap_or_default(); + let mut fst = find_fields_from_contract(linearization, *body_id, path); + let snd = find_fields_from_term_kind(linearization, *body_id, &mut p); fst.extend(snd); fst } @@ -344,7 +347,7 @@ fn collect_record_info( .. }, _, - ) => find_fields_from_term_kind(linearization, *value, path).unwrap_or_default(), + ) => find_fields_from_term_kind(linearization, *value, path), _ => Vec::new(), } }) @@ -647,7 +650,6 @@ mod tests { for id in ids { let mut actual: Vec<_> = find_fields_from_term_kind(&completed, id, &mut Vec::new()) - .unwrap() .iter() .map(|iwm| iwm.ident) .collect(); From 15a7f7060c18d1e15f53064b7d9bfd91b5af7db1 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Wed, 21 Dec 2022 16:43:29 +0100 Subject: [PATCH 7/7] Sort test results --- lsp/nls/src/requests/completion.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 21def49694..5b61bb1692 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -822,8 +822,10 @@ mod tests { let lin = make_completed(vec![a, b, c]); let actual = collect_record_info(&lin, id, &mut Vec::new()); - let expected = vec![Ident::from("one"), Ident::from("two")]; - let actual = actual.iter().map(|iwm| iwm.ident).collect::>(); + let mut expected = vec![Ident::from("one"), Ident::from("two")]; + let mut actual = actual.iter().map(|iwm| iwm.ident).collect::>(); + expected.sort(); + actual.sort(); assert_eq!(actual, expected) } }