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.

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

Make clippy happier #57

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Make clippy happier #57

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6c0bcc6
Remove Iterator::fold impl for Children
CAD97 Apr 26, 2020
ba1df75
Remove redundant lifetime annotation
CAD97 Apr 26, 2020
5228453
Remove identity calls to Into::into
CAD97 Apr 26, 2020
19a2c1f
Remove use of `unwrap_or` followed by a function call
CAD97 Apr 26, 2020
1441e73
Remove unnecessary reference of comparison operator
CAD97 Apr 26, 2020
be56ab8
Allow clippy::unit_arg
CAD97 Apr 26, 2020
3f3aa59
Use next instead of nth in s_expr example
CAD97 Apr 26, 2020
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
2 changes: 1 addition & 1 deletion examples/math.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ impl<I: Iterator<Item = (SyntaxKind, SmolStr)>> Parser<I> {
}

fn print(indent: usize, element: SyntaxElement) {
let kind: SyntaxKind = element.kind().into();
let kind: SyntaxKind = element.kind();
Copy link

Choose a reason for hiding this comment

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

Suggested change
let kind: SyntaxKind = element.kind();
let kind = element.kind();

print!("{:indent$}", "", indent = indent);
match element {
NodeOrToken::Node(node) => {
Expand Down
2 changes: 1 addition & 1 deletion src/cursor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ impl SyntaxNode {
/// Traverse the subtree rooted at the current node (including the current
/// node) in preorder, including tokens.
#[inline]
pub fn preorder_with_tokens<'a>(&'a self) -> impl Iterator<Item = WalkEvent<SyntaxElement>> {
pub fn preorder_with_tokens(&self) -> impl Iterator<Item = WalkEvent<SyntaxElement>> {
let start: SyntaxElement = self.clone().into();
iter::successors(Some(WalkEvent::Enter(start.clone())), move |pos| {
let next = match pos {
Expand Down
12 changes: 0 additions & 12 deletions src/green/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,18 +114,6 @@ impl<'a> Iterator for Children<'a> {
{
self.next_back()
}

#[inline]
fn fold<Acc, Fold>(mut self, init: Acc, mut f: Fold) -> Acc
where
Fold: FnMut(Acc, Self::Item) -> Acc,
{
let mut accum = init;
while let Some(x) = self.next() {
accum = f(accum, x);
}
accum
}
}

impl<'a> DoubleEndedIterator for Children<'a> {
Expand Down
10 changes: 6 additions & 4 deletions src/syntax_text.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ impl SyntaxText {
}

pub fn char_at(&self, offset: TextSize) -> Option<char> {
let offset = offset.into();
let mut start: TextSize = 0.into();
let res = self.try_for_each_chunk(|chunk| {
let end = start + TextSize::of(chunk);
Expand All @@ -59,7 +58,7 @@ impl SyntaxText {

pub fn slice<R: private::SyntaxTextRange>(&self, range: R) -> SyntaxText {
let start = range.start().unwrap_or_default();
let end = range.end().unwrap_or(self.len());
let end = range.end().unwrap_or_else(|| self.len());
Copy link
Member

Choose a reason for hiding this comment

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

false positive imo

assert!(start <= end);
let len = end - start;
let start = self.range.start() + start;
Expand Down Expand Up @@ -97,6 +96,7 @@ impl SyntaxText {

pub fn for_each_chunk<F: FnMut(&str)>(&self, mut f: F) {
enum Void {}
#[allow(clippy::unit_arg)]
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty 👎 on silencing clippy lints in code. I think this is a point where it clearly becomes a hindrance rather than a helper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know if I take a fully opposite position, but I do see value in locally allowing lints.

For one, it's explicitly saying "yes, this may be weird, but it's intended."

Secondly, and more importantly to me, is that a linter is most useful when it's silent. Warnings are a lot more useful when you only have a few and plan to address them all. Not suppressing false positives is how you get warning fatigue and ignoring new lints.

Anecdotal case in point is that the 10 or so clippy warnings this PR fixes were already enough to make me change my check workflow when working with rowan from clippy to check. With multiple warnings, some of which I have to ignore, it makes it harder to find the ones that are an improvement to fix.

In another formulation, imho, the lack of silencing the clippy lint harms clippy's usefulness more than some #[allow]s get in the way of reading code. (In fact, I think they have a (minor) positive that approximately pays for the vertical space they occupy.)

The TL;DR is that clippy has false positives, as a consequence of its design. The options are either to allow some lints sometimes or allow the false positives to overwhelm any useful information the linter provides.

That said, I'd be fine yeeting the two problematic commits and either globally allowing those lints or just stopping trying to use clippy on rowan, depending on your preference.

Copy link
Member

Choose a reason for hiding this comment

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

This is going to get a tad too philosophical, so bear with me.... (but the TL;DR is that I don't find clippy useful in this context).

I feel that the general "use linters" advice is too religiously carried over to Rust. In languages like TypeScript, for example, you basically cannot exist without linter, as even the simplest things, like "did I just drop this promise on the floor, really?" are not checked by the compiler. Basically, linterns tend to paper over compiler deficiencies.

In Rust, however, everything really important is already handled by compiler. For a user, who is already familiar with Rust idioms, majority of clippy suggestions are just a wash. Like in this PR:

  • removing intos is great (and should really be moved into the compiler)
  • removing fold's is great (and probably should be moved into the compiler as well)
  • the nth -> next change is negative, in my opinion. I've specifically used nth repeatedly, as it seems clearer for the example. It's not a big deal, it's just arbitrary change.
  • removal of explicit lifetime: it's, of course, not bad but it doesn't make the code better. Like, the next time I see it, i'll fix it. If I don't see it, than ok, I haven't spend my time on change.
  • unwrap_or_else is, again, slightly negative change, but the differences in utility is tiny in comparisong to the fact that this is a change.
  • the change in tests is pretty negative. It adds two new and (what's worse) two new names.

The overall theme here is that clippy makes me change my code, without making it better. It creates noise, and I think that noise has a pretty high cost. Similarly, explicit allows create noise. They don't show "something's fishy is going on" (b/c these are caught by compiler), they disproportionally highlight code trivialities.

What I can get behind is conservative clippy profile, consisting of a lints which are candidates for inclusion in the compiler. IIRC, there's no such clippy profile yet. What we do in rust-analyzer is that we define, globally, the list of disabled lints:

https://github.com/rust-analyzer/rust-analyzer/blob/7a9ba1657daa9fd90c639dcd937da11b4f526675/xtask/src/lib.rs#L107-L117

But we still don't run clippy on CI, as I find that would be mostly waste of time.

Where clippy shines is when teaching new users Rust idioms, but this use case is different.

I guess the bottom line is that I am 👎 on running clippy on CI, and, for this reason in particular, I don't want to see anything just to appease clippy in the code.

Copy link

Choose a reason for hiding this comment

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

Chiming in from Clippy: The Clippy philosophy is that sprinkling of allows is encouraged, for the reason @CAD97 already stated: "The following code is intentional."

I think disabling clippy::style and clippy::complexity lints already gets pretty close to what you want from Clippy. The clippy::correctness group exists for lints, that catch faulty code and I would recommend to everyone to at least use that lint group in every project (But I'm obviously biased).

match self.try_for_each_chunk(|chunk| Ok::<(), Void>(f(chunk))) {
Ok(()) => (),
Err(void) => match void {},
Expand Down Expand Up @@ -285,10 +285,12 @@ mod tests {
fn do_check(t1: &[&str], t2: &[&str]) {
let t1 = build_tree(t1).text();
let t2 = build_tree(t2).text();
let expected = t1.to_string() == t2.to_string();
let t1_s = t1.to_string();
let t2_s = t2.to_string();
let expected = t1_s == t2_s;
let actual = t1 == t2;
assert_eq!(expected, actual, "`{}` (SyntaxText) `{}` (SyntaxText)", t1, t2);
let actual = t1 == &*t2.to_string();
let actual = t1 == t2_s.as_str();
assert_eq!(expected, actual, "`{}` (SyntaxText) `{}` (&str)", t1, t2);
}
fn check(t1: &[&str], t2: &[&str]) {
Expand Down