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.

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

#14902 Write a MIR lint for rooting analysis #20264

Closed
wants to merge 15 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Added more exceptions to the plugin; servo should compile

  • Loading branch information
mrowqa committed May 11, 2018
commit 3c14830e678b8e57a7d9251169d305d98c744bb0
@@ -13,12 +13,13 @@ use std::sync::mpsc::{Receiver, Sender};
/// common event loop messages. While this SendableWorkerScriptChan is alive, the associated
/// Worker object will remain alive.
#[derive(Clone, JSTraceable)]
pub struct SendableWorkerScriptChan<T: DomObject> {
#[must_root]
pub struct SendableWorkerScriptChan<#[must_root] T: DomObject> {

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

I'm confused by these changes - Trusted<T> is a type that does not require rooting (due to the allow_unrooted_interior attribute), so it's not clear why T needs to be marked must_root here.

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

Sorry, I don't understand the relation between Trusted<T> and this place. I was putting #[must_root] in some places I thought it should be, without further understanding all of the code (there were too many errors).

For methods generic over T that we want to be able to accept #[must_root] we must specify explicitly that it is #[must_root] or mark the method with allow_unrooted_interior and/or allow(unrooted_must_root) (not sure which one).

This comment has been minimized.

Copy link
@jdm

jdm Jun 4, 2018

Member

The reason I brought it up is that T is only used as an argument for the Trusted<T> member that is stored in SendableWorkerScriptChan. Since Trusted<T> is marked #[allow_unrooted_interior] it is allowed to be stored anywhere. SendableWorkerScriptChan should not be marked #[must_root] in that case. Is that case possible to represent with the current plugin?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 5, 2018

Author

Oh, I see. I didn't notice the field 2 lines below. I got your point.

I don't remember how exactly all of these checks work. As I wrote somewhere else, here we are dealing with struct definition which are handled only on the HIR level if I remember correctly.

The intention with using #[must_root] is that it should be properly and explicitly propagated. So if we want T to be able to represent a #[must_root] type, then it should be marked as a #[must_root] type. Then, if we pass T to Trusted<T> and it is marked with allow_unrooted_interior, it means that Trusted<T> isn't #[must_root], so the whole SendableWorkerScriptChan structure doesn't have to be #[must_root], because it consists of regular fields.

Why is SendableWorkerScriptChan a #[must_root]? I marked it because there were some compilation errors, I believe. It is the heuristics I wrote about here and there.

There is also one side note: I am not sure if having sth<T: U> where U is #[must_root] enforces T to also be #[must_root]. But it should be possible to mute the error with allow_unrooted_interior and/or allow(unrooted_must_root).

pub sender: Sender<(Trusted<T>, CommonScriptMsg)>,
pub worker: Trusted<T>,
}

impl<T: JSTraceable + DomObject + 'static> ScriptChan for SendableWorkerScriptChan<T> {
impl<#[must_root] T: JSTraceable + DomObject + 'static> ScriptChan for SendableWorkerScriptChan<T> {
fn send(&self, msg: CommonScriptMsg) -> Result<(), ()> {
self.sender.send((self.worker.clone(), msg)).map_err(|_| ())
}
@@ -35,12 +36,13 @@ impl<T: JSTraceable + DomObject + 'static> ScriptChan for SendableWorkerScriptCh
/// worker event loop messages. While this SendableWorkerScriptChan is alive, the associated
/// Worker object will remain alive.
#[derive(Clone, JSTraceable)]
pub struct WorkerThreadWorkerChan<T: DomObject> {
#[must_root]
pub struct WorkerThreadWorkerChan<#[must_root] T: DomObject> {
pub sender: Sender<(Trusted<T>, WorkerScriptMsg)>,
pub worker: Trusted<T>,
}

impl<T: JSTraceable + DomObject + 'static> ScriptChan for WorkerThreadWorkerChan<T> {
impl<#[must_root] T: JSTraceable + DomObject + 'static> ScriptChan for WorkerThreadWorkerChan<T> {
fn send(&self, msg: CommonScriptMsg) -> Result<(), ()> {
self.sender
.send((self.worker.clone(), WorkerScriptMsg::Common(msg)))
@@ -55,7 +57,7 @@ impl<T: JSTraceable + DomObject + 'static> ScriptChan for WorkerThreadWorkerChan
}
}

impl<T: DomObject> ScriptPort for Receiver<(Trusted<T>, WorkerScriptMsg)> {
impl<#[must_root] T: DomObject> ScriptPort for Receiver<(Trusted<T>, WorkerScriptMsg)> {
fn recv(&self) -> Result<CommonScriptMsg, ()> {
match self.recv().map(|(_, msg)| msg) {
Ok(WorkerScriptMsg::Common(script_msg)) => Ok(script_msg),
@@ -12,9 +12,9 @@ use style::thread_state::{self, ThreadState};
/// This extends the API of `std::cell::RefCell` to allow unsafe access in
/// certain situations, with dynamic checking in debug builds.
#[derive(Clone, Debug, Default, MallocSizeOf, PartialEq)]
#[must_root]
//#[must_root] // TODO should it be #[must_root]? then -> there are usages with #[must_root] types and with normal ones; two different structs for them? or hacking plugin compiler further to mark structure as #[must_root] if it generic type is #[must_root]?

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

This is an interesting question. What we want to represent is that DomRefCell<T> is must_root if and only if T is must_root for each particular instance of T. Is this something that is possible with the current plugin?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

I see the intention. It is what I came up with, but didn't implement so far. After short recalling I am not sure if it was very problematic in some way. It could potentially require some changes to part of the plugin that checks only HIR (check_struct_def method if I remember the name correctly) and maybe some cooperation with the method that analyzes the MIR.

pub struct DomRefCell<#[must_root] T> {
value: RefCell<T>,
value: RefCell<T>, // TODO currently a little bit hacky since RefCell is currently on the exception list
}

// Functionality specific to Servo's `DomRefCell` type
@@ -50,6 +50,7 @@ impl<#[must_root] T> DomRefCell<T> {
// ===================================================
impl<#[must_root] T> DomRefCell<T> {
/// Create a new `DomRefCell` containing `value`.
#[allow(unrooted_must_root)] // TODO don't know if it is okay; T is unrooted argument
pub fn new(value: T) -> DomRefCell<T> {
DomRefCell {
value: RefCell::new(value),
@@ -151,7 +151,7 @@ impl<#[must_root] T: Castable> DomRoot<T> {

impl<#[must_root] T: DomObject> DomRoot<T> {
/// Generate a new root from a reference
pub fn from_ref(unrooted: &T) -> DomRoot<T> {
pub fn from_ref(unrooted: &T) -> DomRoot<T> { // TODO probably should be treated like "new"/"new_*" methods
unsafe { DomRoot::new(Dom::from_ref(unrooted)) }
}
}
@@ -18,18 +18,19 @@ use dom::node::{Node, NodeDamage, window_from_node};
use script_traits::ScriptToConstellationChan;
use textinput::{SelectionDirection, SelectionState, TextInput};

#[must_root]
pub trait TextControlElement: DerivedFrom<EventTarget> + DerivedFrom<Node> {
fn selection_api_applies(&self) -> bool;
fn has_selectable_text(&self) -> bool;
fn set_dirty_value_flag(&self, value: bool);
}

pub struct TextControlSelection<'a, E: TextControlElement> {
pub struct TextControlSelection<'a, #[must_root] E: TextControlElement> {
element: &'a E,
textinput: &'a DomRefCell<TextInput<ScriptToConstellationChan>>,
}

impl<'a, E: TextControlElement> TextControlSelection<'a, E> {
impl<'a, #[must_root] E: TextControlElement> TextControlSelection<'a, E> {
pub fn new(element: &'a E, textinput: &'a DomRefCell<TextInput<ScriptToConstellationChan>>) -> Self {
TextControlSelection { element, textinput }
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.