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

Jump to bottom

Fix REPL panic on transitive imports #1474

Merged
merged 3 commits into from
Jul 28, 2023
Merged

Fix REPL panic on transitive imports #1474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3b830e
report errors on transtive imports
Radvendii Jul 25, 2023
1f6d451
slight refactor of resolve_imports
Radvendii Jul 25, 2023
6282ac3
clippy
Radvendii Jul 26, 2023
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
118 changes: 59 additions & 59 deletions core/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,69 +688,69 @@ impl Cache {
file_id: FileId,
) -> Result<CacheOp<(Vec<FileId>, Vec<ImportError>)>, CacheError<ImportError>> {
match self.entry_state(file_id) {
Some(state) if state >= EntryState::ImportsResolved => {
Ok(CacheOp::Cached((Vec::new(), Vec::new())))
}
Some(state) if state >= EntryState::Parsed => {
let (pending, errors) = if state < EntryState::ImportsResolving {
let TermEntry { term, .. } = self.terms.get(&file_id).unwrap();
let term = term.clone();

let import_resolution::tolerant::ResolveResult {
transformed_term,
resolved_ids: pending,
import_errors,
} = match self.error_tolerance {
ErrorTolerance::Tolerant => {
import_resolution::tolerant::resolve_imports(term, self)
}
ErrorTolerance::Strict => {
import_resolution::strict::resolve_imports(term, self)?.into()
}
};

// unwrap!(): we called `unwrap()` at the beggining of the enclosing if branch
// on the result of `self.terms.get(&file_id)`. We only made recursive calls to
// `resolve_imports` in between, which don't remove anything from `self.terms`.
let cached_term = self.terms.get_mut(&file_id).unwrap();
cached_term.term = transformed_term;
cached_term.state = EntryState::ImportsResolving;

let mut done = Vec::new();

// Transitively resolve the imports, and accumulate the ids of the resolved
// files along the way.
for id in pending {
if let CacheOp::Done((mut done_local, _)) = self.resolve_imports(id)? {
done.push(id);
done.append(&mut done_local)
}
Some(EntryState::Parsed) => {
let TermEntry { term, .. } = self.terms.get(&file_id).unwrap();
let term = term.clone();

let import_resolution::tolerant::ResolveResult {
transformed_term,
resolved_ids: pending,
import_errors,
} = match self.error_tolerance {
ErrorTolerance::Tolerant => {
import_resolution::tolerant::resolve_imports(term, self)
}
ErrorTolerance::Strict => {
import_resolution::strict::resolve_imports(term, self)?.into()
}

self.update_state(file_id, EntryState::ImportsResolved);
(done, import_errors)
} else {
// [transitory_entry_state]:
//
// In this branch, the state is `EntryState::ImportsResolving`. Which means we
// recursively called `resolve_imports` on the same entry: there is a cyclic
// import. In that case, we don't do anything, as the entry is being treated
// by a on-going call to `resolve_import` higher up in the call chain.
//
// Note that in some cases, this intermediate state can be observed by an
// external caller: if a first call to `resolve_imports` fails in the middle of
// resolving the transitive imports, the end state of the entry is
// `ImportsResolving`. Subsequent calls to `resolve_imports` will succeed, but
// won't change the state to `EntryState::ImportsResolved` (and for a good
// reason: we wouldn't even know what are the pending imports to resolve). The
// Nickel pipeline should however fail if `resolve_imports` failed at some
// point, anyway.
(Vec::new(), Vec::new())
};

Ok(CacheOp::Done((pending, errors)))
// unwrap!(): we called `unwrap()` at the beggining of the enclosing if branch
// on the result of `self.terms.get(&file_id)`. We only made recursive calls to
// `resolve_imports` in between, which don't remove anything from `self.terms`.
let cached_term = self.terms.get_mut(&file_id).unwrap();
cached_term.term = transformed_term;
cached_term.state = EntryState::ImportsResolving;

let mut done = Vec::new();

// Transitively resolve the imports, and accumulate the ids of the resolved
// files along the way.
for id in pending {
if let CacheOp::Done((mut done_local, _)) = self.resolve_imports(id)? {
done.push(id);
done.append(&mut done_local)
}
}

self.update_state(file_id, EntryState::ImportsResolved);

Ok(CacheOp::Done((done, import_errors)))
}
_ => Err(CacheError::NotParsed),
// [transitory_entry_state]:
//
// This case is triggered by a cyclic import. The entry is already
// being treated by an ongoing call to `resolve_import` higher up in
// the call chain, so we don't do anything here.
//
// Note that in some cases, this intermediate state can be observed by an
// external caller: if a first call to `resolve_imports` fails in the middle of
// resolving the transitive imports, the end state of the entry is
// `ImportsResolving`. Subsequent calls to `resolve_imports` will succeed, but
// won't change the state to `EntryState::ImportsResolved` (and for a good
// reason: we wouldn't even know what are the pending imports to resolve). The
// Nickel pipeline should however fail if `resolve_imports` failed at some
// point, anyway.
Some(EntryState::ImportsResolving) => Ok(CacheOp::Done((Vec::new(), Vec::new()))),
// >= EntryState::ImportsResolved
Some(
EntryState::ImportsResolved
| EntryState::Typechecking
| EntryState::Typechecked
| EntryState::Transforming
| EntryState::Transformed,
Comment on lines +745 to +751
Copy link
Member

Choose a reason for hiding this comment

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

I believe one case such as Some(state) if state >= EntryState::ImportResolved is equally clear and a bit less verbose. Enumerating the cases has the advantages that if we ever add a phase, this forces us to update this part of this code (and thus to think about what we should do), but I fee like the ordering is enough here: whatever are the next phases, if we're past ImportResovled, there's nothing to do.

I really don't feel strongly about it though, feel free to ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that would be better. The disadvantage is that then rust can't tell the pattern is exhaustive, so we have to either add a _ => unreachable!() or change None to a wildcard match. Before, None was a wildcard match, and that was one of the things that confused me. ("what can this be besides None?? Oh, it can't.")

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Haven't thought about that. If it confused you, it's a sign that it might be better indeed (given than this version is more verbose but not unclear).

) => Ok(CacheOp::Cached((Vec::new(), Vec::new()))),
None => Err(CacheError::NotParsed),
}
}

Expand Down
7 changes: 6 additions & 1 deletion core/src/repl/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,12 @@ impl<EC: EvalCache> ReplImpl<EC> {
resolved_ids: pending,
} = import_resolution::strict::resolve_imports(t, self.vm.import_resolver_mut())?;
for id in &pending {
dbg!(self.vm.import_resolver_mut().resolve_imports(*id)).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Ah, an unwrap without explaining why it's assumed to be ok...good example that this is a bad thing 🙃

self.vm
.import_resolver_mut()
.resolve_imports(*id)
.map_err(|cache_err| {
cache_err.unwrap_error("repl::eval_(): expected imports to be parsed")
})?;
}

let wildcards =
Expand Down