Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completion in arrays #1746

Merged
merged 5 commits into from
Jan 9, 2024
Merged

Completion in arrays #1746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
04e0d04
Rename FieldHaver -> Container and add an Array variant
jneem Dec 19, 2023
8608fd0
Support arrays in the field resolver
jneem Dec 21, 2023
e8ef389
Add a test
jneem Dec 21, 2023
532e15d
Do better on the renaming
jneem Dec 21, 2023
68372d5
Don't look at pending_contracts
jneem Jan 9, 2024
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
67 changes: 60 additions & 7 deletions lsp/nls/src/analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ use std::collections::HashMap;

use codespan::FileId;
use nickel_lang_core::{
identifier::Ident,
term::{BinaryOp, RichTerm, Term, Traverse, TraverseControl},
typ::{Type, TypeF},
typecheck::{reporting::NameReg, TypeTables, TypecheckVisitor, UnifType},
};

use crate::{
field_walker::Def,
field_walker::{Def, EltId},
identifier::LocIdent,
position::PositionLookup,
term::RichTermPtr,
Expand All @@ -19,7 +18,7 @@ use crate::{
#[derive(Clone, Debug)]
pub struct Parent {
term: RichTerm,
child_name: Option<Ident>,
child_name: Option<EltId>,
}

impl From<RichTerm> for Parent {
Expand Down Expand Up @@ -54,7 +53,7 @@ impl ParentLookup {
if let Some(child) = &field.value {
let parent = Parent {
term: rt.clone(),
child_name: Some(name.ident()),
child_name: Some(name.ident().into()),
};
child.traverse_ref(
&mut |rt, parent| traversal(rt, parent, acc),
Expand All @@ -64,6 +63,19 @@ impl ParentLookup {
}
TraverseControl::SkipBranch
}
Term::Array(arr, _) => {
for elt in arr.iter() {
let parent = Parent {
term: rt.clone(),
child_name: Some(EltId::ArrayElt),
};
elt.traverse_ref(
&mut |rt, parent| traversal(rt, parent, acc),
&Some(parent),
);
}
TraverseControl::SkipBranch
}
_ => TraverseControl::ContinueWithScope(Some(rt.clone().into())),
}
}
Expand Down Expand Up @@ -110,7 +122,7 @@ impl ParentLookup {
/// `next` call.
pub struct ParentChainIter<'a> {
table: &'a ParentLookup,
path: Option<Vec<Ident>>,
path: Option<Vec<EltId>>,
next: Option<Parent>,
}

Expand All @@ -122,7 +134,11 @@ impl<'a> ParentChainIter<'a> {
}
if !matches!(
next.term.as_ref(),
Term::Record(_) | Term::RecRecord(..) | Term::Annotated(..) | Term::Op2(..)
Term::Record(_)
| Term::RecRecord(..)
| Term::Annotated(..)
| Term::Op2(..)
| Term::Array(..)
) {
self.path = None;
}
Expand Down Expand Up @@ -172,7 +188,7 @@ impl<'a> ParentChainIter<'a> {
None
}

pub fn path(&self) -> Option<&[Ident]> {
pub fn path(&self) -> Option<&[EltId]> {
self.path.as_deref()
}

Expand Down Expand Up @@ -349,3 +365,40 @@ impl TypeCollector {
CollectedTypes { terms, idents }
}
}

#[cfg(test)]
mod tests {
use assert_matches::assert_matches;
use nickel_lang_core::{identifier::Ident, term::Term};

use crate::{
field_walker::EltId,
position::tests::parse,
usage::{tests::locced, Environment, UsageLookup},
};

use super::ParentLookup;

#[test]
fn parent_chain() {
let (file, rt) = parse("{ foo = [{ bar = 1 }] }");
let bar_id = Ident::new("bar");
let bar = locced(bar_id, file, 11..14);

let parent = ParentLookup::new(&rt);
let usages = UsageLookup::new(&rt, &Environment::new());
let bar_rt = usages.def(&bar).unwrap().value().unwrap();

let p = parent.parent(bar_rt).unwrap();
assert_eq!(p.child_name, Some(EltId::Ident(bar_id)));
assert_matches!(p.term.as_ref(), Term::RecRecord(..));

let gp = parent.parent(&p.term).unwrap();
assert_eq!(gp.child_name, Some(EltId::ArrayElt));
assert_matches!(gp.term.as_ref(), Term::Array(..));

let ggp = parent.parent(&gp.term).unwrap();
assert_matches!(ggp.child_name, Some(EltId::Ident(_)));
assert_matches!(ggp.term.as_ref(), Term::RecRecord(..));
}
}
88 changes: 57 additions & 31 deletions lsp/nls/src/field_walker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,55 @@ use nickel_lang_core::{

use crate::{identifier::LocIdent, requests::completion::CompletionItem, server::Server};

/// A `FieldHaver` is something that... has fields.
/// A `Container` is something that has elements.
///
/// You can use a [`FieldResolver`] to resolve terms, or terms with paths, to `FieldHaver`s.
/// The elements could have names (e.g. in a record) or not (e.g. in an array).
///
/// You can use a [`FieldResolver`] to resolve terms, or terms with paths, to `Container`s.
#[derive(Clone, Debug, PartialEq)]
pub enum FieldHaver {
pub enum Container {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder: would that make sense to make this a trait instead? It seems that most of what you do below is to match on the type of container, and then apply specific behavior. It sounds a lot like what traits are for.

Also, it feels slightly odd to mix array and records, leading to a rather vague notion of container (I guess it's why this is hard to give it a meaningful name) and an EltId which also feels like it smashes two different things together to then match on the different possibilities and apply specific behavior (also, if I'm understanding correctly, some illegal cases such as trying to get an ident from an array aren't statically excluded but just return None).

At this point I might be oblivious to important details that make the current approach actually justified, sorry if this is the case. But from a distance, would that lead to much code duplication to have:

A trait FieldHaver with the methods you already implemented before this PR, and implement it for RecordData, Type, and RecordRows.
A trait Indexable (that's a bad name and it doesn't relate to FieldHaver, so it's doubly bad...) that is mostly doing the same but for array elements, which doesn't use Ident nor field in the method's name, and implement that for Array.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, I will think more about this over the break. I think there is at least one situation where you do want to dynamically handle either a record field or an array element, but it's probably a smaller use case and there's probably a better organization.

To elaborate, suppose we're trying to complete at the a in {foo = [{ a }]} | {foo | Array { aardvark | String }}. The way we currently do this without any evaluation is to first construct the path to the place we want completion. In some weird notation I just made up, this path looks like . foo / !! (where the !! means an array index -- we don't track what the index actually was). Then we follow the same path on the contract {foo | Array { aardvark | String }} to find the aardvark completion.

So the path does need to alternate at runtime between field accesses and array indices, and it might also happen that the path-follower tries to match an array index against a record (say, if the contract had been {foo | {_: {aardvark | String}}}). I don't know of any valid nickel code where this will happen, but of course nls also needs to handle invalid nickel code.

RecordTerm(RecordData),
Dict(Type),
RecordType(RecordRows),
Array(Type),
}

impl FieldHaver {
/// If this `FieldHaver` has a field named `id`, returns its value.
fn get(&self, id: Ident) -> Option<FieldContent> {
match self {
FieldHaver::RecordTerm(data) => data
/// A `ChildId` identifies an element of a container.
#[derive(Clone, Copy, Debug, PartialEq)]
pub enum EltId {
/// A named element, for example of an array.
Ident(Ident),
/// An array element.
ArrayElt,
}

impl From<Ident> for EltId {
fn from(id: Ident) -> Self {
EltId::Ident(id)
}
}

impl Container {
/// If this `Container` has a field named `id`, returns its value.
fn get(&self, id: EltId) -> Option<FieldContent> {
match (self, id) {
(Container::RecordTerm(data), EltId::Ident(id)) => data
.fields
.get(&id)
.map(|field| FieldContent::RecordField(field.clone())),
FieldHaver::Dict(ty) => Some(FieldContent::Type(ty.clone())),
FieldHaver::RecordType(rows) => rows
(Container::Dict(ty), EltId::Ident(_)) => Some(FieldContent::Type(ty.clone())),
(Container::RecordType(rows), EltId::Ident(id)) => rows
.find_path(&[id])
.map(|row| FieldContent::Type(row.typ.clone())),
(Container::Array(ty), EltId::ArrayElt) => Some(FieldContent::Type(ty.clone())),
_ => None,
}
}

/// If this `FieldHaver` is a record term, try to retrieve the field named `id`.
/// If this `Container` is a record term, try to retrieve the field named `id`.
fn get_field_and_loc(&self, id: Ident) -> Option<(LocIdent, &Field)> {
match self {
FieldHaver::RecordTerm(data) => data
Container::RecordTerm(data) => data
.fields
.get_key_value(&id)
.map(|(id, fld)| (LocIdent::from(*id), fld)),
Expand All @@ -51,19 +71,20 @@ impl FieldHaver {

pub fn get_definition_pos(&self, id: Ident) -> Option<LocIdent> {
match self {
FieldHaver::RecordTerm(data) => data
Container::RecordTerm(data) => data
.fields
.get_key_value(&id)
.map(|(id, _field)| (*id).into()),
FieldHaver::RecordType(rows) => rows.find_path(&[id]).map(|r| r.id.into()),
FieldHaver::Dict(_) => None,
Container::RecordType(rows) => rows.find_path(&[id]).map(|r| r.id.into()),
Container::Dict(_) => None,
Container::Array(_) => None,
}
}

/// Returns all fields in this `FieldHaver`, rendered as LSP completion items.
/// Returns all fields in this `Container`, rendered as LSP completion items.
pub fn completion_items(&self) -> impl Iterator<Item = CompletionItem> + '_ {
match self {
FieldHaver::RecordTerm(data) => {
Container::RecordTerm(data) => {
let iter = data.fields.iter().map(|(id, val)| CompletionItem {
label: ident_quoted(id),
detail: metadata_detail(&val.metadata),
Expand All @@ -73,8 +94,10 @@ impl FieldHaver {
});
Box::new(iter)
}
FieldHaver::Dict(_) => Box::new(std::iter::empty()) as Box<dyn Iterator<Item = _>>,
FieldHaver::RecordType(rows) => {
Container::Dict(_) | Container::Array(_) => {
Box::new(std::iter::empty()) as Box<dyn Iterator<Item = _>>
}
Container::RecordType(rows) => {
let iter = rows.iter().filter_map(|r| match r {
RecordRowsIteratorItem::TailDyn => None,
RecordRowsIteratorItem::TailVar(_) => None,
Expand All @@ -91,7 +114,7 @@ impl FieldHaver {
}
}

/// [`FieldHaver`]s can have fields that are either record fields or types.
/// [`Container`]s can have fields that are either record fields or types.
#[derive(Clone, Debug, PartialEq)]
enum FieldContent {
RecordField(Field),
Expand Down Expand Up @@ -219,8 +242,8 @@ impl<'a> FieldResolver<'a> {
pub fn resolve_term_path(
&self,
rt: &RichTerm,
path: impl Iterator<Item = Ident>,
) -> Vec<FieldHaver> {
path: impl Iterator<Item = EltId>,
) -> Vec<Container> {
let mut fields = self.resolve_term(rt);

for id in path {
Expand Down Expand Up @@ -286,11 +309,13 @@ impl<'a> FieldResolver<'a> {
ret
}

fn resolve_def_with_path(&self, def: &Def) -> Vec<FieldHaver> {
fn resolve_def_with_path(&self, def: &Def) -> Vec<Container> {
let mut fields = Vec::new();

if let Some(val) = def.value() {
fields.extend_from_slice(&self.resolve_term_path(val, def.path().iter().copied()))
fields.extend_from_slice(
&self.resolve_term_path(val, def.path().iter().copied().map(EltId::Ident)),
)
}
if let Some(meta) = def.metadata() {
fields.extend(self.resolve_annot(&meta.annotation));
Expand All @@ -306,7 +331,7 @@ impl<'a> FieldResolver<'a> {
fields
}

fn resolve_annot(&'a self, annot: &'a TypeAnnotation) -> impl Iterator<Item = FieldHaver> + 'a {
fn resolve_annot(&'a self, annot: &'a TypeAnnotation) -> impl Iterator<Item = Container> + 'a {
annot
.contracts
.iter()
Expand All @@ -319,10 +344,10 @@ impl<'a> FieldResolver<'a> {
/// This a best-effort thing; it doesn't do full evaluation but it has some reasonable
/// heuristics. For example, it knows that the fields defined on a merge of two records
/// are the fields defined on either record.
pub fn resolve_term(&self, rt: &RichTerm) -> Vec<FieldHaver> {
pub fn resolve_term(&self, rt: &RichTerm) -> Vec<Container> {
let term_fields = match rt.term.as_ref() {
Term::Record(data) | Term::RecRecord(data, ..) => {
vec![FieldHaver::RecordTerm(data.clone())]
vec![Container::RecordTerm(data.clone())]
}
Term::Var(id) => {
let id = LocIdent::from(*id);
Expand Down Expand Up @@ -358,7 +383,7 @@ impl<'a> FieldResolver<'a> {
}
Term::Let(_, _, body, _) | Term::LetPattern(_, _, _, body) => self.resolve_term(body),
Term::Op1(UnaryOp::StaticAccess(id), term) => {
self.resolve_term_path(term, std::iter::once(id.ident()))
self.resolve_term_path(term, std::iter::once(id.ident().into()))
}
Term::Annotated(annot, term) => {
let defs = self.resolve_annot(annot);
Expand All @@ -378,10 +403,11 @@ impl<'a> FieldResolver<'a> {
combine(term_fields, typ_fields)
}

fn resolve_type(&self, typ: &Type) -> Vec<FieldHaver> {
fn resolve_type(&self, typ: &Type) -> Vec<Container> {
match &typ.typ {
TypeF::Record(rows) => vec![FieldHaver::RecordType(rows.clone())],
TypeF::Dict { type_fields, .. } => vec![FieldHaver::Dict(type_fields.as_ref().clone())],
TypeF::Record(rows) => vec![Container::RecordType(rows.clone())],
TypeF::Dict { type_fields, .. } => vec![Container::Dict(type_fields.as_ref().clone())],
TypeF::Array(elt_ty) => vec![Container::Array(elt_ty.as_ref().clone())],
TypeF::Flat(rt) => self.resolve_term(rt),
_ => Default::default(),
}
Expand Down
13 changes: 7 additions & 6 deletions lsp/nls/src/requests/completion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::path::PathBuf;

use crate::{
cache::CacheExt,
field_walker::{FieldHaver, FieldResolver},
field_walker::{Container, EltId, FieldResolver},
identifier::LocIdent,
incomplete,
server::Server,
Expand Down Expand Up @@ -110,8 +110,9 @@ fn record_path_completion(term: RichTerm, server: &Server) -> Vec<CompletionItem

let (start_term, path) = extract_static_path(term);

let defs = FieldResolver::new(server).resolve_term_path(&start_term, path.iter().copied());
defs.iter().flat_map(FieldHaver::completion_items).collect()
let defs = FieldResolver::new(server)
.resolve_term_path(&start_term, path.iter().copied().map(EltId::Ident));
defs.iter().flat_map(Container::completion_items).collect()
}

fn env_completion(rt: &RichTerm, server: &Server) -> Vec<CompletionItem> {
Expand All @@ -124,14 +125,14 @@ fn env_completion(rt: &RichTerm, server: &Server) -> Vec<CompletionItem> {

// If the current term is a record, add its fields. (They won't be in the environment,
// because that's the environment *of* the current term. And we don't want to treat
// all possible FieldHavers here, because for example if the current term is a Term::Var
// all possible Containers here, because for example if the current term is a Term::Var
// that references a record, we don't want it.)
if matches!(rt.as_ref(), Term::RecRecord(..)) {
items.extend(
resolver
.resolve_term(rt)
.iter()
.flat_map(FieldHaver::completion_items),
.flat_map(Container::completion_items),
);
}

Expand All @@ -144,7 +145,7 @@ fn env_completion(rt: &RichTerm, server: &Server) -> Vec<CompletionItem> {
// TODO: This adds our merge "cousins" to the environment, but we should also
// be adding our merge "uncles" and "great-uncles".
let records = resolver.resolve_term_path(&p, path.iter().rev().copied());
items.extend(records.iter().flat_map(FieldHaver::completion_items));
items.extend(records.iter().flat_map(Container::completion_items));
}
}

Expand Down
4 changes: 2 additions & 2 deletions lsp/nls/src/requests/goto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use serde_json::Value;
use crate::{
cache::CacheExt,
diagnostic::LocationCompat,
field_walker::{Def, FieldResolver},
field_walker::{Def, EltId, FieldResolver},
identifier::LocIdent,
server::Server,
};
Expand Down Expand Up @@ -40,7 +40,7 @@ fn get_defs(term: &RichTerm, ident: Option<LocIdent>, server: &Server) -> Option
path.reverse();
let (last, path) = path.split_last()?;
let path: Vec<_> = path.iter().map(|id| id.ident()).collect();
let parents = resolver.resolve_term_path(value, path.iter().copied());
let parents = resolver.resolve_term_path(value, path.iter().copied().map(EltId::Ident));
parents
.iter()
.filter_map(|parent| parent.get_definition_pos(last.ident()))
Expand Down
Loading