Skip to content

Commit

Permalink
Add a platform-abstraction tidy script
Browse files Browse the repository at this point in the history
This is intended to maintain existing standards of code organization
in hopes that the standard library will continue to be refactored to
isolate platform-specific bits, making porting easier; where "standard
library" roughly means "all the dependencies of the std and test
crates".

This generally means placing restrictions on where `cfg(unix)`,
`cfg(windows)`, `cfg(target_os)` and `cfg(target_env)` may appear,
the basic objective being to isolate platform-specific code to the
platform-specific `std::sys` modules, and to the allocation,
unwinding, and libc crates.

Following are the basic rules, though there are currently
exceptions:

- core may not have platform-specific code
- liballoc_system may have platform-specific code
- liballoc_jemalloc may have platform-specific code
- libpanic_abort may have platform-specific code
- libpanic_unwind may have platform-specific code
- other crates in the std facade may not
- std may have platform-specific code in the following places
  - sys/unix/
  - sys/windows/
  - os/

There are plenty of exceptions today though, noted in the whitelist.
  • Loading branch information
brson committed Oct 2, 2016
1 parent 0fb8379 commit 29e0235
Show file tree
Hide file tree
Showing 6 changed files with 239 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/libstd/net/mod.rs
Expand Up @@ -31,7 +31,7 @@ mod addr;
mod tcp;
mod udp;
mod parser;
#[cfg(all(test, not(target_os = "emscripten")))]
#[cfg(test)]
mod test;

/// Possible values which can be passed to the [`shutdown`] method of
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/net/test.rs
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[allow(dead_code)] // not used on emscripten

use env;
use net::{SocketAddr, SocketAddrV4, SocketAddrV6, Ipv4Addr, Ipv6Addr, ToSocketAddrs};
use sync::atomic::{AtomicUsize, Ordering};
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/sync/mpsc/select.rs
Expand Up @@ -366,8 +366,8 @@ impl<'rx, T:Send+'rx> fmt::Debug for Handle<'rx, T> {
}
}

#[cfg(all(test, not(target_os = "emscripten")))]
#[allow(unused_imports)]
#[cfg(all(test, not(target_os = "emscripten")))]
mod tests {
use thread;
use sync::mpsc::*;
Expand Down
3 changes: 2 additions & 1 deletion src/libstd/sys/common/io.rs
Expand Up @@ -50,7 +50,8 @@ pub unsafe fn read_to_end_uninitialized(r: &mut Read, buf: &mut Vec<u8>) -> io::
}
}

#[cfg(all(test, not(target_os = "emscripten")))]
#[cfg(test)]
#[allow(dead_code)] // not used on emscripten
pub mod test {
use path::{Path, PathBuf};
use env;
Expand Down
2 changes: 2 additions & 0 deletions src/tools/tidy/src/main.rs
Expand Up @@ -36,6 +36,7 @@ mod errors;
mod features;
mod cargo;
mod cargo_lock;
mod pal;

fn main() {
let path = env::args_os().skip(1).next().expect("need an argument");
Expand All @@ -48,6 +49,7 @@ fn main() {
cargo::check(&path, &mut bad);
features::check(&path, &mut bad);
cargo_lock::check(&path, &mut bad);
pal::check(&path, &mut bad);

if bad {
panic!("some tidy checks failed");
Expand Down
231 changes: 231 additions & 0 deletions src/tools/tidy/src/pal.rs
@@ -0,0 +1,231 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Tidy check to enforce rules about platform-specific code in std
//!
//! This is intended to maintain existing standards of code
//! organization in hopes that the standard library will continue to
//! be refactored to isolate platform-specific bits, making porting
//! easier; where "standard library" roughly means "all the
//! dependencies of the std and test crates".
//!
//! This generally means placing restrictions on where `cfg(unix)`,
//! `cfg(windows)`, `cfg(target_os)` and `cfg(target_env)` may appear,
//! the basic objective being to isolate platform-specific code to the
//! platform-specific `std::sys` modules, and to the allocation,
//! unwinding, and libc crates.
//!
//! Following are the basic rules, though there are currently
//! exceptions:
//!
//! - core may not have platform-specific code
//! - liballoc_system may have platform-specific code
//! - liballoc_jemalloc may have platform-specific code
//! - libpanic_abort may have platform-specific code
//! - libpanic_unwind may have platform-specific code
//! - libunwind may have platform-specific code
//! - other crates in the std facade may not
//! - std may have platform-specific code in the following places
//! - sys/unix/
//! - sys/windows/
//! - os/
//!
//! `std/sys_common` should _not_ contain platform-specific code.
//! Finally, because std contains tests with platform-specific
//! `ignore` attributes, once the parser encounters `mod tests`,
//! platform-specific cfgs are allowed. Not sure yet how to deal with
//! this in the long term.

use std::fs::File;
use std::io::Read;
use std::path::Path;
use std::iter::Iterator;

// Paths that may contain platform-specific code
const EXCEPTION_PATHS: &'static [&'static str] = &[
// std crates
"src/liballoc_jemalloc",
"src/liballoc_system",
"src/liblibc",
"src/libpanic_abort",
"src/libpanic_unwind",
"src/libunwind",
"src/libstd/sys/unix", // This is where platform-specific code for std should live
"src/libstd/sys/windows", // Ditto
"src/libstd/os", // Platform-specific public interfaces
"src/rtstartup", // Not sure what to do about this. magic stuff for mingw

// temporary exceptions
"src/libstd/lib.rs", // This could probably be done within the sys directory
"src/libstd/rtdeps.rs", // Until rustbuild replaces make
"src/libstd/path.rs",
"src/libstd/io/stdio.rs",
"src/libstd/num/f32.rs",
"src/libstd/num/f64.rs",
"src/libstd/thread/local.rs",
"src/libstd/sys/common/mod.rs",
"src/libstd/sys/common/args.rs",
"src/libstd/sys/common/net.rs",
"src/libstd/sys/common/util.rs",
"src/libterm", // Not sure how to make this crate portable, but test needs it
"src/libtest", // Probably should defer to unstable std::sys APIs

// std testing crates, ok for now at least
"src/libcoretest",

// non-std crates
"src/test",
"src/tools",
"src/librustc",
"src/librustdoc",
"src/libsyntax",
"src/bootstrap",
];

pub fn check(path: &Path, bad: &mut bool) {
let ref mut contents = String::new();
// Sanity check that the complex parsing here works
let ref mut saw_target_arch = false;
let ref mut saw_cfg_bang = false;
super::walk(path, &mut super::filter_dirs, &mut |file| {
let filestr = file.to_string_lossy().replace("\\", "/");
if !filestr.ends_with(".rs") { return }

let is_exception_path = EXCEPTION_PATHS.iter().any(|s| filestr.contains(&**s));
if is_exception_path { return }

check_cfgs(contents, &file, bad, saw_target_arch, saw_cfg_bang);
});

assert!(*saw_target_arch);
assert!(*saw_cfg_bang);
}

fn check_cfgs(contents: &mut String, file: &Path,
bad: &mut bool, saw_target_arch: &mut bool, saw_cfg_bang: &mut bool) {
contents.truncate(0);
t!(t!(File::open(file), file).read_to_string(contents));

// For now it's ok to have platform-specific code after 'mod tests'.
let mod_tests_idx = find_test_mod(contents);
let contents = &contents[..mod_tests_idx];
// Pull out all "cfg(...)" and "cfg!(...)" strings
let cfgs = parse_cfgs(contents);

let mut line_numbers: Option<Vec<usize>> = None;
let mut err = |idx: usize, cfg: &str| {
if line_numbers.is_none() {
line_numbers = Some(contents.match_indices('\n').map(|(i, _)| i).collect());
}
let line_numbers = line_numbers.as_ref().expect("");
let line = match line_numbers.binary_search(&idx) {
Ok(_) => unreachable!(),
Err(i) => i + 1
};
println!("{}:{}: platform-specific cfg: {}", file.display(), line, cfg);
*bad = true;
};

for (idx, cfg) in cfgs.into_iter() {
// Sanity check that the parsing here works
if !*saw_target_arch && cfg.contains("target_arch") { *saw_target_arch = true }
if !*saw_cfg_bang && cfg.contains("cfg!") { *saw_cfg_bang = true }

let contains_platform_specific_cfg =
cfg.contains("target_os")
|| cfg.contains("target_env")
|| cfg.contains("target_vendor")
|| cfg.contains("unix")
|| cfg.contains("windows");

if !contains_platform_specific_cfg { continue }

let preceeded_by_doc_comment = {
let pre_contents = &contents[..idx];
let pre_newline = pre_contents.rfind('\n');
let pre_doc_comment = pre_contents.rfind("///");
match (pre_newline, pre_doc_comment) {
(Some(n), Some(c)) => n < c,
(None, Some(_)) => true,
(_, None) => false,
}
};

if preceeded_by_doc_comment { continue }

err(idx, cfg);
}
}

fn find_test_mod(contents: &str) -> usize {
if let Some(mod_tests_idx) = contents.find("mod tests") {
// Also capture a previos line indicating "mod tests" in cfg-ed out
let prev_newline_idx = contents[..mod_tests_idx].rfind('\n').unwrap_or(mod_tests_idx);
let prev_newline_idx = contents[..prev_newline_idx].rfind('\n');
if let Some(nl) = prev_newline_idx {
let prev_line = &contents[nl + 1 .. mod_tests_idx];
let emcc_cfg = "cfg(all(test, not(target_os";
if prev_line.contains(emcc_cfg) {
nl
} else {
mod_tests_idx
}
} else {
mod_tests_idx
}
} else {
contents.len()
}
}

fn parse_cfgs<'a>(contents: &'a str) -> Vec<(usize, &'a str)> {
let candidate_cfgs = contents.match_indices("cfg");
let candidate_cfg_idxs = candidate_cfgs.map(|(i, _)| i);
// This is puling out the indexes of all "cfg" strings
// that appear to be tokens succeeded by a paren.
let cfgs = candidate_cfg_idxs.filter(|i| {
let pre_idx = i.saturating_sub(*i);
let succeeds_non_ident = !contents.as_bytes().get(pre_idx)
.cloned()
.map(char::from)
.map(char::is_alphanumeric)
.unwrap_or(false);
let contents_after = &contents[*i..];
let first_paren = contents_after.find('(');
let paren_idx = first_paren.map(|ip| i + ip);
let preceeds_whitespace_and_paren = paren_idx.map(|ip| {
let maybe_space = &contents[*i + "cfg".len() .. ip];
maybe_space.chars().all(|c| char::is_whitespace(c) || c == '!')
}).unwrap_or(false);

succeeds_non_ident && preceeds_whitespace_and_paren
});

cfgs.map(|i| {
let mut depth = 0;
let contents_from = &contents[i..];
for (j, byte) in contents_from.bytes().enumerate() {
match byte {
b'(' => {
depth += 1;
}
b')' => {
depth -= 1;
if depth == 0 {
return (i, &contents_from[.. j + 1]);
}
}
_ => { }
}
}

unreachable!()
}).collect()
}

0 comments on commit 29e0235

Please sign in to comment.