-
-
Notifications
You must be signed in to change notification settings - Fork 405
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
Mitigate unsoundness in InternalState
#387
Conversation
cfc103a
to
92e0b7c
Compare
} | ||
} | ||
|
||
/// This trait must be implemented by all structs used for internal state. | ||
pub trait InternalState: Debug {} | ||
pub trait InternalState: Debug + Trace + Any {} |
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.
It seems that this is not enough to be able to call downcast_mut()
on self.state
. It gives the following error:
method not found in `&(dyn builtins::object::internal_state::InternalState + 'static)`
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.
After some refactoring, I'm getting this error:
error[E0605]: non-primitive cast: `&dyn builtins::object::internal_state::InternalState` as `&(dyn std::any::Any + 'static)`
--> boa/src/builtins/object/internal_state.rs:44:9
|
44 | (state as &dyn Any).downcast_ref::<T>()
| ^^^^^^^^^^^^^^^^^^^
|
= note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait
error[E0605]: non-primitive cast: `&mut (dyn builtins::object::internal_state::InternalState + 'static)` as `&mut (dyn std::any::Any + 'static)`
--> boa/src/builtins/object/internal_state.rs:51:9
|
51 | (state as &mut dyn Any).downcast_mut::<T>()
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait
Superseded by #626 |
This comes from #363 (comment). It turns out that implementing an empty trace was not safe if the contained type was not
Trace
.I'm still having issues, though, since we cannot downcast a non
Any
type. AddingAny
to theInternalState
trait bounds doesn't help either. Any idea on how to fix this, @HalidOdat or @jasonwilliams ?This still has the issue that
Regex
does not implementTrace
, and in the future it could, which would make it unsound again. In any case, I think this is a good compromise for now. In the future we will probably need to change theRegex
for a different thing, in any case, since it doesn't implement everything in the JavaScript regular expressions.