Patchwork D6597: rust-2018: switch hg-core and hg-cpython to rust 2018 edition

login
register
mail settings
Submitter phabricator
Date July 4, 2019, 3 p.m.
Message ID <differential-rev-PHID-DREV-4zypi6vajgtez2yblcws-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40767/
State New
Headers show

Comments

phabricator - July 4, 2019, 3 p.m.
Alphare created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.
pulkit added a comment.


  Can you add link of mailing list thread in your commit message? Link from markmail or https://www.mercurial-scm.org/pipermail/mercurial-devel/.

REVISION SUMMARY
  Many interesting changes have happened in Rust since the Oxidation Plan was
  introduced, like the 2018 edition and procedural macros:
  
  - Opting in to the 2018 edition is a clear benefit in terms of future proofing, new (nice to have) syntactical sugar notwithstanding. It also has a new non-lexical, non-AST based borrow checker that has fewer bugs(!) and allows us to write correct code that in some cases would have been rejected by the old one.
  - Procedural macros allow us to use the PyO3 crate which maintainers have expressed the clear goal of compiling on stable, which would help in code maintainability compared to rust-cpython.
  
  In this patch are the following changes:
  
  - Removing most `extern crate` uses
  - Updating `use` clauses (`crate` keyword, nested `use`)
  - Removing `mod.rs` in favor of an aptly named module file
  
  Like discussed in the mailing list, until Rust integration in Mercurial is
  considered to be out of the experimental phase, the maximum version of Rust
  allowed is whatever the latest version Debian packages.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6597

AFFECTED FILES
  rust/hg-core/Cargo.toml
  rust/hg-core/src/dirstate.rs
  rust/hg-core/src/dirstate/dirs_multiset.rs
  rust/hg-core/src/dirstate/mod.rs
  rust/hg-core/src/dirstate/parsers.rs
  rust/hg-core/src/filepatterns.rs
  rust/hg-core/src/lib.rs
  rust/hg-core/src/utils.rs
  rust/hg-core/src/utils/mod.rs
  rust/hg-core/tests/test_missing_ancestors.rs
  rust/hg-cpython/Cargo.toml
  rust/hg-cpython/src/ancestors.rs
  rust/hg-cpython/src/cindex.rs
  rust/hg-cpython/src/dagops.rs
  rust/hg-cpython/src/dirstate.rs
  rust/hg-cpython/src/discovery.rs
  rust/hg-cpython/src/exceptions.rs
  rust/hg-cpython/src/filepatterns.rs
  rust/hg-cpython/src/lib.rs

CHANGE DETAILS




To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - July 10, 2019, 2:53 p.m.
kevincox added a comment.
kevincox accepted this revision.


  Is the SliceExt change related to the 2018 change? If not could you split the two?

INLINE COMMENTS

> utils.rs:3
> +
> +pub fn replace_slice<T>(buf: &mut [T], from: &[T], to: &[T])
> +where

This could really use a doc comment.

> utils.rs:7
> +{
> +    if buf.len() < from.len() || from.len() != to.len() {
> +        return;

from.len() != to.len() sounds like a bug and I would probably use an `assert!()`. Unless this is expected to be common and "okay".

> utils.rs:12
> +        if buf[i..].starts_with(from) {
> +            buf[i..(i + from.len())].clone_from_slice(to);
> +        }

This allows overlapping replacements. I don't know if this is intended.

> utils.rs:38
> +    }
> +    fn trim_end(&self) -> &[u8] {
> +        if let Some(last) = self.iter().rposition(is_not_whitespace) {

Why not define `trim` as `self.trim_start().trim_end()`?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6597/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6597

To: Alphare, #hg-reviewers, kevincox
Cc: pulkit, durin42, kevincox, mercurial-devel
phabricator - July 10, 2019, 3:02 p.m.
Alphare added a comment.


  In D6597#96881 <https://phab.mercurial-scm.org/D6597#96881>, @kevincox wrote:
  
  > Is the SliceExt change related to the 2018 change? If not could you split the two?
  
  It is indeed related to 2018: `mod.rs` files are not needed anymore and this makes it both simpler in terms of file structure, but also for development, as having 5 `mod.rs` files open is not fun.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6597/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6597

To: Alphare, #hg-reviewers, kevincox
Cc: pulkit, durin42, kevincox, mercurial-devel
phabricator - July 10, 2019, 3:02 p.m.
Alphare added a comment.


  In D6597#96317 <https://phab.mercurial-scm.org/D6597#96317>, @pulkit wrote:
  
  > Can you add link of mailing list thread in your commit message? Link from markmail or https://www.mercurial-scm.org/pipermail/mercurial-devel/.
  
  Sure, will amend.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6597/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6597

To: Alphare, #hg-reviewers, kevincox
Cc: pulkit, durin42, kevincox, mercurial-devel
phabricator - July 10, 2019, 3:06 p.m.
Alphare added inline comments.

INLINE COMMENTS

> kevincox wrote in utils.rs:3
> This could really use a doc comment.

I'll address those points in a separate (parent) changeset.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6597/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6597

To: Alphare, #hg-reviewers, kevincox
Cc: pulkit, durin42, kevincox, mercurial-devel
phabricator - July 15, 2019, 6:53 p.m.
durin42 added a comment.


  LGTM, but needs rebased (sorry for slow response)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6597/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6597

To: Alphare, #hg-reviewers, kevincox
Cc: pulkit, durin42, kevincox, mercurial-devel

Patch

diff --git a/rust/hg-cpython/src/lib.rs b/rust/hg-cpython/src/lib.rs
--- a/rust/hg-cpython/src/lib.rs
+++ b/rust/hg-cpython/src/lib.rs
@@ -19,10 +19,10 @@ 
 //! 'Generic DAG ancestor algorithms - Rust implementation'
 //! ```
 
+/// This crate uses nested private macros, `extern crate` is still needed in
+/// 2018 edition.
 #[macro_use]
 extern crate cpython;
-extern crate hg;
-extern crate libc;
 
 pub mod ancestors;
 mod cindex;
diff --git a/rust/hg-cpython/src/filepatterns.rs b/rust/hg-cpython/src/filepatterns.rs
--- a/rust/hg-cpython/src/filepatterns.rs
+++ b/rust/hg-cpython/src/filepatterns.rs
@@ -10,10 +10,10 @@ 
 //! `hg-core` crate. From Python, this will be seen as `rustext.filepatterns`
 //! and can be used as replacement for the the pure `filepatterns` Python module.
 //!
+use crate::exceptions::{PatternError, PatternFileError};
 use cpython::{
     PyBytes, PyDict, PyModule, PyObject, PyResult, PyTuple, Python, ToPyObject,
 };
-use exceptions::{PatternError, PatternFileError};
 use hg::{build_single_regex, read_pattern_file, LineNumber, PatternTuple};
 
 /// Rust does not like functions with different return signatures.
diff --git a/rust/hg-cpython/src/exceptions.rs b/rust/hg-cpython/src/exceptions.rs
--- a/rust/hg-cpython/src/exceptions.rs
+++ b/rust/hg-cpython/src/exceptions.rs
@@ -12,8 +12,10 @@ 
 //! existing Python exceptions if appropriate.
 //!
 //! [`GraphError`]: struct.GraphError.html
-use cpython::exc::{RuntimeError, ValueError};
-use cpython::{exc, PyErr, Python};
+use cpython::{
+    exc::{IOError, RuntimeError, ValueError},
+    py_exception, PyErr, Python,
+};
 use hg;
 
 py_exception!(rustext, GraphError, ValueError);
@@ -55,7 +57,7 @@ 
         match inner {
             hg::PatternFileError::IO(e) => {
                 let value = (e.raw_os_error().unwrap_or(2), e.to_string());
-                PyErr::new::<exc::IOError, _>(py, value)
+                PyErr::new::<IOError, _>(py, value)
             }
             hg::PatternFileError::Pattern(e, l) => match e {
                 hg::PatternError::UnsupportedSyntax(m) => {
diff --git a/rust/hg-cpython/src/discovery.rs b/rust/hg-cpython/src/discovery.rs
--- a/rust/hg-cpython/src/discovery.rs
+++ b/rust/hg-cpython/src/discovery.rs
@@ -12,13 +12,15 @@ 
 //! - [`PartialDiscover`] is the Rust implementation of
 //!   `mercurial.setdiscovery.partialdiscovery`.
 
-use crate::conversion::{py_set, rev_pyiter_collect};
-use cindex::Index;
+use crate::{
+    cindex::Index,
+    conversion::{py_set, rev_pyiter_collect},
+    exceptions::GraphError,
+};
 use cpython::{
     ObjectProtocol, PyDict, PyModule, PyObject, PyResult, Python,
     PythonObject, ToPyObject,
 };
-use exceptions::GraphError;
 use hg::discovery::PartialDiscovery as CorePartialDiscovery;
 use hg::Revision;
 
diff --git a/rust/hg-cpython/src/dirstate.rs b/rust/hg-cpython/src/dirstate.rs
--- a/rust/hg-cpython/src/dirstate.rs
+++ b/rust/hg-cpython/src/dirstate.rs
@@ -19,17 +19,14 @@ 
     DirstateEntry, DirstateMapError, DirstatePackError, DirstateParents,
     DirstateParseError, DirstateVec,
 };
+use libc::{c_char, c_int};
+#[cfg(feature = "python27")]
+use python27_sys::PyCapsule_Import;
+#[cfg(feature = "python3")]
+use python3_sys::PyCapsule_Import;
+use std::cell::RefCell;
 use std::collections::HashMap;
 use std::ffi::CStr;
-
-#[cfg(feature = "python27")]
-extern crate python27_sys as python_sys;
-#[cfg(feature = "python3")]
-extern crate python3_sys as python_sys;
-
-use self::python_sys::PyCapsule_Import;
-use libc::{c_char, c_int};
-use std::cell::RefCell;
 use std::mem::transmute;
 
 /// C code uses a custom `dirstate_tuple` type, checks in multiple instances
diff --git a/rust/hg-cpython/src/dagops.rs b/rust/hg-cpython/src/dagops.rs
--- a/rust/hg-cpython/src/dagops.rs
+++ b/rust/hg-cpython/src/dagops.rs
@@ -9,10 +9,12 @@ 
 //! `hg-core` package.
 //!
 //! From Python, this will be seen as `mercurial.rustext.dagop`
-use crate::conversion::{py_set, rev_pyiter_collect};
-use cindex::Index;
+use crate::{
+    cindex::Index,
+    conversion::{py_set, rev_pyiter_collect},
+    exceptions::GraphError,
+};
 use cpython::{PyDict, PyModule, PyObject, PyResult, Python};
-use exceptions::GraphError;
 use hg::dagops;
 use hg::Revision;
 use std::collections::HashSet;
diff --git a/rust/hg-cpython/src/cindex.rs b/rust/hg-cpython/src/cindex.rs
--- a/rust/hg-cpython/src/cindex.rs
+++ b/rust/hg-cpython/src/cindex.rs
@@ -10,14 +10,14 @@ 
 //! Ideally, we should use an Index entirely implemented in Rust,
 //! but this will take some time to get there.
 #[cfg(feature = "python27")]
-extern crate python27_sys as python_sys;
+use python27_sys as python_sys;
 #[cfg(feature = "python3")]
-extern crate python3_sys as python_sys;
+use python3_sys as python_sys;
 
-use self::python_sys::PyCapsule_Import;
 use cpython::{PyClone, PyErr, PyObject, PyResult, Python};
 use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION};
 use libc::c_int;
+use python_sys::PyCapsule_Import;
 use std::ffi::CStr;
 use std::mem::transmute;
 
diff --git a/rust/hg-cpython/src/ancestors.rs b/rust/hg-cpython/src/ancestors.rs
--- a/rust/hg-cpython/src/ancestors.rs
+++ b/rust/hg-cpython/src/ancestors.rs
@@ -34,13 +34,15 @@ 
 //! [`LazyAncestors`]: struct.LazyAncestors.html
 //! [`MissingAncestors`]: struct.MissingAncestors.html
 //! [`AncestorsIterator`]: struct.AncestorsIterator.html
-use crate::conversion::{py_set, rev_pyiter_collect};
-use cindex::Index;
+use crate::{
+    cindex::Index,
+    conversion::{py_set, rev_pyiter_collect},
+    exceptions::GraphError,
+};
 use cpython::{
     ObjectProtocol, PyClone, PyDict, PyList, PyModule, PyObject, PyResult,
     Python, PythonObject, ToPyObject,
 };
-use exceptions::GraphError;
 use hg::Revision;
 use hg::{
     AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy,
diff --git a/rust/hg-cpython/Cargo.toml b/rust/hg-cpython/Cargo.toml
--- a/rust/hg-cpython/Cargo.toml
+++ b/rust/hg-cpython/Cargo.toml
@@ -2,6 +2,7 @@ 
 name = "hg-cpython"
 version = "0.1.0"
 authors = ["Georges Racinet <gracinet@anybox.fr>"]
+edition = "2018"
 
 [lib]
 name='rusthg'
diff --git a/rust/hg-core/tests/test_missing_ancestors.rs b/rust/hg-core/tests/test_missing_ancestors.rs
--- a/rust/hg-core/tests/test_missing_ancestors.rs
+++ b/rust/hg-core/tests/test_missing_ancestors.rs
@@ -1,7 +1,3 @@ 
-extern crate hg;
-extern crate rand;
-extern crate rand_pcg;
-
 use hg::testing::VecGraph;
 use hg::Revision;
 use hg::*;
diff --git a/rust/hg-core/src/utils/mod.rs b/rust/hg-core/src/utils/mod.rs
deleted file mode 100644
--- a/rust/hg-core/src/utils/mod.rs
+++ /dev/null
@@ -1,45 +0,0 @@ 
-pub mod files;
-
-pub fn replace_slice<T>(buf: &mut [T], from: &[T], to: &[T])
-where
-    T: Clone + PartialEq,
-{
-    if buf.len() < from.len() || from.len() != to.len() {
-        return;
-    }
-    for i in 0..=buf.len() - from.len() {
-        if buf[i..].starts_with(from) {
-            buf[i..(i + from.len())].clone_from_slice(to);
-        }
-    }
-}
-
-pub trait SliceExt {
-    fn trim(&self) -> &Self;
-    fn trim_end(&self) -> &Self;
-}
-
-fn is_not_whitespace(c: &u8) -> bool {
-    !(*c as char).is_whitespace()
-}
-
-impl SliceExt for [u8] {
-    fn trim(&self) -> &[u8] {
-        if let Some(first) = self.iter().position(is_not_whitespace) {
-            if let Some(last) = self.iter().rposition(is_not_whitespace) {
-                &self[first..last + 1]
-            } else {
-                unreachable!();
-            }
-        } else {
-            &[]
-        }
-    }
-    fn trim_end(&self) -> &[u8] {
-        if let Some(last) = self.iter().rposition(is_not_whitespace) {
-            &self[..last + 1]
-        } else {
-            &[]
-        }
-    }
-}
diff --git a/rust/hg-core/src/utils.rs b/rust/hg-core/src/utils.rs
new file mode 100644
--- /dev/null
+++ b/rust/hg-core/src/utils.rs
@@ -0,0 +1,45 @@ 
+pub mod files;
+
+pub fn replace_slice<T>(buf: &mut [T], from: &[T], to: &[T])
+where
+    T: Clone + PartialEq,
+{
+    if buf.len() < from.len() || from.len() != to.len() {
+        return;
+    }
+    for i in 0..=buf.len() - from.len() {
+        if buf[i..].starts_with(from) {
+            buf[i..(i + from.len())].clone_from_slice(to);
+        }
+    }
+}
+
+pub trait SliceExt {
+    fn trim(&self) -> &Self;
+    fn trim_end(&self) -> &Self;
+}
+
+fn is_not_whitespace(c: &u8) -> bool {
+    !(*c as char).is_whitespace()
+}
+
+impl SliceExt for [u8] {
+    fn trim(&self) -> &[u8] {
+        if let Some(first) = self.iter().position(is_not_whitespace) {
+            if let Some(last) = self.iter().rposition(is_not_whitespace) {
+                &self[first..last + 1]
+            } else {
+                unreachable!();
+            }
+        } else {
+            &[]
+        }
+    }
+    fn trim_end(&self) -> &[u8] {
+        if let Some(last) = self.iter().rposition(is_not_whitespace) {
+            &self[..last + 1]
+        } else {
+            &[]
+        }
+    }
+}
diff --git a/rust/hg-core/src/lib.rs b/rust/hg-core/src/lib.rs
--- a/rust/hg-core/src/lib.rs
+++ b/rust/hg-core/src/lib.rs
@@ -2,12 +2,6 @@ 
 //
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
-extern crate byteorder;
-extern crate memchr;
-#[macro_use]
-extern crate lazy_static;
-extern crate regex;
-
 mod ancestors;
 pub mod dagops;
 pub use ancestors::{AncestorsIterator, LazyAncestors, MissingAncestors};
@@ -50,7 +44,7 @@ 
     /// Return the two parents of the given `Revision`.
     ///
     /// Each of the parents can be independently `NULL_REVISION`
-    fn parents(&self, Revision) -> Result<[Revision; 2], GraphError>;
+    fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError>;
 }
 
 pub type LineNumber = usize;
diff --git a/rust/hg-core/src/filepatterns.rs b/rust/hg-core/src/filepatterns.rs
--- a/rust/hg-core/src/filepatterns.rs
+++ b/rust/hg-core/src/filepatterns.rs
@@ -1,11 +1,13 @@ 
-use crate::{LineNumber, PatternError, PatternFileError};
+use crate::{
+    utils::{files::get_path_from_bytes, replace_slice, SliceExt},
+    LineNumber, PatternError, PatternFileError,
+};
+use lazy_static::lazy_static;
 use regex::bytes::Regex;
 use std::collections::HashMap;
 use std::fs::File;
 use std::io::Read;
 use std::vec::Vec;
-use utils::files::get_path_from_bytes;
-use utils::{replace_slice, SliceExt};
 
 lazy_static! {
     static ref RE_ESCAPE: Vec<Vec<u8>> = {
diff --git a/rust/hg-core/src/dirstate/parsers.rs b/rust/hg-core/src/dirstate/parsers.rs
--- a/rust/hg-core/src/dirstate/parsers.rs
+++ b/rust/hg-core/src/dirstate/parsers.rs
@@ -3,13 +3,13 @@ 
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
 
+use crate::{
+    CopyVec, CopyVecEntry, DirstateEntry, DirstatePackError, DirstateParents,
+    DirstateParseError, DirstateVec,
+};
 use byteorder::{BigEndian, ReadBytesExt, WriteBytesExt};
 use std::collections::HashMap;
 use std::io::Cursor;
-use {
-    CopyVec, CopyVecEntry, DirstateEntry, DirstatePackError, DirstateParents,
-    DirstateParseError, DirstateVec,
-};
 
 /// Parents are stored in the dirstate as byte hashes.
 const PARENT_SIZE: usize = 20;
diff --git a/rust/hg-core/src/dirstate/mod.rs b/rust/hg-core/src/dirstate/mod.rs
deleted file mode 100644
--- a/rust/hg-core/src/dirstate/mod.rs
+++ /dev/null
@@ -1,36 +0,0 @@ 
-pub mod dirs_multiset;
-pub mod parsers;
-
-#[derive(Debug, PartialEq, Copy, Clone)]
-pub struct DirstateParents<'a> {
-    pub p1: &'a [u8],
-    pub p2: &'a [u8],
-}
-
-/// The C implementation uses all signed types. This will be an issue
-/// either when 4GB+ source files are commonplace or in 2038, whichever
-/// comes first.
-#[derive(Debug, PartialEq)]
-pub struct DirstateEntry {
-    pub state: i8,
-    pub mode: i32,
-    pub mtime: i32,
-    pub size: i32,
-}
-
-pub type DirstateVec = Vec<(Vec<u8>, DirstateEntry)>;
-
-#[derive(Debug, PartialEq)]
-pub struct CopyVecEntry<'a> {
-    pub path: &'a [u8],
-    pub copy_path: &'a [u8],
-}
-
-pub type CopyVec<'a> = Vec<CopyVecEntry<'a>>;
-
-/// The Python implementation passes either a mapping (dirstate) or a flat
-/// iterable (manifest)
-pub enum DirsIterable {
-    Dirstate(DirstateVec),
-    Manifest(Vec<Vec<u8>>),
-}
diff --git a/rust/hg-core/src/dirstate/dirs_multiset.rs b/rust/hg-core/src/dirstate/dirs_multiset.rs
--- a/rust/hg-core/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs
@@ -8,9 +8,9 @@ 
 //! A multiset of directory names.
 //!
 //! Used to counts the references to directories in a manifest or dirstate.
+use crate::{DirsIterable, DirstateEntry, DirstateMapError};
 use std::collections::hash_map::{Entry, Iter};
 use std::collections::HashMap;
-use {DirsIterable, DirstateEntry, DirstateMapError};
 
 #[derive(PartialEq, Debug)]
 pub struct DirsMultiset {
diff --git a/rust/hg-core/src/dirstate.rs b/rust/hg-core/src/dirstate.rs
new file mode 100644
--- /dev/null
+++ b/rust/hg-core/src/dirstate.rs
@@ -0,0 +1,36 @@ 
+pub mod dirs_multiset;
+pub mod parsers;
+
+#[derive(Debug, PartialEq, Copy, Clone)]
+pub struct DirstateParents<'a> {
+    pub p1: &'a [u8],
+    pub p2: &'a [u8],
+}
+
+/// The C implementation uses all signed types. This will be an issue
+/// either when 4GB+ source files are commonplace or in 2038, whichever
+/// comes first.
+#[derive(Debug, PartialEq)]
+pub struct DirstateEntry {
+    pub state: i8,
+    pub mode: i32,
+    pub mtime: i32,
+    pub size: i32,
+}
+
+pub type DirstateVec = Vec<(Vec<u8>, DirstateEntry)>;
+
+#[derive(Debug, PartialEq)]
+pub struct CopyVecEntry<'a> {
+    pub path: &'a [u8],
+    pub copy_path: &'a [u8],
+}
+
+pub type CopyVec<'a> = Vec<CopyVecEntry<'a>>;
+
+/// The Python implementation passes either a mapping (dirstate) or a flat
+/// iterable (manifest)
+pub enum DirsIterable {
+    Dirstate(DirstateVec),
+    Manifest(Vec<Vec<u8>>),
+}
diff --git a/rust/hg-core/Cargo.toml b/rust/hg-core/Cargo.toml
--- a/rust/hg-core/Cargo.toml
+++ b/rust/hg-core/Cargo.toml
@@ -3,6 +3,7 @@ 
 version = "0.1.0"
 authors = ["Georges Racinet <gracinet@anybox.fr>"]
 description = "Mercurial pure Rust core library, with no assumption on Python bindings (FFI)"
+edition = "2018"
 
 [lib]
 name = "hg"