From bb23cd78fcdd497fef158f54a22b4d92801cf48c Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Wed, 11 Sep 2024 00:04:24 +0200 Subject: [PATCH 1/9] Explicit imports --- core/src/cache.rs | 5 +++- core/src/error/mod.rs | 13 +++++++++ core/src/eval/mod.rs | 4 +-- core/src/eval/tests.rs | 12 ++++++--- core/src/parser/error.rs | 3 +++ core/src/parser/grammar.lalrpop | 7 ++++- core/src/parser/tests.rs | 4 +-- core/src/parser/utils.rs | 35 +++++++++++++++++++++++++ core/src/pretty.rs | 4 +-- core/src/program.rs | 10 +++++-- core/src/term/mod.rs | 25 +++++++++++------- core/src/transform/free_vars.rs | 2 +- core/src/transform/import_resolution.rs | 2 +- core/src/typecheck/mod.rs | 4 +-- 14 files changed, 103 insertions(+), 27 deletions(-) diff --git a/core/src/cache.rs b/core/src/cache.rs index e5f962f3ea..784143c9a0 100644 --- a/core/src/cache.rs +++ b/core/src/cache.rs @@ -1319,6 +1319,7 @@ pub trait ImportResolver { fn resolve( &mut self, path: &OsStr, + format: InputFormat, parent: Option, pos: &TermPos, ) -> Result<(ResolvedTerm, FileId), ImportError>; @@ -1333,6 +1334,7 @@ impl ImportResolver for Cache { fn resolve( &mut self, path: &OsStr, + format: InputFormat, parent: Option, pos: &TermPos, ) -> Result<(ResolvedTerm, FileId), ImportError> { @@ -1367,7 +1369,6 @@ impl ImportResolver for Cache { ) })?; - let format = InputFormat::from_path(&path_buf).unwrap_or_default(); let (result, file_id) = match id_op { CacheOp::Cached(id) => (ResolvedTerm::FromCache, id), CacheOp::Done(id) => (ResolvedTerm::FromFile { path: path_buf }, id), @@ -1467,6 +1468,7 @@ pub mod resolvers { fn resolve( &mut self, _path: &OsStr, + _format: InputFormat, _parent: Option, _pos: &TermPos, ) -> Result<(ResolvedTerm, FileId), ImportError> { @@ -1508,6 +1510,7 @@ pub mod resolvers { fn resolve( &mut self, path: &OsStr, + _format: InputFormat, _parent: Option, pos: &TermPos, ) -> Result<(ResolvedTerm, FileId), ImportError> { diff --git a/core/src/error/mod.rs b/core/src/error/mod.rs index 27a26740dc..b779c206a5 100644 --- a/core/src/error/mod.rs +++ b/core/src/error/mod.rs @@ -583,6 +583,9 @@ pub enum ParseError { /// time, there are a set of expressions that can be excluded syntactically. Currently, it's /// mostly constants. InvalidContract(RawSpan), + /// Attempt to specify an import, type of which is not known at the moment of compilation. + /// `explicit` determines whether explicit import type annotation was used + InvalidImport { span: RawSpan, explicit: bool }, } /// An error occurring during the resolution of an import. @@ -815,6 +818,9 @@ impl ParseError { } } InternalParseError::InvalidContract(span) => ParseError::InvalidContract(span), + InternalParseError::InvalidImport { span, explicit } => { + ParseError::InvalidImport { span, explicit } + } }, } } @@ -2113,6 +2119,13 @@ impl IntoDiagnostics for ParseError { .to_owned(), "Only functions and records might be valid contracts".to_owned(), ]), + ParseError::InvalidImport{span, explicit: false} => Diagnostic::error() + .with_message("cannot determine type of this import using filename extension") + .with_labels(vec![primary(&span)]) + .with_notes(vec!["You can specify type of the import by using a tag like 'Nickel or 'Raw as a first argument to import statement".to_owned()]), + ParseError::InvalidImport{span, explicit: true} => Diagnostic::error() + .with_message("unknown import type tag") + .with_labels(vec![primary(&span)]), }; vec![diagnostic] diff --git a/core/src/eval/mod.rs b/core/src/eval/mod.rs index 0f315b3464..e5a52e9121 100644 --- a/core/src/eval/mod.rs +++ b/core/src/eval/mod.rs @@ -796,7 +796,7 @@ impl VirtualMachine { )); } } - Term::Import(path) => { + Term::Import { path, .. } => { return Err(EvalError::InternalError( format!("Unresolved import ({})", path.to_string_lossy()), pos, @@ -1169,7 +1169,7 @@ pub fn subst( | v @ Term::ForeignId(_) | v @ Term::SealingKey(_) | v @ Term::Enum(_) - | v @ Term::Import(_) + | v @ Term::Import{..} | v @ Term::ResolvedImport(_) // We could recurse here, because types can contain terms which would then be subject to // substitution. Not recursing should be fine, though, because a type in term position diff --git a/core/src/eval/tests.rs b/core/src/eval/tests.rs index 7ff97f59fa..ea3c9f0de5 100644 --- a/core/src/eval/tests.rs +++ b/core/src/eval/tests.rs @@ -131,15 +131,15 @@ fn imports() { .add_source(String::from("bad"), String::from("^$*/.23ab 0°@")); vm.import_resolver_mut().add_source( String::from("nested"), - String::from("let x = import \"two\" in x + 1"), + String::from("let x = import 'Nickel \"two\" in x + 1"), ); vm.import_resolver_mut().add_source( String::from("cycle"), - String::from("let x = import \"cycle_b\" in {a = 1, b = x.a}"), + String::from("let x = import 'Nickel \"cycle_b\" in {a = 1, b = x.a}"), ); vm.import_resolver_mut().add_source( String::from("cycle_b"), - String::from("let x = import \"cycle\" in {a = x.a}"), + String::from("let x = import 'Nickel \"cycle\" in {a = x.a}"), ); fn mk_import( @@ -152,7 +152,11 @@ fn imports() { R: ImportResolver, { resolve_imports( - mk_term::let_one_in(var, mk_term::import(import), body), + mk_term::let_one_in( + var, + mk_term::import(import, crate::cache::InputFormat::Nickel), + body, + ), vm.import_resolver_mut(), ) .map(|resolve_result| resolve_result.transformed_term) diff --git a/core/src/parser/error.rs b/core/src/parser/error.rs index 99206e9f9d..d1517c326e 100644 --- a/core/src/parser/error.rs +++ b/core/src/parser/error.rs @@ -153,4 +153,7 @@ pub enum ParseError { /// time, there are a set of expressions that can be excluded syntactically. Currently, it's /// mostly constants. InvalidContract(RawSpan), + /// Attempt to specify an import, type of which is not known at the moment of compilation. + /// `explicit` determines whether explicit import type annotation was used + InvalidImport { span: RawSpan, explicit: bool }, } diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop index 991673ec80..1f504d9006 100644 --- a/core/src/parser/grammar.lalrpop +++ b/core/src/parser/grammar.lalrpop @@ -252,7 +252,12 @@ UniTerm: UniTerm = { => { UniTerm::from(err) }, - "import" => UniTerm::from(Term::Import(OsString::from(s))), + "import" =>? { + Ok(UniTerm::from(mk_import_based_on_filename(s, mk_span(src_id, l, r))?)) + }, + "import" =>? { + Ok(UniTerm::from(mk_import_explicit(s, t, mk_span(src_id, l, r))?)) + }, }; AnnotatedInfixExpr: UniTerm = { diff --git a/core/src/parser/tests.rs b/core/src/parser/tests.rs index 5d0783aec5..ef1e827013 100644 --- a/core/src/parser/tests.rs +++ b/core/src/parser/tests.rs @@ -555,7 +555,7 @@ fn ty_var_kind_mismatch() { fn import() { assert_eq!( parse_without_pos("import \"file.ncl\""), - mk_term::import("file.ncl") + mk_term::import("file.ncl", crate::cache::InputFormat::Nickel) ); assert_matches!( parse("import \"file.ncl\" some args"), @@ -564,7 +564,7 @@ fn import() { assert_eq!( parse_without_pos("(import \"file.ncl\") some args"), mk_app!( - mk_term::import("file.ncl"), + mk_term::import("file.ncl", crate::cache::InputFormat::Nickel), mk_term::var("some"), mk_term::var("args") ) diff --git a/core/src/parser/utils.rs b/core/src/parser/utils.rs index 4ac5b0ca17..18cb475b33 100644 --- a/core/src/parser/utils.rs +++ b/core/src/parser/utils.rs @@ -1,6 +1,7 @@ //! Various helpers and companion code for the parser are put here to keep the grammar definition //! uncluttered. use indexmap::map::Entry; +use std::ffi::OsString; use std::rc::Rc; use std::{collections::HashSet, fmt::Debug}; @@ -10,6 +11,7 @@ use self::pattern::bindings::Bindings as _; use super::error::ParseError; +use crate::cache::InputFormat; use crate::{ combine::Combine, eval::{ @@ -699,6 +701,39 @@ pub fn mk_fun(pat: Pattern, body: RichTerm) -> Term { } } +pub fn mk_import_based_on_filename(path: String, span: RawSpan) -> Result { + let path = OsString::from(path); + let Some(typ): Option = + InputFormat::from_path(std::path::Path::new(path.as_os_str())) + else { + return Err(ParseError::InvalidImport { + span, + explicit: false, + }); + }; + Ok(Term::Import { path, typ }) +} + +pub fn mk_import_explicit(path: String, typ: LocIdent, span: RawSpan) -> Result { + let path = OsString::from(path); + let typ = match typ.label() { + "Json" => crate::cache::InputFormat::Json, + "Nickel" => crate::cache::InputFormat::Nickel, + "Raw" => crate::cache::InputFormat::Raw, + "Yaml" => crate::cache::InputFormat::Yaml, + "Toml" => crate::cache::InputFormat::Toml, + #[cfg(feature = "nix-experimental")] + "Nix" => crate::cache::InputFormat::Nix, + _ => { + return Err(ParseError::InvalidImport { + span, + explicit: true, + }) + } + }; + Ok(Term::Import { path, typ }) +} + /// Determine the minimal level of indentation of a multi-line string. /// /// The result is determined by computing the minimum indentation level among all lines, where the diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 97f2101bd6..73d33bbdec 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -143,7 +143,7 @@ fn needs_parens_in_type_pos(typ: &Type) -> bool { | Term::Let(..) | Term::LetPattern(..) | Term::Op1(UnaryOp::IfThenElse, _) - | Term::Import(..) + | Term::Import { .. } | Term::ResolvedImport(..) ) } else { @@ -1056,7 +1056,7 @@ where SealingKey(sym) => allocator.text(format!("%")), Sealed(_i, _rt, _lbl) => allocator.text("%"), Annotated(annot, rt) => allocator.atom(rt).append(annot.pretty(allocator)), - Import(f) => allocator + Import { path: f, .. } => allocator // TODO: preserve type .text("import ") .append(allocator.as_string(f.to_string_lossy()).double_quotes()), ResolvedImport(id) => allocator.text(format!("import ")), diff --git a/core/src/program.rs b/core/src/program.rs index 34005b7df9..fc1e810d7c 100644 --- a/core/src/program.rs +++ b/core/src/program.rs @@ -252,13 +252,19 @@ impl Program { let merge_term = inputs .into_iter() .map(|input| match input { - Input::Path(path) => RichTerm::from(Term::Import(path.into())), + Input::Path(path) => RichTerm::from(Term::Import { + path: path.into(), + typ: InputFormat::Nickel, + }), Input::Source(source, name) => { let path = PathBuf::from(name.into()); cache .add_source(SourcePath::Path(path.clone()), source) .unwrap(); - RichTerm::from(Term::Import(path.into())) + RichTerm::from(Term::Import { + path: path.into(), + typ: InputFormat::Nickel, + }) } }) .reduce(|acc, f| mk_term::op2(BinaryOp::Merge(Label::default().into()), acc, f)) diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 12b72dd7b6..13797bb403 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -21,6 +21,7 @@ use smallvec::SmallVec; use string::NickelString; use crate::{ + cache::InputFormat, error::{EvalError, ParseError}, eval::{cache::CacheIndex, Environment}, identifier::LocIdent, @@ -207,7 +208,7 @@ pub enum Term { /// An unresolved import. #[serde(skip)] - Import(OsString), + Import { path: OsString, typ: InputFormat }, /// A resolved import (which has already been loaded and parsed). #[serde(skip)] @@ -365,7 +366,9 @@ impl PartialEq for Term { l0 == r0 && l1 == r1 && l2 == r2 } (Self::Annotated(l0, l1), Self::Annotated(r0, r1)) => l0 == r0 && l1 == r1, - (Self::Import(l0), Self::Import(r0)) => l0 == r0, + (Self::Import { path: l0, typ: l1 }, Self::Import { path: r0, typ: r1 }) => { + l0 == r0 && l1 == r1 + } (Self::ResolvedImport(l0), Self::ResolvedImport(r0)) => l0 == r0, ( Self::Type { @@ -973,7 +976,7 @@ impl Term { | Term::Op1(_, _) | Term::Op2(_, _, _) | Term::OpN(..) - | Term::Import(_) + | Term::Import { .. } | Term::ResolvedImport(_) | Term::StrChunks(_) | Term::ParseError(_) @@ -1026,7 +1029,7 @@ impl Term { | Term::OpN(..) | Term::Sealed(..) | Term::Annotated(..) - | Term::Import(_) + | Term::Import{..} | Term::ResolvedImport(_) | Term::StrChunks(_) | Term::RecRecord(..) @@ -1088,7 +1091,7 @@ impl Term { | Term::OpN(..) | Term::Sealed(..) | Term::Annotated(..) - | Term::Import(_) + | Term::Import { .. } | Term::ResolvedImport(_) | Term::StrChunks(_) | Term::RecRecord(..) @@ -1144,7 +1147,7 @@ impl Term { | Term::OpN(..) | Term::Sealed(..) | Term::Annotated(..) - | Term::Import(..) + | Term::Import{..} | Term::ResolvedImport(..) | Term::Closure(_) | Term::ParseError(_) @@ -2383,7 +2386,7 @@ impl Traverse for RichTerm { | Term::Var(_) | Term::Closure(_) | Term::Enum(_) - | Term::Import(_) + | Term::Import { .. } | Term::ResolvedImport(_) | Term::SealingKey(_) | Term::ForeignId(_) @@ -2852,11 +2855,15 @@ pub mod make { mk_fun!("x", var("x")) } - pub fn import(path: S) -> RichTerm + pub fn import(path: S, typ: InputFormat) -> RichTerm where S: Into, { - Term::Import(path.into()).into() + Term::Import { + path: path.into(), + typ, + } + .into() } pub fn integer(n: impl Into) -> RichTerm { diff --git a/core/src/transform/free_vars.rs b/core/src/transform/free_vars.rs index a01643eb1c..b91395a957 100644 --- a/core/src/transform/free_vars.rs +++ b/core/src/transform/free_vars.rs @@ -42,7 +42,7 @@ impl CollectFreeVars for RichTerm { | Term::ForeignId(_) | Term::SealingKey(_) | Term::Enum(_) - | Term::Import(_) + | Term::Import { .. } | Term::ResolvedImport(_) => (), Term::Fun(id, t) => { let mut fresh = HashSet::new(); diff --git a/core/src/transform/import_resolution.rs b/core/src/transform/import_resolution.rs index f549750452..8d1115d24c 100644 --- a/core/src/transform/import_resolution.rs +++ b/core/src/transform/import_resolution.rs @@ -144,7 +144,7 @@ pub mod tolerant { { let term = rt.as_ref(); match term { - Term::Import(path) => match resolver.resolve(path, parent, &rt.pos) { + Term::Import { path, typ } => match resolver.resolve(path, *typ, parent, &rt.pos) { Ok((_, file_id)) => (RichTerm::new(Term::ResolvedImport(file_id), rt.pos), None), Err(err) => (rt, Some(err)), }, diff --git a/core/src/typecheck/mod.rs b/core/src/typecheck/mod.rs index a2c335dedc..f8f1d06f46 100644 --- a/core/src/typecheck/mod.rs +++ b/core/src/typecheck/mod.rs @@ -1449,7 +1449,7 @@ fn walk( | Term::SealingKey(_) // This function doesn't recursively typecheck imports: this is the responsibility of the // caller. - | Term::Import(_) + | Term::Import{..} | Term::ResolvedImport(_) => Ok(()), Term::Var(x) => ctxt.type_env .get(&x.ident()) @@ -2407,7 +2407,7 @@ fn check( .unify(mk_uniftype::sym(), state, &ctxt) .map_err(|err| err.into_typecheck_err(state, rt.pos)), Term::Sealed(_, t, _) => check(state, ctxt, visitor, t, ty), - Term::Import(_) => ty + Term::Import { .. } => ty .unify(mk_uniftype::dynamic(), state, &ctxt) .map_err(|err| err.into_typecheck_err(state, rt.pos)), // We use the apparent type of the import for checking. This function doesn't recursively From 97a9616e23da83ea43711831f8bb58c9752d8ae0 Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Thu, 12 Sep 2024 21:15:26 +0200 Subject: [PATCH 2/9] explicit import: more changes. * Rename typ to format * Bring back the fallback * Implement pretty-printer part * Fix compilation of NLP --- core/src/cache.rs | 25 +++++++++++++++ core/src/error/mod.rs | 21 ++++++------- core/src/parser/error.rs | 5 ++- core/src/parser/utils.rs | 42 ++++++++++--------------- core/src/pretty.rs | 11 +++++-- core/src/program.rs | 4 +-- core/src/term/mod.rs | 19 +++++++---- core/src/transform/import_resolution.rs | 3 +- lsp/nls/src/requests/completion.rs | 2 +- 9 files changed, 79 insertions(+), 53 deletions(-) diff --git a/core/src/cache.rs b/core/src/cache.rs index 784143c9a0..bda6711403 100644 --- a/core/src/cache.rs +++ b/core/src/cache.rs @@ -58,6 +58,31 @@ impl InputFormat { _ => None, } } + pub fn from_tag(tag: &str) -> Option { + Some(match tag { + "Json" => InputFormat::Json, + "Nickel" => InputFormat::Nickel, + "Raw" => InputFormat::Raw, + "Yaml" => InputFormat::Yaml, + "Toml" => InputFormat::Toml, + #[cfg(feature = "nix-experimental")] + "Nix" => InputFormat::Nix, + _ => return None, + }) + } + + pub fn to_tag(&self) -> &'static str { + match self { + InputFormat::Nickel => "Nickel", + InputFormat::Json => "Json", + InputFormat::Yaml => "Yaml", + InputFormat::Toml => "Toml", + InputFormat::Raw => "Raw", + #[cfg(feature = "nix-experimental")] + InputFormat::Nix => "Nix", + } + } + /// Renturns an [InputFormat] based on the extension of a source path. pub fn from_source_path(source_path: &SourcePath) -> Option { if let SourcePath::Path(p) = source_path { diff --git a/core/src/error/mod.rs b/core/src/error/mod.rs index b779c206a5..faef695eb7 100644 --- a/core/src/error/mod.rs +++ b/core/src/error/mod.rs @@ -583,9 +583,8 @@ pub enum ParseError { /// time, there are a set of expressions that can be excluded syntactically. Currently, it's /// mostly constants. InvalidContract(RawSpan), - /// Attempt to specify an import, type of which is not known at the moment of compilation. - /// `explicit` determines whether explicit import type annotation was used - InvalidImport { span: RawSpan, explicit: bool }, + /// Unrecognized explicit import format tag + InvalidImportFormat { span: RawSpan }, } /// An error occurring during the resolution of an import. @@ -818,8 +817,8 @@ impl ParseError { } } InternalParseError::InvalidContract(span) => ParseError::InvalidContract(span), - InternalParseError::InvalidImport { span, explicit } => { - ParseError::InvalidImport { span, explicit } + InternalParseError::InvalidImportFormat { span } => { + ParseError::InvalidImportFormat { span } } }, } @@ -2119,13 +2118,13 @@ impl IntoDiagnostics for ParseError { .to_owned(), "Only functions and records might be valid contracts".to_owned(), ]), - ParseError::InvalidImport{span, explicit: false} => Diagnostic::error() - .with_message("cannot determine type of this import using filename extension") + ParseError::InvalidImportFormat{span} => Diagnostic::error() + .with_message("unknown import format tag") .with_labels(vec![primary(&span)]) - .with_notes(vec!["You can specify type of the import by using a tag like 'Nickel or 'Raw as a first argument to import statement".to_owned()]), - ParseError::InvalidImport{span, explicit: true} => Diagnostic::error() - .with_message("unknown import type tag") - .with_labels(vec![primary(&span)]), + .with_notes(vec![ + "Examples of valid format tags: 'Nickel 'Json 'Yaml 'Toml 'Raw" + .to_owned() + ]), }; vec![diagnostic] diff --git a/core/src/parser/error.rs b/core/src/parser/error.rs index d1517c326e..817460d83a 100644 --- a/core/src/parser/error.rs +++ b/core/src/parser/error.rs @@ -153,7 +153,6 @@ pub enum ParseError { /// time, there are a set of expressions that can be excluded syntactically. Currently, it's /// mostly constants. InvalidContract(RawSpan), - /// Attempt to specify an import, type of which is not known at the moment of compilation. - /// `explicit` determines whether explicit import type annotation was used - InvalidImport { span: RawSpan, explicit: bool }, + /// Unrecognized explicit import format tag + InvalidImportFormat { span: RawSpan }, } diff --git a/core/src/parser/utils.rs b/core/src/parser/utils.rs index 18cb475b33..1db0114ff7 100644 --- a/core/src/parser/utils.rs +++ b/core/src/parser/utils.rs @@ -701,37 +701,27 @@ pub fn mk_fun(pat: Pattern, body: RichTerm) -> Term { } } -pub fn mk_import_based_on_filename(path: String, span: RawSpan) -> Result { +pub fn mk_import_based_on_filename(path: String, _span: RawSpan) -> Result { let path = OsString::from(path); - let Some(typ): Option = - InputFormat::from_path(std::path::Path::new(path.as_os_str())) - else { - return Err(ParseError::InvalidImport { - span, - explicit: false, - }); - }; - Ok(Term::Import { path, typ }) + let format: Option = + InputFormat::from_path(std::path::Path::new(path.as_os_str())); + + // Fall back to InputFormat::Nickel in case of unknown filename extension for backwards compatiblilty. + let format = format.unwrap_or_default(); + + Ok(Term::Import { path, format }) } -pub fn mk_import_explicit(path: String, typ: LocIdent, span: RawSpan) -> Result { +pub fn mk_import_explicit( + path: String, + format: LocIdent, + span: RawSpan, +) -> Result { let path = OsString::from(path); - let typ = match typ.label() { - "Json" => crate::cache::InputFormat::Json, - "Nickel" => crate::cache::InputFormat::Nickel, - "Raw" => crate::cache::InputFormat::Raw, - "Yaml" => crate::cache::InputFormat::Yaml, - "Toml" => crate::cache::InputFormat::Toml, - #[cfg(feature = "nix-experimental")] - "Nix" => crate::cache::InputFormat::Nix, - _ => { - return Err(ParseError::InvalidImport { - span, - explicit: true, - }) - } + let Some(format) = InputFormat::from_tag(format.label()) else { + return Err(ParseError::InvalidImportFormat { span }); }; - Ok(Term::Import { path, typ }) + Ok(Term::Import { path, format }) } /// Determine the minimal level of indentation of a multi-line string. diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 73d33bbdec..995d5733ec 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1,5 +1,6 @@ use std::fmt; +use crate::cache::InputFormat; use crate::identifier::LocIdent; use crate::parser::lexer::KEYWORDS; use crate::term::{ @@ -1056,9 +1057,13 @@ where SealingKey(sym) => allocator.text(format!("%")), Sealed(_i, _rt, _lbl) => allocator.text("%"), Annotated(annot, rt) => allocator.atom(rt).append(annot.pretty(allocator)), - Import { path: f, .. } => allocator // TODO: preserve type - .text("import ") - .append(allocator.as_string(f.to_string_lossy()).double_quotes()), + Import { path, format } => { + let mut a = allocator.text("import "); + if Some(*format) != InputFormat::from_path(std::path::Path::new(path.as_os_str())) { + a = a.append("'").append(format.to_tag()).append(" "); + } + a.append(allocator.as_string(path.to_string_lossy()).double_quotes()) + } ResolvedImport(id) => allocator.text(format!("import ")), // This type is in term position, so we don't need to add parentheses. Type { typ, contract: _ } => typ.pretty(allocator), diff --git a/core/src/program.rs b/core/src/program.rs index fc1e810d7c..c7d5e880af 100644 --- a/core/src/program.rs +++ b/core/src/program.rs @@ -254,7 +254,7 @@ impl Program { .map(|input| match input { Input::Path(path) => RichTerm::from(Term::Import { path: path.into(), - typ: InputFormat::Nickel, + format: InputFormat::Nickel, }), Input::Source(source, name) => { let path = PathBuf::from(name.into()); @@ -263,7 +263,7 @@ impl Program { .unwrap(); RichTerm::from(Term::Import { path: path.into(), - typ: InputFormat::Nickel, + format: InputFormat::Nickel, }) } }) diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 13797bb403..28206a94c2 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -208,7 +208,7 @@ pub enum Term { /// An unresolved import. #[serde(skip)] - Import { path: OsString, typ: InputFormat }, + Import { path: OsString, format: InputFormat }, /// A resolved import (which has already been loaded and parsed). #[serde(skip)] @@ -366,9 +366,16 @@ impl PartialEq for Term { l0 == r0 && l1 == r1 && l2 == r2 } (Self::Annotated(l0, l1), Self::Annotated(r0, r1)) => l0 == r0 && l1 == r1, - (Self::Import { path: l0, typ: l1 }, Self::Import { path: r0, typ: r1 }) => { - l0 == r0 && l1 == r1 - } + ( + Self::Import { + path: l0, + format: l1, + }, + Self::Import { + path: r0, + format: r1, + }, + ) => l0 == r0 && l1 == r1, (Self::ResolvedImport(l0), Self::ResolvedImport(r0)) => l0 == r0, ( Self::Type { @@ -2855,13 +2862,13 @@ pub mod make { mk_fun!("x", var("x")) } - pub fn import(path: S, typ: InputFormat) -> RichTerm + pub fn import(path: S, format: InputFormat) -> RichTerm where S: Into, { Term::Import { path: path.into(), - typ, + format, } .into() } diff --git a/core/src/transform/import_resolution.rs b/core/src/transform/import_resolution.rs index 8d1115d24c..6bf9d24c91 100644 --- a/core/src/transform/import_resolution.rs +++ b/core/src/transform/import_resolution.rs @@ -144,7 +144,8 @@ pub mod tolerant { { let term = rt.as_ref(); match term { - Term::Import { path, typ } => match resolver.resolve(path, *typ, parent, &rt.pos) { + Term::Import { path, format } => match resolver.resolve(path, *format, parent, &rt.pos) + { Ok((_, file_id)) => (RichTerm::new(Term::ResolvedImport(file_id), rt.pos), None), Err(err) => (rt, Some(err)), }, diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 4c26ed5722..e1a748a586 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -229,7 +229,7 @@ pub fn handle_completion( let term = server.world.lookup_term_by_position(pos)?.cloned(); let ident = server.world.lookup_ident_by_position(pos)?; - if let Some(Term::Import(import)) = term.as_ref().map(|t| t.term.as_ref()) { + if let Some(Term::Import { path: import, .. }) = term.as_ref().map(|t| t.term.as_ref()) { // Don't respond with anything if trigger is a `.`, as that may be the // start of a relative file path `./`, or the start of a file extension if !matches!(trigger, Some(".")) { From b3752e0cbbe5bfd0dae4f38f37f51de559168605 Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Thu, 12 Sep 2024 21:50:47 +0200 Subject: [PATCH 3/9] Document imports --- doc/manual/syntax.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/doc/manual/syntax.md b/doc/manual/syntax.md index 2774bb9d5f..fdbbd777eb 100644 --- a/doc/manual/syntax.md +++ b/doc/manual/syntax.md @@ -1305,4 +1305,29 @@ record is serialized. This includes the output of the `nickel export` command: "{\n \"foo\": 1\n}" ``` +## Imports + +There is special keyword `import`, which can be followed by either a +string literal or a enum tag and a string literal. + +This causes Nickel to read, evaluate and return the specified file. + +The file is searched in directories specified by `NICKEL_IMPORT_PATH` +environment variable or similar command line option, with default being +the current directory. + +Type of the expression depends on import format and, in case of +Nicke source code files, on the type of the expression contained in it. + +One-argument import, like `import "myfile.ncl"`, uses filename extension +to determine the file format. Nickel embeds a short list of known filename +extensions: `ncl`, `json`, `yml`, `yaml`, `toml`, `txt` and +`nix` with a fallback to a Nickel file if there is no extetnsion +or the extension is unknown. + +Two-argument import, like `import 'Raw "test.html"` uses a special enum +tags to determine the format. Currently the tags are `'Nickel`, `'Json`, +`'Yaml`, `'Toml`, `'Raw` and `'Nix`. Some of the formats may be unavailable +depending on compilation options of the Nickel interpreter. + [nix-string-context]: https://shealevy.com/blog/2018/08/05/understanding-nixs-string-context/ From 73eb4e6245e9c1dd100dc0bb90da625a3551377e Mon Sep 17 00:00:00 2001 From: Vitaly Shukela Date: Fri, 13 Sep 2024 14:39:57 +0300 Subject: [PATCH 4/9] Apply suggestions from code review Co-authored-by: Yann Hamdaoui --- core/src/cache.rs | 1 + core/src/pretty.rs | 21 ++++++++++++++++----- doc/manual/syntax.md | 7 ++----- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/core/src/cache.rs b/core/src/cache.rs index bda6711403..8f41ad9211 100644 --- a/core/src/cache.rs +++ b/core/src/cache.rs @@ -58,6 +58,7 @@ impl InputFormat { _ => None, } } + pub fn from_tag(tag: &str) -> Option { Some(match tag { "Json" => InputFormat::Json, diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 995d5733ec..e0e464eafc 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1058,11 +1058,22 @@ where Sealed(_i, _rt, _lbl) => allocator.text("%"), Annotated(annot, rt) => allocator.atom(rt).append(annot.pretty(allocator)), Import { path, format } => { - let mut a = allocator.text("import "); - if Some(*format) != InputFormat::from_path(std::path::Path::new(path.as_os_str())) { - a = a.append("'").append(format.to_tag()).append(" "); - } - a.append(allocator.as_string(path.to_string_lossy()).double_quotes()) + docs![ + allocator + "import", + if Some(*format) != InputFormat::from_path(std::path::Path::new(path.as_os_str())) { + docs![ + allocator, + "'", + format.to_tag(), + allocator.space() + ] + } + else { + allocator.space() + }, + allocator.as_string(path.to_string_lossy()).double_quotes() + ] } ResolvedImport(id) => allocator.text(format!("import ")), // This type is in term position, so we don't need to add parentheses. diff --git a/doc/manual/syntax.md b/doc/manual/syntax.md index fdbbd777eb..d8d7d6d52f 100644 --- a/doc/manual/syntax.md +++ b/doc/manual/syntax.md @@ -1308,7 +1308,7 @@ record is serialized. This includes the output of the `nickel export` command: ## Imports There is special keyword `import`, which can be followed by either a -string literal or a enum tag and a string literal. +string literal or an enum tag and a string literal. This causes Nickel to read, evaluate and return the specified file. @@ -1316,9 +1316,6 @@ The file is searched in directories specified by `NICKEL_IMPORT_PATH` environment variable or similar command line option, with default being the current directory. -Type of the expression depends on import format and, in case of -Nicke source code files, on the type of the expression contained in it. - One-argument import, like `import "myfile.ncl"`, uses filename extension to determine the file format. Nickel embeds a short list of known filename extensions: `ncl`, `json`, `yml`, `yaml`, `toml`, `txt` and @@ -1326,7 +1323,7 @@ extensions: `ncl`, `json`, `yml`, `yaml`, `toml`, `txt` and or the extension is unknown. Two-argument import, like `import 'Raw "test.html"` uses a special enum -tags to determine the format. Currently the tags are `'Nickel`, `'Json`, +tag to determine the format. Currently the tags are `'Nickel`, `'Json`, `'Yaml`, `'Toml`, `'Raw` and `'Nix`. Some of the formats may be unavailable depending on compilation options of the Nickel interpreter. From 7458e7070e51390c0f5557318517004c8e529b5f Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Fri, 13 Sep 2024 22:48:37 +0200 Subject: [PATCH 5/9] minor fix --- core/src/pretty.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/pretty.rs b/core/src/pretty.rs index e0e464eafc..0f77dcfe22 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1059,7 +1059,7 @@ where Annotated(annot, rt) => allocator.atom(rt).append(annot.pretty(allocator)), Import { path, format } => { docs![ - allocator + allocator, "import", if Some(*format) != InputFormat::from_path(std::path::Path::new(path.as_os_str())) { docs![ From 5302eb43ad7d6fbb94acc5e23857ecb1a298da2f Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Fri, 13 Sep 2024 23:25:37 +0200 Subject: [PATCH 6/9] explicit imports: add tests --- core/tests/integration/inputs/imports/explicit.ncl | 10 ++++++++++ .../integration/inputs/imports/explicit_unknowntag.ncl | 6 ++++++ core/tests/integration/inputs/imports/fallback.ncl | 8 ++++++++ .../imports/imported/file_with_unknown_extension.tst | 1 + .../inputs/imports/imported/file_without_extension | 1 + 5 files changed, 26 insertions(+) create mode 100644 core/tests/integration/inputs/imports/explicit.ncl create mode 100644 core/tests/integration/inputs/imports/explicit_unknowntag.ncl create mode 100644 core/tests/integration/inputs/imports/fallback.ncl create mode 100644 core/tests/integration/inputs/imports/imported/file_with_unknown_extension.tst create mode 100644 core/tests/integration/inputs/imports/imported/file_without_extension diff --git a/core/tests/integration/inputs/imports/explicit.ncl b/core/tests/integration/inputs/imports/explicit.ncl new file mode 100644 index 0000000000..b8785426e9 --- /dev/null +++ b/core/tests/integration/inputs/imports/explicit.ncl @@ -0,0 +1,10 @@ +# test.type = 'pass' + +# Note: cannot use repeated file, as it gets cached with the first encountered type +[ + (import 'Nickel "imported/file_without_extension") == 1234, + (import 'Raw "imported/file_with_unknown_extension.tst") == "34+1200\n", + (import 'Raw "imported/empty.yaml") == "", + (import 'Raw "imported/two.ncl") == "# test.type = 'skip'\n1 + 1 : Number\n", +] +|> std.test.assert_all diff --git a/core/tests/integration/inputs/imports/explicit_unknowntag.ncl b/core/tests/integration/inputs/imports/explicit_unknowntag.ncl new file mode 100644 index 0000000000..23102f5ca1 --- /dev/null +++ b/core/tests/integration/inputs/imports/explicit_unknowntag.ncl @@ -0,0 +1,6 @@ +# test.type = 'error' +# +# [test.metadata] +# error = 'ParseError' + +import 'Qqq "imported/empty.yaml" diff --git a/core/tests/integration/inputs/imports/fallback.ncl b/core/tests/integration/inputs/imports/fallback.ncl new file mode 100644 index 0000000000..674b62c07f --- /dev/null +++ b/core/tests/integration/inputs/imports/fallback.ncl @@ -0,0 +1,8 @@ +# test.type = 'pass' + +# Testing that importing files with unknown extension interprets them as Nickel source code +[ + (import "imported/file_without_extension") == 1234, + (import "imported/file_with_unknown_extension.tst") == 1234, +] +|> std.test.assert_all diff --git a/core/tests/integration/inputs/imports/imported/file_with_unknown_extension.tst b/core/tests/integration/inputs/imports/imported/file_with_unknown_extension.tst new file mode 100644 index 0000000000..a4a447b72a --- /dev/null +++ b/core/tests/integration/inputs/imports/imported/file_with_unknown_extension.tst @@ -0,0 +1 @@ +34+1200 diff --git a/core/tests/integration/inputs/imports/imported/file_without_extension b/core/tests/integration/inputs/imports/imported/file_without_extension new file mode 100644 index 0000000000..8ea46cc466 --- /dev/null +++ b/core/tests/integration/inputs/imports/imported/file_without_extension @@ -0,0 +1 @@ +1200+34 From a6c3533c01c2d38b428d5acb8e5dece075048671 Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Sat, 14 Sep 2024 00:26:29 +0200 Subject: [PATCH 7/9] explicit imports: support importing the same file with different formats --- core/src/cache.rs | 38 +++++++++---------- core/src/program.rs | 6 +-- core/src/repl/mod.rs | 2 +- .../integration/inputs/imports/explicit.ncl | 3 +- lsp/nls/src/background.rs | 4 +- lsp/nls/src/cache.rs | 3 +- lsp/nls/src/requests/formatting.rs | 4 +- lsp/nls/src/world.rs | 4 +- utils/src/bench.rs | 6 +-- 9 files changed, 36 insertions(+), 34 deletions(-) diff --git a/core/src/cache.rs b/core/src/cache.rs index 8f41ad9211..4dfae3408c 100644 --- a/core/src/cache.rs +++ b/core/src/cache.rs @@ -32,7 +32,7 @@ use std::time::SystemTime; use void::Void; /// Supported input formats. -#[derive(Default, Clone, Copy, Eq, Debug, PartialEq)] +#[derive(Default, Clone, Copy, Eq, Debug, PartialEq, Hash)] pub enum InputFormat { #[default] Nickel, @@ -84,10 +84,10 @@ impl InputFormat { } } - /// Renturns an [InputFormat] based on the extension of a source path. + /// Extracts format embedded in SourcePath pub fn from_source_path(source_path: &SourcePath) -> Option { - if let SourcePath::Path(p) = source_path { - Self::from_path(p) + if let SourcePath::Path(_p, fmt) = source_path { + Some(*fmt) } else { None } @@ -295,7 +295,7 @@ pub enum SourcePath { /// /// This is the only `SourcePath` variant that can be resolved as the target /// of an import statement. - Path(PathBuf), + Path(PathBuf, InputFormat), /// A subrange of a file at the given path. /// /// This is used by nls to analyze small parts of files that don't fully parse. The @@ -316,7 +316,7 @@ impl<'a> TryFrom<&'a SourcePath> for &'a OsStr { fn try_from(value: &'a SourcePath) -> Result { match value { - SourcePath::Path(p) | SourcePath::Snippet(p) => Ok(p.as_os_str()), + SourcePath::Path(p, _) | SourcePath::Snippet(p) => Ok(p.as_os_str()), _ => Err(()), } } @@ -328,7 +328,7 @@ impl<'a> TryFrom<&'a SourcePath> for &'a OsStr { impl From for OsString { fn from(source_path: SourcePath) -> Self { match source_path { - SourcePath::Path(p) | SourcePath::Snippet(p) => p.into(), + SourcePath::Path(p, _) | SourcePath::Snippet(p) => p.into(), SourcePath::Std(StdlibModule::Std) => "".into(), SourcePath::Std(StdlibModule::Internals) => "".into(), SourcePath::Query => "".into(), @@ -390,13 +390,13 @@ impl Cache { /// Same as [Self::add_file], but assume that the path is already normalized, and take the /// timestamp as a parameter. - fn add_file_(&mut self, path: PathBuf, timestamp: SystemTime) -> io::Result { + fn add_file_(&mut self, path: PathBuf, format: InputFormat, timestamp: SystemTime) -> io::Result { let contents = std::fs::read_to_string(&path)?; let file_id = self.files.add(&path, contents); self.file_paths - .insert(file_id, SourcePath::Path(path.clone())); + .insert(file_id, SourcePath::Path(path.clone(), format)); self.file_ids.insert( - SourcePath::Path(path), + SourcePath::Path(path, format), NameIdEntry { id: file_id, source: SourceKind::Filesystem(timestamp), @@ -409,23 +409,23 @@ impl Cache { /// /// Uses the normalized path and the *modified at* timestamp as the name-id table entry. /// Overrides any existing entry with the same name. - pub fn add_file(&mut self, path: impl Into) -> io::Result { + pub fn add_file(&mut self, path: impl Into, format: InputFormat) -> io::Result { let path = path.into(); let timestamp = timestamp(&path)?; let normalized = normalize_path(&path)?; - self.add_file_(normalized, timestamp) + self.add_file_(normalized, format, timestamp) } /// Try to retrieve the id of a file from the cache. /// /// If it was not in cache, try to read it from the filesystem and add it as a new entry. - pub fn get_or_add_file(&mut self, path: impl Into) -> io::Result> { + pub fn get_or_add_file(&mut self, path: impl Into, format: InputFormat) -> io::Result> { let path = path.into(); let normalized = normalize_path(&path)?; - match self.id_or_new_timestamp_of(path.as_ref())? { + match self.id_or_new_timestamp_of(path.as_ref(), format)? { SourceState::UpToDate(id) => Ok(CacheOp::Cached(id)), SourceState::Stale(timestamp) => { - self.add_file_(normalized, timestamp).map(CacheOp::Done) + self.add_file_(normalized, format, timestamp).map(CacheOp::Done) } } } @@ -980,7 +980,7 @@ impl Cache { /// [normalize_path]). pub fn id_of(&self, name: &SourcePath) -> Option { match name { - SourcePath::Path(p) => match self.id_or_new_timestamp_of(p).ok()? { + SourcePath::Path(p, fmt) => match self.id_or_new_timestamp_of(p, *fmt).ok()? { SourceState::UpToDate(id) => Some(id), SourceState::Stale(_) => None, }, @@ -996,8 +996,8 @@ impl Cache { /// /// The main point of this awkward signature is to minimize I/O operations: if we accessed /// the timestamp, keep it around. - fn id_or_new_timestamp_of(&self, name: &Path) -> io::Result { - match self.file_ids.get(&SourcePath::Path(name.to_owned())) { + fn id_or_new_timestamp_of(&self, name: &Path, format: InputFormat) -> io::Result { + match self.file_ids.get(&SourcePath::Path(name.to_owned(), format)) { None => Ok(SourceState::Stale(timestamp(name)?)), Some(NameIdEntry { id, @@ -1381,7 +1381,7 @@ impl ImportResolver for Cache { .find_map(|parent| { let mut path_buf = parent.clone(); path_buf.push(path); - self.get_or_add_file(&path_buf).ok().map(|x| (x, path_buf)) + self.get_or_add_file(&path_buf, format).ok().map(|x| (x, path_buf)) }) .ok_or_else(|| { let parents = possible_parents diff --git a/core/src/program.rs b/core/src/program.rs index c7d5e880af..63e93ad407 100644 --- a/core/src/program.rs +++ b/core/src/program.rs @@ -220,10 +220,10 @@ impl Program { let mut cache = Cache::new(ErrorTolerance::Strict); let main_id = match input { - Input::Path(path) => cache.add_file(path)?, + Input::Path(path) => cache.add_file(path, InputFormat::Nickel)?, Input::Source(source, name) => { let path = PathBuf::from(name.into()); - cache.add_source(SourcePath::Path(path), source)? + cache.add_source(SourcePath::Path(path, InputFormat::Nickel), source)? } }; @@ -259,7 +259,7 @@ impl Program { Input::Source(source, name) => { let path = PathBuf::from(name.into()); cache - .add_source(SourcePath::Path(path.clone()), source) + .add_source(SourcePath::Path(path.clone(), InputFormat::Nickel), source) .unwrap(); RichTerm::from(Term::Import { path: path.into(), diff --git a/core/src/repl/mod.rs b/core/src/repl/mod.rs index 1e376a10f0..28ef952fa3 100644 --- a/core/src/repl/mod.rs +++ b/core/src/repl/mod.rs @@ -230,7 +230,7 @@ impl Repl for ReplImpl { let file_id = self .vm .import_resolver_mut() - .add_file(OsString::from(path.as_ref())) + .add_file(OsString::from(path.as_ref()), InputFormat::Nickel) .map_err(IOError::from)?; self.vm .import_resolver_mut() diff --git a/core/tests/integration/inputs/imports/explicit.ncl b/core/tests/integration/inputs/imports/explicit.ncl index b8785426e9..5a5118719d 100644 --- a/core/tests/integration/inputs/imports/explicit.ncl +++ b/core/tests/integration/inputs/imports/explicit.ncl @@ -1,8 +1,9 @@ # test.type = 'pass' -# Note: cannot use repeated file, as it gets cached with the first encountered type [ (import 'Nickel "imported/file_without_extension") == 1234, + (import 'Raw "imported/file_without_extension") == "1200+34\n", + (import 'Nickel "imported/file_with_unknown_extension.tst") == 1234, (import 'Raw "imported/file_with_unknown_extension.tst") == "34+1200\n", (import 'Raw "imported/empty.yaml") == "", (import 'Raw "imported/two.ncl") == "# test.type = 'skip'\n1 + 1 : Number\n", diff --git a/lsp/nls/src/background.rs b/lsp/nls/src/background.rs index 1a7b10a351..1d49986d1e 100644 --- a/lsp/nls/src/background.rs +++ b/lsp/nls/src/background.rs @@ -10,7 +10,7 @@ use crossbeam::channel::{bounded, Receiver, RecvTimeoutError, Sender}; use log::warn; use lsp_types::Url; use nickel_lang_core::{ - cache::SourcePath, + cache::{InputFormat, SourcePath}, eval::{cache::CacheImpl, VirtualMachine}, }; use serde::{Deserialize, Serialize}; @@ -93,7 +93,7 @@ pub fn worker_main() -> anyhow::Result<()> { anyhow::bail!("skipping invalid uri {}", eval.eval); }; - if let Some(file_id) = world.cache.id_of(&SourcePath::Path(path.clone())) { + if let Some(file_id) = world.cache.id_of(&SourcePath::Path(path.clone(), InputFormat::Nickel)) { let mut diagnostics = world.parse_and_typecheck(file_id); // Evaluation diagnostics (but only if there were no parse/type errors). diff --git a/lsp/nls/src/cache.rs b/lsp/nls/src/cache.rs index 1f7a1d71f3..5b3c788f0a 100644 --- a/lsp/nls/src/cache.rs +++ b/lsp/nls/src/cache.rs @@ -1,5 +1,6 @@ use codespan::{ByteIndex, FileId}; use lsp_types::{TextDocumentPositionParams, Url}; +use nickel_lang_core::cache::InputFormat; use nickel_lang_core::term::{RichTerm, Term, Traverse}; use nickel_lang_core::{ cache::{Cache, CacheError, CacheOp, EntryState, SourcePath, TermEntry}, @@ -112,7 +113,7 @@ impl CacheExt for Cache { let path = uri .to_file_path() .map_err(|_| crate::error::Error::FileNotFound(uri.clone()))?; - Ok(self.id_of(&SourcePath::Path(path))) + Ok(self.id_of(&SourcePath::Path(path, InputFormat::Nickel))) } fn position( diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index b5f0058fca..0ce84fa5ff 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -1,6 +1,6 @@ use lsp_server::{RequestId, Response, ResponseError}; use lsp_types::{DocumentFormattingParams, Position, Range, TextEdit}; -use nickel_lang_core::cache::SourcePath; +use nickel_lang_core::cache::{InputFormat, SourcePath}; use crate::{error::Error, files::uri_to_path, server::Server}; @@ -13,7 +13,7 @@ pub fn handle_format_document( server: &mut Server, ) -> Result<(), ResponseError> { let path = uri_to_path(¶ms.text_document.uri)?; - let file_id = server.world.cache.id_of(&SourcePath::Path(path)).unwrap(); + let file_id = server.world.cache.id_of(&SourcePath::Path(path, InputFormat::Nickel)).unwrap(); let text = server.world.cache.files().source(file_id).clone(); let document_length = text.lines().count() as u32; diff --git a/lsp/nls/src/world.rs b/lsp/nls/src/world.rs index 7c8cb0eaaa..63eff995f0 100644 --- a/lsp/nls/src/world.rs +++ b/lsp/nls/src/world.rs @@ -91,7 +91,7 @@ impl World { // Replace the path (as opposed to adding it): we may already have this file in the // cache if it was imported by an already-open file. - let file_id = self.cache.replace_string(SourcePath::Path(path), contents); + let file_id = self.cache.replace_string(SourcePath::Path(path, InputFormat::Nickel), contents); // The cache automatically invalidates reverse-dependencies; we also need // to track them, so that we can clear our own analysis. @@ -120,7 +120,7 @@ impl World { contents: String, ) -> anyhow::Result<(FileId, Vec)> { let path = uri_to_path(&uri)?; - let file_id = self.cache.replace_string(SourcePath::Path(path), contents); + let file_id = self.cache.replace_string(SourcePath::Path(path, InputFormat::Nickel), contents); let invalid = self.cache.invalidate_cache(file_id); for f in &invalid { diff --git a/utils/src/bench.rs b/utils/src/bench.rs index 6179e07266..da40fd9a22 100644 --- a/utils/src/bench.rs +++ b/utils/src/bench.rs @@ -1,6 +1,6 @@ use criterion::Criterion; use nickel_lang_core::{ - cache::{Cache, Envs, ErrorTolerance}, + cache::{Cache, Envs, ErrorTolerance, InputFormat}, eval::{ cache::{Cache as EvalCache, CacheImpl}, VirtualMachine, @@ -112,7 +112,7 @@ pub fn bench_terms<'r>(rts: Vec>) -> Box b.iter_batched( || { let mut cache = cache.clone(); - let id = cache.add_file(bench.path()).unwrap(); + let id = cache.add_file(bench.path(), InputFormat::Nickel).unwrap(); let t = import_resolution::strict::resolve_imports(t.clone(), &mut cache) .unwrap() .transformed_term; @@ -202,7 +202,7 @@ macro_rules! ncl_bench_group { b.iter_batched( || { let mut cache = cache.clone(); - let id = cache.add_file(bench.path()).unwrap(); + let id = cache.add_file(bench.path(), InputFormat::Nickel).unwrap(); let t = resolve_imports(t.clone(), &mut cache) .unwrap() .transformed_term; From d6f9aff0d86465b192b27cbfc69e8748dccb916b Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Tue, 17 Sep 2024 14:07:49 +0200 Subject: [PATCH 8/9] fmt --- core/src/cache.rs | 34 +++++++++++++++++++++++------- core/src/pretty.rs | 24 +++++++++------------ lsp/nls/src/background.rs | 5 ++++- lsp/nls/src/requests/formatting.rs | 6 +++++- lsp/nls/src/world.rs | 8 +++++-- 5 files changed, 51 insertions(+), 26 deletions(-) diff --git a/core/src/cache.rs b/core/src/cache.rs index 4dfae3408c..e120f1106f 100644 --- a/core/src/cache.rs +++ b/core/src/cache.rs @@ -390,7 +390,12 @@ impl Cache { /// Same as [Self::add_file], but assume that the path is already normalized, and take the /// timestamp as a parameter. - fn add_file_(&mut self, path: PathBuf, format: InputFormat, timestamp: SystemTime) -> io::Result { + fn add_file_( + &mut self, + path: PathBuf, + format: InputFormat, + timestamp: SystemTime, + ) -> io::Result { let contents = std::fs::read_to_string(&path)?; let file_id = self.files.add(&path, contents); self.file_paths @@ -409,7 +414,11 @@ impl Cache { /// /// Uses the normalized path and the *modified at* timestamp as the name-id table entry. /// Overrides any existing entry with the same name. - pub fn add_file(&mut self, path: impl Into, format: InputFormat) -> io::Result { + pub fn add_file( + &mut self, + path: impl Into, + format: InputFormat, + ) -> io::Result { let path = path.into(); let timestamp = timestamp(&path)?; let normalized = normalize_path(&path)?; @@ -419,14 +428,18 @@ impl Cache { /// Try to retrieve the id of a file from the cache. /// /// If it was not in cache, try to read it from the filesystem and add it as a new entry. - pub fn get_or_add_file(&mut self, path: impl Into, format: InputFormat) -> io::Result> { + pub fn get_or_add_file( + &mut self, + path: impl Into, + format: InputFormat, + ) -> io::Result> { let path = path.into(); let normalized = normalize_path(&path)?; match self.id_or_new_timestamp_of(path.as_ref(), format)? { SourceState::UpToDate(id) => Ok(CacheOp::Cached(id)), - SourceState::Stale(timestamp) => { - self.add_file_(normalized, format, timestamp).map(CacheOp::Done) - } + SourceState::Stale(timestamp) => self + .add_file_(normalized, format, timestamp) + .map(CacheOp::Done), } } @@ -997,7 +1010,10 @@ impl Cache { /// The main point of this awkward signature is to minimize I/O operations: if we accessed /// the timestamp, keep it around. fn id_or_new_timestamp_of(&self, name: &Path, format: InputFormat) -> io::Result { - match self.file_ids.get(&SourcePath::Path(name.to_owned(), format)) { + match self + .file_ids + .get(&SourcePath::Path(name.to_owned(), format)) + { None => Ok(SourceState::Stale(timestamp(name)?)), Some(NameIdEntry { id, @@ -1381,7 +1397,9 @@ impl ImportResolver for Cache { .find_map(|parent| { let mut path_buf = parent.clone(); path_buf.push(path); - self.get_or_add_file(&path_buf, format).ok().map(|x| (x, path_buf)) + self.get_or_add_file(&path_buf, format) + .ok() + .map(|x| (x, path_buf)) }) .ok_or_else(|| { let parents = possible_parents diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 0f77dcfe22..c1795740c2 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1059,20 +1059,16 @@ where Annotated(annot, rt) => allocator.atom(rt).append(annot.pretty(allocator)), Import { path, format } => { docs![ - allocator, - "import", - if Some(*format) != InputFormat::from_path(std::path::Path::new(path.as_os_str())) { - docs![ - allocator, - "'", - format.to_tag(), - allocator.space() - ] - } - else { - allocator.space() - }, - allocator.as_string(path.to_string_lossy()).double_quotes() + allocator, + "import", + if Some(*format) + != InputFormat::from_path(std::path::Path::new(path.as_os_str())) + { + docs![allocator, "'", format.to_tag(), allocator.space()] + } else { + allocator.space() + }, + allocator.as_string(path.to_string_lossy()).double_quotes() ] } ResolvedImport(id) => allocator.text(format!("import ")), diff --git a/lsp/nls/src/background.rs b/lsp/nls/src/background.rs index 1d49986d1e..e58b6419a5 100644 --- a/lsp/nls/src/background.rs +++ b/lsp/nls/src/background.rs @@ -93,7 +93,10 @@ pub fn worker_main() -> anyhow::Result<()> { anyhow::bail!("skipping invalid uri {}", eval.eval); }; - if let Some(file_id) = world.cache.id_of(&SourcePath::Path(path.clone(), InputFormat::Nickel)) { + if let Some(file_id) = world + .cache + .id_of(&SourcePath::Path(path.clone(), InputFormat::Nickel)) + { let mut diagnostics = world.parse_and_typecheck(file_id); // Evaluation diagnostics (but only if there were no parse/type errors). diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index 0ce84fa5ff..4c8d172667 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -13,7 +13,11 @@ pub fn handle_format_document( server: &mut Server, ) -> Result<(), ResponseError> { let path = uri_to_path(¶ms.text_document.uri)?; - let file_id = server.world.cache.id_of(&SourcePath::Path(path, InputFormat::Nickel)).unwrap(); + let file_id = server + .world + .cache + .id_of(&SourcePath::Path(path, InputFormat::Nickel)) + .unwrap(); let text = server.world.cache.files().source(file_id).clone(); let document_length = text.lines().count() as u32; diff --git a/lsp/nls/src/world.rs b/lsp/nls/src/world.rs index 63eff995f0..5847e5224b 100644 --- a/lsp/nls/src/world.rs +++ b/lsp/nls/src/world.rs @@ -91,7 +91,9 @@ impl World { // Replace the path (as opposed to adding it): we may already have this file in the // cache if it was imported by an already-open file. - let file_id = self.cache.replace_string(SourcePath::Path(path, InputFormat::Nickel), contents); + let file_id = self + .cache + .replace_string(SourcePath::Path(path, InputFormat::Nickel), contents); // The cache automatically invalidates reverse-dependencies; we also need // to track them, so that we can clear our own analysis. @@ -120,7 +122,9 @@ impl World { contents: String, ) -> anyhow::Result<(FileId, Vec)> { let path = uri_to_path(&uri)?; - let file_id = self.cache.replace_string(SourcePath::Path(path, InputFormat::Nickel), contents); + let file_id = self + .cache + .replace_string(SourcePath::Path(path, InputFormat::Nickel), contents); let invalid = self.cache.invalidate_cache(file_id); for f in &invalid { From 75bd460ad710626d2ec7e9ad982c6bc59341b81f Mon Sep 17 00:00:00 2001 From: Vitaly _Vi Shukela Date: Tue, 17 Sep 2024 21:02:53 +0200 Subject: [PATCH 9/9] Reinforce explicit imports test against some whitespace changes. --- core/tests/integration/inputs/imports/explicit.ncl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/tests/integration/inputs/imports/explicit.ncl b/core/tests/integration/inputs/imports/explicit.ncl index 5a5118719d..dfa8be93f0 100644 --- a/core/tests/integration/inputs/imports/explicit.ncl +++ b/core/tests/integration/inputs/imports/explicit.ncl @@ -2,10 +2,10 @@ [ (import 'Nickel "imported/file_without_extension") == 1234, - (import 'Raw "imported/file_without_extension") == "1200+34\n", + (import 'Raw "imported/file_without_extension") |> std.string.is_match "^1200\\+34\\s*$", (import 'Nickel "imported/file_with_unknown_extension.tst") == 1234, - (import 'Raw "imported/file_with_unknown_extension.tst") == "34+1200\n", + (import 'Raw "imported/file_with_unknown_extension.tst") |> std.string.is_match "^34\\+1200\\s*$", (import 'Raw "imported/empty.yaml") == "", - (import 'Raw "imported/two.ncl") == "# test.type = 'skip'\n1 + 1 : Number\n", + (import 'Raw "imported/two.ncl") |> std.string.is_match "^\\s*\\#", ] |> std.test.assert_all