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

Add clippy into CI and fix clippy warnings #1569

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 36 additions & 8 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,27 @@ name: CI
on: [push, pull_request]

env:
RUST_MINVERSION: 1.41.1
RUST_MINVERSION: "1.50"
CARGO_INCREMENTAL: 0
CARGO_NET_RETRY: 10

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.ref || github.run_id }}
cancel-in-progress: true

jobs:
test:
name: Test
runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
rust:
- stable
- beta
- nightly
- 1.48.0
- 1.50.0

features:
- ''
Expand All @@ -41,7 +46,7 @@ jobs:

steps:
- name: Checkout sources
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Install rust (${{ matrix.rust }})
uses: actions-rs/toolchain@v1
Expand Down Expand Up @@ -71,7 +76,7 @@ jobs:

steps:
- name: Checkout sources
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Install rust (${{ env.RUST_MINVERSION }})
uses: actions-rs/toolchain@v1
Expand All @@ -95,7 +100,7 @@ jobs:

steps:
- name: Checkout sources
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Install rust
uses: actions-rs/toolchain@v1
Expand Down Expand Up @@ -125,7 +130,7 @@ jobs:

steps:
- name: Checkout sources
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Install rust
uses: actions-rs/toolchain@v1
Expand All @@ -145,7 +150,7 @@ jobs:

steps:
- name: Checkout sources
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Install rust
uses: actions-rs/toolchain@v1
Expand All @@ -162,13 +167,36 @@ jobs:
command: fmt
args: -- --check

clippy:
name: Check clippy
runs-on: ubuntu-latest

steps:
- name: Checkout sources
uses: actions/checkout@v3

- name: Install rust
uses: actions-rs/toolchain@v1
with:
toolchain: stable
components: clippy
profile: minimal
override: true

- name: cargo clippy -- -D warnings
continue-on-error: true
uses: actions-rs/cargo@v1
with:
command: clippy
args: -- -D warnings

coverage:
name: Coverage
runs-on: ubuntu-latest

steps:
- name: Checkout sources
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Install rust
uses: actions-rs/toolchain@v1
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ edition = "2018"
autoexamples = false

# also update in README.md (badge and "Rust version requirements" section)
rust-version = "1.48"
rust-version = "1.50"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MSRV bump shouldn't be neccesary once #1549 is merged.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you can see there is an error now: https://github.com/Geal/nom/actions/runs/3040066364

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fix error and also add some improvements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and #1549 fixes that by using min() instead of clamp().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, sorry, I see that!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I don't see a problem to drop support for rust version which had a release 2 years ago.

Copy link
Contributor

@cky cky Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikrivosheev It is a huge problem, because it would require a major version bump. Quoting the README (emphasis in original):

The 7.0 series of nom supports Rustc version 1.48 or greater. It is known to work properly on Rust 1.41.1 but there is no guarantee it will stay the case through this major release.

The current policy is that this will only be updated in the next major nom release.

MSRV guarantees are serious business for widely used libraries. It's fine if your library doesn't commit to an MSRV (but that means no library that has an MSRV can use your library as a dependency), but if your library does make such a commitment, you cannot break it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully 8.0 changes the MSRV guarantee to minor compatibility breaks (8.x.0) rather than major (x.0). This is the approach of most libraries I work with and it seems to work out well. Having more MSRV smarts in cargo would he,lp though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSRV will probably change for nom 8 yes, but will stay the same for nom 7. Contrary to some libraries that bump MSRV in minor versions, we can't do that for nom because those versions have to be shipped in software for distributions like debian


include = [
"CHANGELOG.md",
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
[![Build Status](https://github.com/Geal/nom/actions/workflows/ci.yml/badge.svg)](https://github.com/Geal/nom/actions/workflows/ci.yml)
[![Coverage Status](https://coveralls.io/repos/github/Geal/nom/badge.svg?branch=main)](https://coveralls.io/github/Geal/nom?branch=main)
[![Crates.io Version](https://img.shields.io/crates/v/nom.svg)](https://crates.io/crates/nom)
[![Minimum rustc version](https://img.shields.io/badge/rustc-1.48.0+-lightgray.svg)](#rust-version-requirements-msrv)
[![Minimum rustc version](https://img.shields.io/badge/rustc-1.50.0+-lightgray.svg)](#rust-version-requirements-msrv)

nom is a parser combinators library written in Rust. Its goal is to provide tools
to build safe parsers without compromising the speed or memory consumption. To
Expand Down Expand Up @@ -207,7 +207,7 @@ Some benchmarks are available on [Github](https://github.com/Geal/nom_benchmarks

## Rust version requirements (MSRV)

The 7.0 series of nom supports **Rustc version 1.48 or greater**. It is known to work properly on Rust 1.41.1 but there is no guarantee it will stay the case through this major release.
The 7.0 series of nom supports **Rustc version 1.50 or greater**. It is known to work properly on Rust 1.41.1 but there is no guarantee it will stay the case through this major release.

The current policy is that this will only be updated in the next major nom release.

Expand Down
2 changes: 2 additions & 0 deletions src/bytes/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ where
/// assert_eq!(till_colon("12345"), Ok(("", "12345")));
/// assert_eq!(till_colon(""), Ok(("", "")));
/// ```
#[allow(clippy::redundant_closure)]
pub fn take_till<F, Input, Error: ParseError<Input>>(
cond: F,
) -> impl Fn(Input) -> IResult<Input, Input, Error>
Expand Down Expand Up @@ -358,6 +359,7 @@ where
/// assert_eq!(till_colon("12345"), Ok(("", "12345")));
/// assert_eq!(till_colon(""), Err(Err::Error(Error::new("", ErrorKind::TakeTill1))));
/// ```
#[allow(clippy::redundant_closure)]
pub fn take_till1<F, Input, Error: ParseError<Input>>(
cond: F,
) -> impl Fn(Input) -> IResult<Input, Input, Error>
Expand Down
2 changes: 2 additions & 0 deletions src/bytes/streaming.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ where
/// assert_eq!(till_colon("12345"), Err(Err::Incomplete(Needed::new(1))));
/// assert_eq!(till_colon(""), Err(Err::Incomplete(Needed::new(1))));
/// ```
#[allow(clippy::redundant_closure)]
pub fn take_till<F, Input, Error: ParseError<Input>>(
cond: F,
) -> impl Fn(Input) -> IResult<Input, Input, Error>
Expand Down Expand Up @@ -372,6 +373,7 @@ where
/// assert_eq!(till_colon("12345"), Err(Err::Incomplete(Needed::new(1))));
/// assert_eq!(till_colon(""), Err(Err::Incomplete(Needed::new(1))));
/// ```
#[allow(clippy::redundant_closure)]
pub fn take_till1<F, Input, Error: ParseError<Input>>(
cond: F,
) -> impl Fn(Input) -> IResult<Input, Input, Error>
Expand Down
8 changes: 4 additions & 4 deletions src/character/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,23 +414,23 @@ where
/// assert_eq!(parser("c1"), Err(Err::Error(Error::new("c1", ErrorKind::Digit))));
/// assert_eq!(parser(""), Err(Err::Error(Error::new("", ErrorKind::Digit))));
/// ```
///
///
/// ## Parsing an integer
/// You can use `digit1` in combination with [`map_res`] to parse an integer:
///
///
/// ```
/// # use nom::{Err, error::{Error, ErrorKind}, IResult, Needed};
/// # use nom::combinator::map_res;
/// # use nom::character::complete::digit1;
/// fn parser(input: &str) -> IResult<&str, u32> {
/// map_res(digit1, str::parse)(input)
/// }
///
///
/// assert_eq!(parser("416"), Ok(("", 416)));
/// assert_eq!(parser("12b"), Ok(("b", 12)));
/// assert!(parser("b").is_err());
/// ```
///
///
/// [`map_res`]: crate::combinator::map_res
pub fn digit1<T, E: ParseError<T>>(input: T) -> IResult<T, T, E>
where
Expand Down
8 changes: 4 additions & 4 deletions src/character/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub mod streaming;
/// ```
#[inline]
pub fn is_alphabetic(chr: u8) -> bool {
(chr >= 0x41 && chr <= 0x5A) || (chr >= 0x61 && chr <= 0x7A)
(0x41..=0x5A).contains(&chr) || (0x61..=0x7A).contains(&chr)
}

/// Tests if byte is ASCII digit: 0-9
Expand All @@ -33,7 +33,7 @@ pub fn is_alphabetic(chr: u8) -> bool {
/// ```
#[inline]
pub fn is_digit(chr: u8) -> bool {
chr >= 0x30 && chr <= 0x39
(0x30..=0x39).contains(&chr)
}

/// Tests if byte is ASCII hex digit: 0-9, A-F, a-f
Expand All @@ -49,7 +49,7 @@ pub fn is_digit(chr: u8) -> bool {
/// ```
#[inline]
pub fn is_hex_digit(chr: u8) -> bool {
(chr >= 0x30 && chr <= 0x39) || (chr >= 0x41 && chr <= 0x46) || (chr >= 0x61 && chr <= 0x66)
(0x30..=0x39).contains(&chr) || (0x41..=0x46).contains(&chr) || (0x61..=0x66).contains(&chr)
}

/// Tests if byte is ASCII octal digit: 0-7
Expand All @@ -64,7 +64,7 @@ pub fn is_hex_digit(chr: u8) -> bool {
/// ```
#[inline]
pub fn is_oct_digit(chr: u8) -> bool {
chr >= 0x30 && chr <= 0x37
(0x30..=0x37).contains(&chr)
}

/// Tests if byte is ASCII alphanumeric: A-Z, a-z, 0-9
Expand Down
2 changes: 2 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ pub trait FromExternalError<I, E> {
}

/// default error type, only contains the error' location and code
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Debug, PartialEq)]
pub struct Error<I> {
/// position of the error in the input data
Expand Down Expand Up @@ -156,6 +157,7 @@ pub struct VerboseError<I> {

#[cfg(feature = "alloc")]
#[cfg_attr(feature = "docsrs", doc(cfg(feature = "alloc")))]
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, Debug, PartialEq)]
/// Error context for `VerboseError`
pub enum VerboseErrorKind {
Expand Down
31 changes: 9 additions & 22 deletions src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,7 @@ pub enum Err<E> {
impl<E> Err<E> {
/// Tests if the result is Incomplete
pub fn is_incomplete(&self) -> bool {
if let Err::Incomplete(_) = self {
true
} else {
false
}
matches!(self, Err::Incomplete(_))
}

/// Applies the given function to the inner error
Expand Down Expand Up @@ -344,7 +340,7 @@ pub struct Map<F, G, O1> {
phantom: core::marker::PhantomData<O1>,
}

impl<'a, I, O1, O2, E, F: Parser<I, O1, E>, G: Fn(O1) -> O2> Parser<I, O2, E> for Map<F, G, O1> {
impl<I, O1, O2, E, F: Parser<I, O1, E>, G: Fn(O1) -> O2> Parser<I, O2, E> for Map<F, G, O1> {
fn parse(&mut self, i: I) -> IResult<I, O2, E> {
match self.f.parse(i) {
Err(e) => Err(e),
Expand All @@ -361,7 +357,7 @@ pub struct FlatMap<F, G, O1> {
phantom: core::marker::PhantomData<O1>,
}

impl<'a, I, O1, O2, E, F: Parser<I, O1, E>, G: Fn(O1) -> H, H: Parser<I, O2, E>> Parser<I, O2, E>
impl<I, O1, O2, E, F: Parser<I, O1, E>, G: Fn(O1) -> H, H: Parser<I, O2, E>> Parser<I, O2, E>
for FlatMap<F, G, O1>
{
fn parse(&mut self, i: I) -> IResult<I, O2, E> {
Expand All @@ -378,7 +374,7 @@ pub struct AndThen<F, G, O1> {
phantom: core::marker::PhantomData<O1>,
}

impl<'a, I, O1, O2, E, F: Parser<I, O1, E>, G: Parser<O1, O2, E>> Parser<I, O2, E>
impl<I, O1, O2, E, F: Parser<I, O1, E>, G: Parser<O1, O2, E>> Parser<I, O2, E>
for AndThen<F, G, O1>
{
fn parse(&mut self, i: I) -> IResult<I, O2, E> {
Expand All @@ -395,9 +391,7 @@ pub struct And<F, G> {
g: G,
}

impl<'a, I, O1, O2, E, F: Parser<I, O1, E>, G: Parser<I, O2, E>> Parser<I, (O1, O2), E>
for And<F, G>
{
impl<I, O1, O2, E, F: Parser<I, O1, E>, G: Parser<I, O2, E>> Parser<I, (O1, O2), E> for And<F, G> {
fn parse(&mut self, i: I) -> IResult<I, (O1, O2), E> {
let (i, o1) = self.f.parse(i)?;
let (i, o2) = self.g.parse(i)?;
Expand All @@ -412,8 +406,8 @@ pub struct Or<F, G> {
g: G,
}

impl<'a, I: Clone, O, E: crate::error::ParseError<I>, F: Parser<I, O, E>, G: Parser<I, O, E>>
Parser<I, O, E> for Or<F, G>
impl<I: Clone, O, E: error::ParseError<I>, F: Parser<I, O, E>, G: Parser<I, O, E>> Parser<I, O, E>
for Or<F, G>
{
fn parse(&mut self, i: I) -> IResult<I, O, E> {
match self.f.parse(i.clone()) {
Expand All @@ -436,15 +430,8 @@ pub struct Into<F, O1, O2: From<O1>, E1, E2: From<E1>> {
phantom_err2: core::marker::PhantomData<E2>,
}

impl<
'a,
I: Clone,
O1,
O2: From<O1>,
E1,
E2: crate::error::ParseError<I> + From<E1>,
F: Parser<I, O1, E1>,
> Parser<I, O2, E2> for Into<F, O1, O2, E1, E2>
impl<I: Clone, O1, O2: From<O1>, E1, E2: error::ParseError<I> + From<E1>, F: Parser<I, O1, E1>>
Parser<I, O2, E2> for Into<F, O1, O2, E1, E2>
{
fn parse(&mut self, i: I) -> IResult<I, O2, E2> {
match self.f.parse(i) {
Expand Down
2 changes: 1 addition & 1 deletion src/number/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1515,7 +1515,7 @@ where
}
}

let position = position.unwrap_or(i.input_len());
let position = position.unwrap_or_else(|| i.input_len());

let index = if zero_count == 0 {
position
Expand Down
17 changes: 13 additions & 4 deletions src/sequence/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::*;
use crate::bytes::streaming::{tag, take};
use crate::error::{ErrorKind, Error};
use crate::error::{Error, ErrorKind};
use crate::internal::{Err, IResult, Needed};
use crate::number::streaming::be_u16;

Expand Down Expand Up @@ -275,7 +275,16 @@ fn tuple_test() {

#[test]
fn unit_type() {
assert_eq!(tuple::<&'static str, (), Error<&'static str>, ()>(())("abxsbsh"), Ok(("abxsbsh", ())));
assert_eq!(tuple::<&'static str, (), Error<&'static str>, ()>(())("sdfjakdsas"), Ok(("sdfjakdsas", ())));
assert_eq!(tuple::<&'static str, (), Error<&'static str>, ()>(())(""), Ok(("", ())));
assert_eq!(
tuple::<&'static str, (), Error<&'static str>, ()>(())("abxsbsh"),
Ok(("abxsbsh", ()))
);
assert_eq!(
tuple::<&'static str, (), Error<&'static str>, ()>(())("sdfjakdsas"),
Ok(("sdfjakdsas", ()))
);
assert_eq!(
tuple::<&'static str, (), Error<&'static str>, ()>(())(""),
Ok(("", ()))
);
}
Loading