Fix an issue where closure rewriting required class internals #1175
Conversation
Make sure that we don't actually try to expose something when they've already been written, causing an internal tool panic rather than wrong JS.
|
@fitzgen I'm curious if you have thoughts on this, we've had a good number of bugs about the ordering of operations in Do you have thoughts on how we might better structure this and/or fix this for good in terms of JS generation? |
crates/cli-support/src/js/mod.rs
Outdated
| @@ -478,6 +484,10 @@ impl<'a> Context<'a> { | |||
| self.rewrite_imports(module_name); | |||
| self.update_producers_section(); | |||
|
|
|||
| // Cause any future calls to `should_write_global` to panic, making sure | |||
| // we don't ask for items which we can no longer emit. | |||
| self.globals_written = true; | |||
There was a problem hiding this comment.
Alternatively, exposed_globals could be an Option<...> and then we can have should_write_global do self.exposed_globals.as_mut().unwrap().insert(name) and then here do self.exposed_globals.take().unwrap().
This is equivalent to having a bool flag (which becomes the option discriminant in this scheme) but it is impossible to forget to check the flag in should_write_global or anything else that wants to touch exposed_globals thanks to the type system.
| @@ -56,7 +56,7 @@ pub struct Context<'a> { | |||
| /// wasm-bindgen emits. | |||
| pub direct_imports: HashMap<&'a str, (&'a str, &'a str)>, | |||
|
|
|||
| pub exported_classes: HashMap<String, ExportedClass>, | |||
| pub exported_classes: Option<HashMap<String, ExportedClass>>, | |||
There was a problem hiding this comment.
Ah ok, this second commit is exactly the pattern that I was suggesting for the first one :-p
Throw it in an `Option` and then `take()` it when we consume it to ensure that future calls to insert data into it panic instead of producing inconsistent JS.
Surfaced through previous sanity-checking commits, this reorders some internal operations to... Closes rustwasm#1174
alexcrichton
force-pushed
the
internal-consistency
branch
from
f0bc180
to
20e871f
Compare
Surfaced through previous sanity-checking commits, this reorders some
internal operations to...
Closes #1174