Skip to content

/ servo

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

Jump to bottom

Disallow uses of `unwrap` in constellation.rs, compositor.rs, and pipeline.rs #10446

Closed
jdm opened this issue Apr 6, 2016 · 9 comments
Closed

Disallow uses of `unwrap` in constellation.rs, compositor.rs, and pipeline.rs #10446

jdm opened this issue Apr 6, 2016 · 9 comments
Labels
A-infrastructure C-assigned

Comments

@jdm
Copy link
Member

@jdm jdm commented Apr 6, 2016

Three options:

  • shell script that greps for unwrap in those files and runs on travis
  • rustc lint that disallows calling any method named unwrap in files marked #[disallow(unwrap)]
  • a warning from highfive if any line in those files contains unwrap
@jdm jdm added the A-infrastructure label Apr 6, 2016
@matthiaskrgr
Copy link
Contributor

@matthiaskrgr matthiaskrgr commented Apr 6, 2016

- sh -c "! grep "\.unwrap(" components/compositing/compositor.rs components/compositing/pipeline.rs components/compositing/constellation.rs"
could be directly written into travis.yml as a job.

If that is sufficient, feel free to assign to me.
@matthiaskrgr
Copy link
Contributor

@matthiaskrgr matthiaskrgr commented Apr 6, 2016

Or do you really want ".*unwrap.*"?
There is also .unwrap_or_else([...]), I was not sure if that should match or not.
@jdm
Copy link
Member Author

@jdm jdm commented Apr 6, 2016

Unwrap_or_else is fine, since it doesn't actually panic like unwrap does. And yes, I would be ok with using shell scripts for this, but we should make a separate script in etc/ci/ and invoke that from .travis.yml as well as places like https://github.com/servo/saltfs/blob/45a0596d2e7470187121b529d49796fd2668e850/buildbot/master/master.cfg#L112 .
@jdm jdm added the C-assigned label Apr 6, 2016
@notriddle
Copy link
Contributor

@notriddle notriddle commented Apr 6, 2016

Could we add it to tidy?
matthiaskrgr added a commit to matthiaskrgr/servo that referenced this issue Apr 7, 2016
@matthiaskrgr
travis: add and run script which checks if listed files contain "unwr…
b7257e1 
…ap". Should fix servo#10446.
bors-servo added a commit that referenced this issue Apr 7, 2016
@bors-servo
Auto merge of #10448 - matthiaskrgr:travis, r=<try>
Loading status checks…
9a1a9e6 
travis: add and run script which checks if listed files contain "unwrap". Should fix #10446.

Unfortunately there is a lot of python code in tidy.py that I don't understand yet, so I simply wrote a bash script to do the job and hooked it up in travis.yml.

Tests are expected to fail:

````
components/compositing/compositor.rs:758:        self.pipeline_details.get_mut(&pipeline_id).unwrap()
components/compositing/compositor.rs:2194:        RgbImage::from_raw(width as u32, height as u32, pixels).unwrap()
components/compositing/pipeline.rs:130:        let (paint_shutdown_chan, paint_shutdown_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:131:        let (layout_shutdown_chan, layout_shutdown_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:132:        let (pipeline_chan, pipeline_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:133:        let (script_to_compositor_chan, script_to_compositor_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:151:            let (script_to_devtools_chan, script_to_devtools_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:154:                let message: ScriptToDevtoolsControlMsg = message.to().unwrap();
components/compositing/pipeline.rs:155:                devtools_chan.send(DevtoolsControlMsg::FromScript(message)).unwrap()
components/compositing/pipeline.rs:161:            ipc::channel().unwrap();
components/compositing/pipeline.rs:174:                    pipeline_port: mem::replace(&mut pipeline_port, None).unwrap(),
components/compositing/pipeline.rs:180:                           .unwrap();
components/compositing/pipeline.rs:184:                let (script_chan, script_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:190:            ipc::channel().unwrap();
components/compositing/pipeline.rs:298:        let (sender, receiver) = ipc::channel().unwrap();
components/compositing/pipeline.rs:300:        receiver.recv().unwrap();
components/compositing/pipeline.rs:315:        let _ = self.script_chan.send(ConstellationControlMsg::Freeze(self.id)).unwrap();
components/compositing/pipeline.rs:319:        let _ = self.script_chan.send(ConstellationControlMsg::Thaw(self.id)).unwrap();
components/compositing/pipeline.rs:323:        let _ = self.script_chan.send(ConstellationControlMsg::ExitPipeline(self.id)).unwrap();
components/compositing/pipeline.rs:326:        let _ = layout_channel.send(LayoutControlMsg::ExitNow).unwrap();
components/compositing/pipeline.rs:343:        let index = self.children.iter().position(|id| *id == frame_id).unwrap();
components/compositing/pipeline.rs:355:        self.script_chan.send(event).unwrap();
components/compositing/pipeline.rs:402:            control_port: mem::replace(&mut self.script_port, None).unwrap(),
components/compositing/pipeline.rs:423:                                  self.pipeline_port.unwrap(),
components/compositing/pipeline.rs:437:            self.script_content_process_shutdown_port.recv().unwrap();
components/compositing/pipeline.rs:438:            self.layout_content_process_shutdown_port.recv().unwrap();
components/compositing/constellation.rs:464:            IpcOneShotServer::<IpcSender<UnprivilegedPipelineContent>>::new().unwrap();
components/compositing/constellation.rs:468:            let mut command = sandbox::Command::me().unwrap();
components/compositing/constellation.rs:474:            let path_to_self = env::current_exe().unwrap();
components/compositing/constellation.rs:478:            ChildProcess::Unsandboxed(child_process.spawn().unwrap())
components/compositing/constellation.rs:482:        let (_receiver, sender) = server.accept().unwrap();
components/compositing/constellation.rs:835:            stderr.write_all("Pipeline failed in hard-fail mode.  Crashing!\n".as_bytes()).unwrap();
components/compositing/constellation.rs:1621:            let (sender, receiver) = ipc::channel().unwrap();
components/compositing/constellation.rs:1657:                        let (sender, receiver) = ipc::channel().unwrap();
components/compositing/constellation.rs:1659:                        layout_chan.send(LayoutControlMsg::GetCurrentEpoch(sender)).unwrap();
components/compositing/constellation.rs:1660:                        let layout_thread_epoch = receiver.recv().unwrap();
components/compositing/constellation.rs:1838:                let (chan, port) = ipc::channel().unwrap();
````

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10448)
<!-- Reviewable:end -->
@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Apr 7, 2016

Yep, this could be given to tidy (it already checks all the lines in Rust source files). It shouldn't add any painful overhead. @jdm Why don't we go for tidy here?
@jdm
Copy link
Member Author

@jdm jdm commented Apr 7, 2016

I guess it seemed so specifically targeted (three files) that it seemed like the wrong tool to reach for.
@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Apr 7, 2016

Yes, but (with the bulk of unwraps we have everywhere) won't the count increase, as we clean the other files in the near future?
@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Apr 7, 2016

Oh, never mind, we'll add it later :)
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Apr 8, 2016

I would be very happy to see this, for stopping regressions to #10124 and friends.
@asajeffrey asajeffrey mentioned this issue Apr 8, 2016
Closed
matthiaskrgr added a commit to matthiaskrgr/servo that referenced this issue Apr 8, 2016
@matthiaskrgr
travis: add and run script which checks if listed files contain "unwr…
1b0c181 
…ap". Should fix servo#10446.
matthiaskrgr added a commit to matthiaskrgr/servo that referenced this issue Apr 8, 2016
@matthiaskrgr
travis: add and run script which checks if listed files contain "unwr…
1015a00 
…ap". Should fix servo#10446.
matthiaskrgr added a commit to matthiaskrgr/servo that referenced this issue Apr 8, 2016
@matthiaskrgr
travis: add and run script which checks if listed files contain "unwr…
8731a73 
…ap". Should fix servo#10446.
bors-servo added a commit that referenced this issue Apr 8, 2016
@bors-servo
Auto merge of #10448 - matthiaskrgr:travis, r=aneeshusa
Loading status checks…
c56eb65 
travis: add and run script which checks if listed files contain "unwrap". Should fix #10446.

Unfortunately there is a lot of python code in tidy.py that I don't understand yet, so I simply wrote a bash script to do the job and hooked it up in travis.yml.

Tests are expected to fail:

````
components/compositing/compositor.rs:758:        self.pipeline_details.get_mut(&pipeline_id).unwrap()
components/compositing/compositor.rs:2194:        RgbImage::from_raw(width as u32, height as u32, pixels).unwrap()
components/compositing/pipeline.rs:130:        let (paint_shutdown_chan, paint_shutdown_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:131:        let (layout_shutdown_chan, layout_shutdown_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:132:        let (pipeline_chan, pipeline_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:133:        let (script_to_compositor_chan, script_to_compositor_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:151:            let (script_to_devtools_chan, script_to_devtools_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:154:                let message: ScriptToDevtoolsControlMsg = message.to().unwrap();
components/compositing/pipeline.rs:155:                devtools_chan.send(DevtoolsControlMsg::FromScript(message)).unwrap()
components/compositing/pipeline.rs:161:            ipc::channel().unwrap();
components/compositing/pipeline.rs:174:                    pipeline_port: mem::replace(&mut pipeline_port, None).unwrap(),
components/compositing/pipeline.rs:180:                           .unwrap();
components/compositing/pipeline.rs:184:                let (script_chan, script_port) = ipc::channel().unwrap();
components/compositing/pipeline.rs:190:            ipc::channel().unwrap();
components/compositing/pipeline.rs:298:        let (sender, receiver) = ipc::channel().unwrap();
components/compositing/pipeline.rs:300:        receiver.recv().unwrap();
components/compositing/pipeline.rs:315:        let _ = self.script_chan.send(ConstellationControlMsg::Freeze(self.id)).unwrap();
components/compositing/pipeline.rs:319:        let _ = self.script_chan.send(ConstellationControlMsg::Thaw(self.id)).unwrap();
components/compositing/pipeline.rs:323:        let _ = self.script_chan.send(ConstellationControlMsg::ExitPipeline(self.id)).unwrap();
components/compositing/pipeline.rs:326:        let _ = layout_channel.send(LayoutControlMsg::ExitNow).unwrap();
components/compositing/pipeline.rs:343:        let index = self.children.iter().position(|id| *id == frame_id).unwrap();
components/compositing/pipeline.rs:355:        self.script_chan.send(event).unwrap();
components/compositing/pipeline.rs:402:            control_port: mem::replace(&mut self.script_port, None).unwrap(),
components/compositing/pipeline.rs:423:                                  self.pipeline_port.unwrap(),
components/compositing/pipeline.rs:437:            self.script_content_process_shutdown_port.recv().unwrap();
components/compositing/pipeline.rs:438:            self.layout_content_process_shutdown_port.recv().unwrap();
components/compositing/constellation.rs:464:            IpcOneShotServer::<IpcSender<UnprivilegedPipelineContent>>::new().unwrap();
components/compositing/constellation.rs:468:            let mut command = sandbox::Command::me().unwrap();
components/compositing/constellation.rs:474:            let path_to_self = env::current_exe().unwrap();
components/compositing/constellation.rs:478:            ChildProcess::Unsandboxed(child_process.spawn().unwrap())
components/compositing/constellation.rs:482:        let (_receiver, sender) = server.accept().unwrap();
components/compositing/constellation.rs:835:            stderr.write_all("Pipeline failed in hard-fail mode.  Crashing!\n".as_bytes()).unwrap();
components/compositing/constellation.rs:1621:            let (sender, receiver) = ipc::channel().unwrap();
components/compositing/constellation.rs:1657:                        let (sender, receiver) = ipc::channel().unwrap();
components/compositing/constellation.rs:1659:                        layout_chan.send(LayoutControlMsg::GetCurrentEpoch(sender)).unwrap();
components/compositing/constellation.rs:1660:                        let layout_thread_epoch = receiver.recv().unwrap();
components/compositing/constellation.rs:1838:                let (chan, port) = ipc::channel().unwrap();
````

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10448)
<!-- Reviewable:end -->
@bors-servo bors-servo closed this in dc4fe7e Apr 8, 2016
@SimonSapin SimonSapin mentioned this issue Dec 18, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-infrastructure C-assigned
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
@jdm @asajeffrey @matthiaskrgr @notriddle @wafflespeanut
You can’t perform that action at this time.