Skip to content

Enable clippy in CI #1114

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 7, 2019
Merged

Enable clippy in CI #1114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
347d481
fix remaining clippy warnings
mkurnikov Jul 7, 2019
de9a917
add Clippy to CI
mkurnikov Jul 7, 2019
67bbb5d
use unique JOBCACHE value for clippy in travis config
mkurnikov Jul 7, 2019
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
11 changes: 11 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ matrix:
env:
- JOBCACHE=3

- name: Lint code with clippy
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to combine the rustfmt check with this check to reduce the amount of different jobs? We could add cargo clippy --all to the rustfmt job above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to make them appear in a different rows in the Travis output? I'd like to spare user from looking inside the Travis row to see what's going on. He sees that formatter failed, goes to his own terminal, runs it, creates new commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that sounds fair to me.

language: rust
rust: stable
cache: cargo
before_script:
- rustup component add clippy
script:
- cargo clippy --all -- -Dwarnings
env:
- JOBCACHE=8

- name: publish documentation
language: rust
rust: stable
Expand Down
22 changes: 10 additions & 12 deletions compiler/src/symboltable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,19 +301,17 @@ impl SymbolTableBuilder {
if let Some(alias) = &part.alias {
// `import mymodule as myalias`
self.register_name(alias, SymbolRole::Assigned)?;
} else if part.symbols.is_empty() {
// `import module`
self.register_name(&part.module, SymbolRole::Assigned)?;
} else {
if part.symbols.is_empty() {
// `import module`
self.register_name(&part.module, SymbolRole::Assigned)?;
} else {
// `from mymodule import myimport`
for symbol in &part.symbols {
if let Some(alias) = &symbol.alias {
// `from mymodule import myimportname as myalias`
self.register_name(alias, SymbolRole::Assigned)?;
} else {
self.register_name(&symbol.symbol, SymbolRole::Assigned)?;
}
// `from mymodule import myimport`
for symbol in &part.symbols {
if let Some(alias) = &symbol.alias {
// `from mymodule import myimportname as myalias`
self.register_name(alias, SymbolRole::Assigned)?;
} else {
self.register_name(&symbol.symbol, SymbolRole::Assigned)?;
}
}
}
Expand Down
79 changes: 38 additions & 41 deletions derive/src/compile_bytecode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl PyCompileInput {
fn assert_source_empty(source: &Option<CompilationSource>) -> Result<(), Diagnostic> {
if let Some(source) = source {
Err(Diagnostic::spans_error(
source.span.clone(),
source.span,
"Cannot have more than one source",
))
} else {
Expand All @@ -86,53 +86,50 @@ impl PyCompileInput {
}

for meta in &self.metas {
match meta {
Meta::NameValue(name_value) => {
if name_value.ident == "mode" {
mode = Some(match &name_value.lit {
Lit::Str(s) => match s.value().as_str() {
"exec" => compile::Mode::Exec,
"eval" => compile::Mode::Eval,
"single" => compile::Mode::Single,
_ => bail_span!(s, "mode must be exec, eval, or single"),
},
_ => bail_span!(name_value.lit, "mode must be a string"),
})
} else if name_value.ident == "module_name" {
module_name = Some(match &name_value.lit {
Lit::Str(s) => s.value(),
_ => bail_span!(name_value.lit, "module_name must be string"),
})
} else if name_value.ident == "source" {
assert_source_empty(&source)?;
let code = match &name_value.lit {
Lit::Str(s) => s.value(),
_ => bail_span!(name_value.lit, "source must be a string"),
};
source = Some(CompilationSource {
kind: CompilationSourceKind::SourceCode(code),
span: extract_spans(&name_value).unwrap(),
});
} else if name_value.ident == "file" {
assert_source_empty(&source)?;
let path = match &name_value.lit {
Lit::Str(s) => PathBuf::from(s.value()),
_ => bail_span!(name_value.lit, "source must be a string"),
};
source = Some(CompilationSource {
kind: CompilationSourceKind::File(path),
span: extract_spans(&name_value).unwrap(),
});
}
if let Meta::NameValue(name_value) = meta {
if name_value.ident == "mode" {
mode = Some(match &name_value.lit {
Lit::Str(s) => match s.value().as_str() {
"exec" => compile::Mode::Exec,
"eval" => compile::Mode::Eval,
"single" => compile::Mode::Single,
_ => bail_span!(s, "mode must be exec, eval, or single"),
},
_ => bail_span!(name_value.lit, "mode must be a string"),
})
} else if name_value.ident == "module_name" {
module_name = Some(match &name_value.lit {
Lit::Str(s) => s.value(),
_ => bail_span!(name_value.lit, "module_name must be string"),
})
} else if name_value.ident == "source" {
assert_source_empty(&source)?;
let code = match &name_value.lit {
Lit::Str(s) => s.value(),
_ => bail_span!(name_value.lit, "source must be a string"),
};
source = Some(CompilationSource {
kind: CompilationSourceKind::SourceCode(code),
span: extract_spans(&name_value).unwrap(),
});
} else if name_value.ident == "file" {
assert_source_empty(&source)?;
let path = match &name_value.lit {
Lit::Str(s) => PathBuf::from(s.value()),
_ => bail_span!(name_value.lit, "source must be a string"),
};
source = Some(CompilationSource {
kind: CompilationSourceKind::File(path),
span: extract_spans(&name_value).unwrap(),
});
}
_ => {}
}
}

source
.ok_or_else(|| {
Diagnostic::span_error(
self.span.clone(),
self.span,
"Must have either file or source in py_compile_bytecode!()",
)
})?
Expand Down
2 changes: 1 addition & 1 deletion derive/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl Diagnostic {
}

pub fn from_vec(diagnostics: Vec<Diagnostic>) -> Result<(), Diagnostic> {
if diagnostics.len() == 0 {
if diagnostics.is_empty() {
Ok(())
} else {
Err(Diagnostic {
Expand Down
2 changes: 1 addition & 1 deletion derive/src/from_args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl ArgAttribute {
optional: false,
};

while let Some(arg) = iter.next() {
for arg in iter {
attribute.parse_argument(arg)?;
}

Expand Down
73 changes: 31 additions & 42 deletions derive/src/pyclass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,20 +63,14 @@ impl ClassItem {
NestedMeta::Meta(meta) => meta,
NestedMeta::Literal(_) => continue,
};
match meta {
Meta::NameValue(name_value) => {
if name_value.ident == "name" {
if let Lit::Str(s) = &name_value.lit {
py_name = Some(s.value());
} else {
bail_span!(
&sig.ident,
"#[pymethod(name = ...)] must be a string"
);
}
if let Meta::NameValue(name_value) = meta {
if name_value.ident == "name" {
if let Lit::Str(s) = &name_value.lit {
py_name = Some(s.value());
} else {
bail_span!(&sig.ident, "#[pymethod(name = ...)] must be a string");
}
}
_ => {}
}
}
item = Some(ClassItem::Method {
Expand Down Expand Up @@ -104,20 +98,17 @@ impl ClassItem {
NestedMeta::Meta(meta) => meta,
NestedMeta::Literal(_) => continue,
};
match meta {
Meta::NameValue(name_value) => {
if name_value.ident == "name" {
if let Lit::Str(s) = &name_value.lit {
py_name = Some(s.value());
} else {
bail_span!(
&sig.ident,
"#[pyclassmethod(name = ...)] must be a string"
);
}
if let Meta::NameValue(name_value) = meta {
if name_value.ident == "name" {
if let Lit::Str(s) = &name_value.lit {
py_name = Some(s.value());
} else {
bail_span!(
&sig.ident,
"#[pyclassmethod(name = ...)] must be a string"
);
}
}
_ => {}
}
}
item = Some(ClassItem::ClassMethod {
Expand Down Expand Up @@ -235,24 +226,22 @@ pub fn impl_pyimpl(_attr: AttributeArgs, item: Item) -> Result<TokenStream2, Dia
let ty = &imp.self_ty;
let mut properties: HashMap<&str, (Option<&Ident>, Option<&Ident>)> = HashMap::new();
for item in items.iter() {
match item {
ClassItem::Property {
ref item_ident,
ref py_name,
setter,
} => {
let entry = properties.entry(py_name).or_default();
let func = if *setter { &mut entry.1 } else { &mut entry.0 };
if func.is_some() {
bail_span!(
item_ident,
"Multiple property accessors with name {:?}",
py_name
)
}
*func = Some(item_ident);
if let ClassItem::Property {
ref item_ident,
ref py_name,
setter,
} = item
{
let entry = properties.entry(py_name).or_default();
let func = if *setter { &mut entry.1 } else { &mut entry.0 };
if func.is_some() {
bail_span!(
item_ident,
"Multiple property accessors with name {:?}",
py_name
)
}
_ => {}
*func = Some(item_ident);
}
}
let methods = items.iter().filter_map(|item| match item {
Expand Down Expand Up @@ -319,7 +308,7 @@ fn generate_class_def(
ident: &Ident,
attr_name: &'static str,
attr: AttributeArgs,
attrs: &Vec<Attribute>,
attrs: &[Attribute],
) -> Result<TokenStream2, Diagnostic> {
let mut class_name = None;
for attr in attr {
Expand Down
10 changes: 6 additions & 4 deletions parser/src/lexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,15 +488,15 @@ where
for i in 1..=literal_number {
match self.next_char() {
Some(c) => match c.to_digit(16) {
Some(d) => p += d << (literal_number - i) * 4,
Some(d) => p += d << ((literal_number - i) * 4),
None => return unicode_error,
},
None => return unicode_error,
}
}
match wtf8::CodePoint::from_u32(p) {
Some(cp) => return Ok(cp.to_char_lossy()),
None => return unicode_error,
Some(cp) => Ok(cp.to_char_lossy()),
None => unicode_error,
}
}

Expand Down Expand Up @@ -755,12 +755,14 @@ where
Ok(IndentationLevel { spaces, tabs })
}

#[allow(clippy::cognitive_complexity)]
fn inner_next(&mut self) -> LexResult {
if !self.pending.is_empty() {
return self.pending.remove(0);
}

'top_loop: loop {
// top loop
loop {
// Detect indentation levels
if self.at_begin_of_line {
self.at_begin_of_line = false;
Expand Down
2 changes: 1 addition & 1 deletion vm/src/builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ fn builtin_reversed(vm: &VirtualMachine, args: PyFuncArgs) -> PyResult {
vm.invoke(reversed_method?, PyFuncArgs::default())
} else {
vm.get_method_or_type_error(obj.clone(), "__getitem__", || {
format!("argument to reversed() must be a sequence")
"argument to reversed() must be a sequence".to_string()
})?;
let len = vm.call_method(&obj.clone(), "__len__", PyFuncArgs::default())?;
let obj_iterator = objiter::PySequenceIterator {
Expand Down
16 changes: 8 additions & 8 deletions vm/src/cformat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl CFormatSpec {
) -> String {
let mut num_chars = string.chars().count();
if let Some(num_prefix_chars) = num_prefix_chars {
num_chars = num_chars + num_prefix_chars;
num_chars += num_prefix_chars;
}
let num_chars = num_chars;

Expand Down Expand Up @@ -250,7 +250,7 @@ impl FromStr for CFormatString {
.or_else(|_| parse_specifier(cur_text))
.map(|(format_part, new_text, consumed)| {
parts.push((index, format_part));
index = index + consumed;
index += consumed;
new_text
})
.map_err(|(e, consumed)| CFormatError {
Expand Down Expand Up @@ -320,7 +320,7 @@ fn parse_literal(text: &str) -> Result<(CFormatPart, &str, usize), ParsingError>
match parse_literal_single(cur_text) {
Ok((next_char, remaining)) => {
result_string.push(next_char);
consumed = consumed + 1;
consumed += 1;
cur_text = remaining;
}
Err(err) => {
Expand Down Expand Up @@ -537,12 +537,12 @@ impl FromStr for CFormatSpec {
};

Ok(CFormatSpec {
mapping_key: mapping_key,
flags: flags,
mapping_key,
flags,
min_field_width: width,
precision: precision,
format_type: format_type,
format_char: format_char,
precision,
format_type,
format_char,
chars_consumed: calc_consumed(text, remaining_text),
})
}
Expand Down
4 changes: 2 additions & 2 deletions vm/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ impl FormatPreconversor {
}
}

pub fn from_str(text: &str) -> Option<FormatPreconversor> {
pub fn from_string(text: &str) -> Option<FormatPreconversor> {
let mut chars = text.chars();
if chars.next() != Some('!') {
return None;
Expand All @@ -33,7 +33,7 @@ impl FormatPreconversor {
}

pub fn parse_and_consume(text: &str) -> (Option<FormatPreconversor>, &str) {
let preconversor = FormatPreconversor::from_str(text);
let preconversor = FormatPreconversor::from_string(text);
match preconversor {
None => (None, text),
Some(_) => {
Expand Down
Loading