-
Notifications
You must be signed in to change notification settings - Fork 88
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.
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
Fix REPL panic on transitive imports #1474
Conversation
as it was before, we would crash
Is there an issue linked to this PR? A small sum up of what the problem was and how it was solved could help a bit, because honestly I have a bit of hard time understanding the description as it is. |
I believe the issue is #1443 |
Thanks @jneem . I still think a short summary of why the bug occurred (which isn't really contained in the issue) and how it has been fixed would be useful |
There are two kinds of errors that could in theory occur at that point in the code, So I changed it to propagate those errors instead of ignoring them. The whole change is essentially just this: .unwrap()
becomes
.map_err(|cache_err| { cache_err.unwrap_error("repl::eval_(): expected imports to be parsed")})? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, all clear now! The change LGTM and I find the match
statement to read clearer that way.
// >= EntryState::ImportsResolved | ||
Some( | ||
EntryState::ImportsResolved | ||
| EntryState::Typechecking | ||
| EntryState::Typechecked | ||
| EntryState::Transforming | ||
| EntryState::Transformed, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.")
There was a problem hiding this comment.
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).
@@ -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(); |
There was a problem hiding this comment.
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 🙃
I also refactored one of the functions i had trouble understanding. I don't feel too strongly about it though if someone else objects to the refactor.
Something that took me a while to track down, and I still am having a little trouble understanding:
We can safely call
unwrap_error()
because the only case where.resolve_imports()
returnsNotParsed
is when the call toentry_state()
, in other wordsterms.get()
isNone
.And we've already called
import-resolution::strict::resolve_imports()
, and if I'm reading the code correctly everything passed back inresolved_ids
must have been put interms
(ultimately byparse_lax_multi()
)My confusion is... we haven't parsed the file at that point though, right? We clearly parse them during hte call to
.resolve_imports()
, because that's where parse errors are being generated. But they must already have state>= EntryState::Parsed
before that call.Also, do we have any mechanism for testing the repl?