quick and dirty fix of lsp error when position is None (#773)

* try to manage items without position in the lsp

* fix `linearisation::Completed::elem_at` and some code cleaning

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>

lsp/nls/src/linearization/building.rs

@@ -81,7 +81,7 @@ impl Building {
             let id = self.id_gen().get_and_advance();
             self.push(LinearizationItem {
                 id,
-                pos: ident.pos.unwrap(),
+                pos: ident.pos,
                 // temporary, the actual type is resolved later and the item retyped
                 ty: TypeWrapper::Concrete(AbsType::Dyn()),
                 kind: TermKind::RecordField {

lsp/nls/src/linearization/completed.rs

@@ -2,6 +2,7 @@ use std::collections::HashMap;
 
 use codespan::ByteIndex;
 use nickel_lang::{
+    position::TermPos,
     term::MetaValue,
     typecheck::linearization::{LinearizationState, Scope},
 };
@@ -80,21 +81,31 @@ impl Completed {
     ) -> Option<&LinearizationItem<Resolved>> {
         let (file_id, start) = locator;
         let linearization = &self.linearization;
-        let item = match linearization
+        let item = match linearization.binary_search_by(|item| {
-            .binary_search_by_key(locator, |item| (item.pos.src_id, item.pos.start))
+            item.pos
-        {
+                .as_opt_ref()
+                .map(|pos| (pos.src_id, pos.start).cmp(locator))
+                .unwrap_or(std::cmp::Ordering::Less)
+        }) {
             // Found item(s) starting at `locator`
             // search for most precise element
             Ok(index) => linearization[index..]
                 .iter()
-                .take_while(|item| (item.pos.src_id, item.pos.start) == *locator)
+                .take_while(|item| {
+                    // Here because None is smaller than everything, if binary search succeeds,
+                    // we can safely unwrap the position.
+                    let pos = item.pos.unwrap();
+                    (pos.src_id, pos.start) == *locator
+                })
                 .last(),
             // No perfect match found
             // iterate back finding the first wrapping linearization item
             Err(index) => linearization[..index].iter().rfind(|item| {
-                let (istart, iend, ifile) = (item.pos.start, item.pos.end, item.pos.src_id);
+                item.pos
-
+                    .as_opt_ref()
-                file_id == &ifile && start > &istart && start < &iend
+                    .map(|pos| file_id == &pos.src_id && start > &pos.start && start < &pos.end)
+                    // if the item found is None, we can not find a better one.
+                    .unwrap_or(true)
             }),
         };
         item

lsp/nls/src/linearization/mod.rs

@@ -35,7 +35,7 @@ pub type Environment = nickel_lang::environment::Environment<Ident, usize>;
 pub struct LinearizationItem<S: ResolutionState> {
     //term_: Box<Term>,
     pub id: usize,
-    pub pos: RawSpan,
+    pub pos: TermPos,
     pub ty: S,
     pub kind: TermKind,
     pub scope: Scope,
@@ -171,7 +171,7 @@ impl Linearizer for AnalysisHost {
                                 id: id_gen.get_and_advance(),
 
                                 ty: ty.clone(),
-                                pos: ident.pos.clone().unwrap(),
+                                pos: ident.pos.clone(),
                                 scope: self.scope.clone(),
                                 kind: TermKind::Structure,
                                 meta: self.meta.take(),
@@ -187,7 +187,7 @@ impl Linearizer for AnalysisHost {
                     lin.push(LinearizationItem {
                         id,
                         ty,
-                        pos: ident.pos.unwrap(),
+                        pos: ident.pos,
                         scope: self.scope.clone(),
                         kind: TermKind::Declaration(ident.to_owned(), Vec::new(), value_ptr),
                         meta: None,
@@ -201,7 +201,7 @@ impl Linearizer for AnalysisHost {
                         id,
                         // TODO: get type from pattern
                         ty: TypeWrapper::Concrete(AbsType::Dyn()),
-                        pos: ident.pos.unwrap(),
+                        pos: ident.pos,
                         scope: self.scope.clone(),
                         kind: TermKind::Declaration(
                             ident.to_owned(),
@@ -230,7 +230,7 @@ impl Linearizer for AnalysisHost {
                             id: id_gen.get_and_advance(),
 
                             ty: ty.clone(),
-                            pos: ident.pos.clone().unwrap(),
+                            pos: ident.pos.clone(),
                             scope: self.scope.clone(),
                             kind: TermKind::Structure,
                             meta: self.meta.take(),
@@ -244,7 +244,7 @@ impl Linearizer for AnalysisHost {
                 lin.push(LinearizationItem {
                     id: id_gen.get(),
                     ty,
-                    pos: ident.pos.unwrap(),
+                    pos: ident.pos,
                     scope: self.scope.clone(),
                     kind: TermKind::Declaration(ident.to_owned(), Vec::new(), value_ptr),
                     meta: self.meta.take(),
@@ -260,7 +260,7 @@ impl Linearizer for AnalysisHost {
 
                 lin.push(LinearizationItem {
                     id: root_id,
-                    pos: ident.pos.unwrap(),
+                    pos: ident.pos,
                     ty: TypeWrapper::Concrete(AbsType::Dyn()),
                     scope: self.scope.clone(),
                     kind: TermKind::Usage(UsageState::from(self.env.get(ident))),
@@ -278,7 +278,7 @@ impl Linearizer for AnalysisHost {
                         let id = id_gen.get_and_advance();
                         lin.push(LinearizationItem {
                             id,
-                            pos: accessor.pos.unwrap(),
+                            pos: accessor.pos,
                             ty: TypeWrapper::Concrete(AbsType::Dyn()),
                             scope: self.scope.clone(),
                             kind: TermKind::Usage(UsageState::Deferred {
@@ -293,7 +293,7 @@ impl Linearizer for AnalysisHost {
             Term::Record(fields, _) | Term::RecRecord(fields, ..) => {
                 lin.push(LinearizationItem {
                     id,
-                    pos: pos.unwrap(),
+                    pos: pos,
                     ty,
                     kind: TermKind::Record(HashMap::new()),
                     scope: self.scope.clone(),
@@ -395,7 +395,7 @@ impl Linearizer for AnalysisHost {
 
                 lin.push(LinearizationItem {
                     id,
-                    pos: pos.unwrap(),
+                    pos: pos,
                     ty,
                     scope: self.scope.clone(),
                     kind: TermKind::Structure,
@@ -437,7 +437,16 @@ impl Linearizer for AnalysisHost {
             scope,
         } = lin.into_inner();
 
-        linearization.sort_by_key(|item| (item.pos.src_id, item.pos.start));
+        linearization.sort_by(
+            |it1, it2| match (it1.pos.as_opt_ref(), it2.pos.as_opt_ref()) {
+                (None, None) => std::cmp::Ordering::Equal,
+                (None, _) => std::cmp::Ordering::Less,
+                (_, None) => std::cmp::Ordering::Greater,
+                (Some(pos1), Some(pos2)) => {
+                    (pos1.src_id, pos1.start).cmp(&(pos2.src_id, pos2.start))
+                }
+            },
+        );
 
         // create an index of id -> new position
         let mut id_mapping = HashMap::new();

lsp/nls/src/requests/goto.rs

@@ -5,7 +5,7 @@ use lsp_server::{RequestId, Response, ResponseError};
 use lsp_types::{
     GotoDefinitionParams, GotoDefinitionResponse, Location, Range, ReferenceParams, Url,
 };
-use nickel_lang::position::RawSpan;
+use nickel_lang::position::{RawSpan, TermPos};
 use serde_json::Value;
 
 use crate::{
@@ -57,7 +57,7 @@ pub fn handle_to_definition(
     let location = match item.kind {
         TermKind::Usage(UsageState::Resolved(usage_id)) => {
             let definition = linearization.get_item(usage_id).unwrap();
-            let location = match definition.pos {
+            let location = match definition.pos.unwrap() {
                 RawSpan {
                     start: ByteIndex(start),
                     end: ByteIndex(end),
@@ -117,30 +117,27 @@ pub fn handle_to_usages(
 
     debug!("found referencing item: {:?}", item);
 
-    let locations = match &item.kind {
+    let locations: Option<Vec<Location>> = match &item.kind {
-        TermKind::Declaration(_, usages, _) | TermKind::RecordField { usages, .. } => {
+        TermKind::Declaration(_, usages, _) | TermKind::RecordField { usages, .. } => Some(
-            let mut locations = Vec::new();
+            usages
-
+                .iter()
-            for reference_id in usages.iter() {
+                .filter_map(|reference_id| {
-                let reference = linearization.get_item(*reference_id).unwrap();
+                    linearization
-                let location = match reference.pos {
+                        .get_item(*reference_id)
-                    RawSpan {
+                        .unwrap()
-                        start: ByteIndex(start),
+                        .pos
-                        end: ByteIndex(end),
+                        .as_opt_ref()
-                        src_id,
+                        .map(|RawSpan { start, end, src_id }| Location {
-                    } => Location {
+                            uri: Url::parse(&server.cache.name(*src_id).to_string_lossy()).unwrap(),
-                        uri: Url::parse(&server.cache.name(src_id).to_string_lossy()).unwrap(),
+                            range: Range::from_codespan(
-                        range: Range::from_codespan(
+                                src_id,
-                            &src_id,
+                                &((*start).into()..(*end).into()),
-                            &(start as usize..end as usize),
+                                server.cache.files(),
-                            server.cache.files(),
+                            ),
-                        ),
+                        })
-                    },
+                })
-                };
+                .collect(),
-                locations.push(location);
+        ),
-            }
-            Some(locations)
-        }
         _ => None,
     };
 

lsp/nls/src/requests/hover.rs

@@ -3,6 +3,7 @@ use codespan_lsp::position_to_byte_index;
 use log::debug;
 use lsp_server::{RequestId, Response, ResponseError};
 use lsp_types::{Hover, HoverContents, HoverParams, LanguageString, MarkedString, Range};
+use nickel_lang::position::TermPos;
 use serde_json::Value;
 
 use crate::{
@@ -54,9 +55,14 @@ pub fn handle(
 
     let (ty, meta) = linearization.resolve_item_type_meta(&item);
 
+    if item.pos == TermPos::None {
+        server.reply(Response::new_ok(id, Value::Null));
+        return Ok(());
+    }
+    let pos = item.pos.unwrap();
     let range = Range::from_codespan(
         &file_id,
-        &(item.pos.start.to_usize()..item.pos.end.to_usize()),
+        &(pos.start.to_usize()..pos.end.to_usize()),
         server.cache.files(),
     );
 

lsp/nls/src/requests/symbols.rs

@@ -26,7 +26,8 @@ pub fn handle_document_symbols(
             .iter()
             .filter_map(|item| match &item.kind {
                 TermKind::Declaration(name, _, _) => {
-                    let (file_id, span) = item.pos.to_range();
+                    // TODO: can `unwrap` fail here?
+                    let (file_id, span) = item.pos.unwrap().to_range();
 
                     let range =
                         codespan_lsp::byte_span_to_range(server.cache.files(), file_id, span)