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

Remove different behaviour when single input #1902

Merged
merged 1 commit into from
May 7, 2024
Merged

Remove different behaviour when single input #1902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 19 additions & 46 deletions core/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ impl InputFormat {
_ => None,
}
}
/// Renturns an [InputFormat] based on the extension of a source path.
pub fn from_source_path(source_path: &SourcePath) -> Option<InputFormat> {
if let SourcePath::Path(p) = source_path {
Self::from_path(p)
} else {
None
}
}
}

/// File and terms cache.
Expand Down Expand Up @@ -456,50 +464,10 @@ impl Cache {
}
}

/// Parse a source and populate the corresponding entry in the cache, or do nothing if the
/// entry has already been parsed. This function is error tolerant: parts of the source which
/// result in parse errors are parsed as [`crate::term::Term::ParseError`] and the
/// corresponding error messages are collected and returned.
///
/// The `Err` part of the result corresponds to non-recoverable errors.
fn parse_lax(&mut self, file_id: FileId) -> Result<CacheOp<ParseErrors>, ParseError> {
if let Some(TermEntry { parse_errs, .. }) = self.terms.get(&file_id) {
Ok(CacheOp::Cached(parse_errs.clone()))
} else {
let (term, parse_errs) = self.parse_nocache(file_id)?;
self.terms.insert(
file_id,
TermEntry {
term,
state: EntryState::Parsed,
parse_errs: parse_errs.clone(),
},
);

Ok(CacheOp::Done(parse_errs))
}
}

/// Parse a source and populate the corresponding entry in the cache, or do
/// nothing if the entry has already been parsed. This function is error
/// tolerant if `self.error_tolerant` is `true`.
pub fn parse(&mut self, file_id: FileId) -> Result<CacheOp<ParseErrors>, ParseErrors> {
let result = self.parse_lax(file_id);

match self.error_tolerance {
ErrorTolerance::Tolerant => result.map_err(|err| err.into()),
ErrorTolerance::Strict => match result? {
CacheOp::Done(e) | CacheOp::Cached(e) if !e.no_errors() => Err(e),
CacheOp::Done(_) => Ok(CacheOp::Done(ParseErrors::none())),
CacheOp::Cached(_) => Ok(CacheOp::Cached(ParseErrors::none())),
},
}
}

/// Parse a source and populate the corresponding entry in the cache, or do
/// nothing if the entry has already been parsed. Support multiple formats.
/// This function is always error tolerant, independently from `self.error_tolerant`.
fn parse_multi_lax(
fn parse_lax(
&mut self,
file_id: FileId,
format: InputFormat,
Expand All @@ -523,12 +491,12 @@ impl Cache {
/// Parse a source and populate the corresponding entry in the cache, or do
/// nothing if the entry has already been parsed. Support multiple formats.
/// This function is error tolerant if `self.error_tolerant` is `true`.
pub fn parse_multi(
pub fn parse(
&mut self,
file_id: FileId,
format: InputFormat,
) -> Result<CacheOp<ParseErrors>, ParseErrors> {
let result = self.parse_multi_lax(file_id, format);
let result = self.parse_lax(file_id, format);

match self.error_tolerance {
ErrorTolerance::Tolerant => result.map_err(|err| err.into()),
Expand Down Expand Up @@ -902,7 +870,12 @@ impl Cache {
) -> Result<CacheOp<()>, Error> {
let mut result = CacheOp::Cached(());

if let CacheOp::Done(_) = self.parse(file_id)? {
let format = self
.file_paths
.get(&file_id)
.and_then(InputFormat::from_source_path)
.unwrap_or_default();
if let CacheOp::Done(_) = self.parse(file_id, format)? {
result = CacheOp::Done(());
}

Expand Down Expand Up @@ -1134,7 +1107,7 @@ impl Cache {
.collect();

for (_, file_id) in file_ids.iter() {
self.parse(*file_id)?;
self.parse(*file_id, InputFormat::Nickel)?;
}
self.stdlib_ids.replace(file_ids);
Ok(CacheOp::Done(()))
Expand Down Expand Up @@ -1371,7 +1344,7 @@ impl ImportResolver for Cache {
self.rev_imports.entry(file_id).or_default().insert(parent);
}

self.parse_multi(file_id, format)
self.parse(file_id, format)
.map_err(|err| ImportError::ParseErrors(err, *pos))?;

Ok((result, file_id))
Expand Down
6 changes: 4 additions & 2 deletions core/src/program.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ impl<EC: EvalCache> Program<EC> {
pub fn parse(&mut self) -> Result<RichTerm, Error> {
self.vm
.import_resolver_mut()
.parse(self.main_id)
.parse(self.main_id, InputFormat::Nickel)
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 if those shouldn't guess the format as well, instead of InputFormat::Nickel. That's why it could be handy to have a parse_auto (the name isn't great, so feel free to use another if you find some!). However, this one can also be done in a subsequent PR, as the current code at least fixes the original issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_infer_format? less concise, but more descriptive ...

Copy link
Member

Choose a reason for hiding this comment

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

@olorin37 right, it's a reasonable suggestion and better than parse_auto, so go for it 🙂

.map_err(Error::ParseErrors)?;
Ok(self
.vm
Expand Down Expand Up @@ -496,7 +496,9 @@ impl<EC: EvalCache> Program<EC> {

/// Load, parse, and typecheck the program and the standard library, if not already done.
pub fn typecheck(&mut self) -> Result<(), Error> {
self.vm.import_resolver_mut().parse(self.main_id)?;
self.vm
.import_resolver_mut()
.parse(self.main_id, InputFormat::Nickel)?;
Copy link
Member

Choose a reason for hiding this comment

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

Same, I think any parse from within Program methods should probably try to guess the format, to have the same behavior as prepare.

self.vm.import_resolver_mut().load_stdlib()?;
let initial_env = self.vm.import_resolver().mk_type_ctxt().expect(
"program::typecheck(): \
Expand Down
6 changes: 4 additions & 2 deletions core/src/repl/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
//! Dually, the frontend is the user-facing part, which may be a CLI, a web application, a
//! jupyter-kernel (which is not exactly user-facing, but still manages input/output and
//! formatting), etc.
use crate::cache::{Cache, Envs, ErrorTolerance, SourcePath};
use crate::cache::{Cache, Envs, ErrorTolerance, InputFormat, SourcePath};
use crate::error::{
report::{self, ColorOpt, ErrorFormat},
Error, EvalError, IOError, IntoDiagnostics, ParseError, ParseErrors, ReplError,
Expand Down Expand Up @@ -232,7 +232,9 @@ impl<EC: EvalCache> Repl for ReplImpl<EC> {
.import_resolver_mut()
.add_file(OsString::from(path.as_ref()))
.map_err(IOError::from)?;
self.vm.import_resolver_mut().parse(file_id)?;
self.vm
.import_resolver_mut()
.parse(file_id, InputFormat::Nickel)?;

let term = self.vm.import_resolver().get_owned(file_id).unwrap();
let pos = term.pos;
Expand Down
4 changes: 2 additions & 2 deletions lsp/nls/src/incomplete.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use std::path::PathBuf;

use nickel_lang_core::{
cache::SourcePath,
cache::{InputFormat, SourcePath},
parser::lexer::{self, NormalToken, SpannedToken, Token},
position::RawSpan,
term::{RichTerm, Term},
Expand Down Expand Up @@ -102,7 +102,7 @@ fn resolve_imports(rt: RichTerm, world: &mut World) -> RichTerm {
} = import_resolution::tolerant::resolve_imports(rt, &mut world.cache);

for id in resolved_ids {
if world.cache.parse(id).is_ok() {
if world.cache.parse(id, InputFormat::Nickel).is_ok() {
// If a new input got imported in an incomplete term, try to typecheck
// (and build lookup tables etc.) for it, but don't issue diagnostics.
let _ = world.typecheck(id);
Expand Down
4 changes: 2 additions & 2 deletions lsp/nls/src/world.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use codespan::FileId;
use lsp_server::{ErrorCode, ResponseError};
use lsp_types::Url;
use nickel_lang_core::{
cache::{Cache, CacheError, ErrorTolerance, SourcePath},
cache::{Cache, CacheError, ErrorTolerance, InputFormat, SourcePath},
error::{ImportError, IntoDiagnostics},
position::{RawPos, RawSpan},
term::{record::FieldMetadata, RichTerm, Term, UnaryOp},
Expand Down Expand Up @@ -160,7 +160,7 @@ impl World {
file_id: FileId,
) -> Result<Vec<SerializableDiagnostic>, Vec<SerializableDiagnostic>> {
self.cache
.parse(file_id)
.parse(file_id, InputFormat::Nickel)
.map(|nonfatal| self.lsp_diagnostics(file_id, nonfatal.inner()))
.map_err(|fatal| self.lsp_diagnostics(file_id, fatal))
}
Expand Down
4 changes: 2 additions & 2 deletions utils/src/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ macro_rules! ncl_bench_group {
(name = $group_name:ident; config = $config:expr; $($b:tt),+ $(,)*) => {
pub fn $group_name() {
use nickel_lang_core::{
cache::{Envs, Cache, ErrorTolerance, ImportResolver},
cache::{Envs, Cache, ErrorTolerance, ImportResolver, InputFormat},
eval::{VirtualMachine, cache::{CacheImpl, Cache as EvalCache}},
transform::import_resolution::strict::resolve_imports,
error::report::{report, ColorOpt, ErrorFormat},
Expand All @@ -194,7 +194,7 @@ macro_rules! ncl_bench_group {
.unwrap()
.transformed_term;
if bench.eval_mode == $crate::bench::EvalMode::TypeCheck {
cache.parse(id).unwrap();
cache.parse(id, InputFormat::Nickel).unwrap();
Copy link
Member

@yannham yannham May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you should either:

  1. Use a fully qualified name for InputFormat, that is nickel_lang_core::cache::InputFormat
  2. Import it a few line above, inside the macro, and keep it as InputFormat

But in any case, the consumers/callers of ncl_bench_group should (Edit: + NOT) have to import it themselves (as you had to do in various bench files). This leads to code duplication, but more importantly, callers of nlc_bench_group have to magically guess that they need to do this import (or wait for a compilation error), while the macro should - and can - be self-contained.

Copy link
Member

Choose a reason for hiding this comment

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

Edit: I meant should NOT have to import it themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, right now I see. I missed that the code I edited was inside a macro, sure it's a mistake. So, I'll import it in macro, and get rid of those imports from benches:)

cache.resolve_imports(id).unwrap();
}
(cache, id, t)
Expand Down
Loading