Rework the suspicious formatting lints. #12980

Jarcho · 2024-06-23T03:52:02Z

Short overview of the functional changes:

possible_missing_comma:
- Actually check that the formatting looks like a unary op.
- Also lint in tuples, parenthesis and argument lists.
- Check child expressions of binary operators as well. 1 -2 - 3 and 1 - 2 -3 should both be detected.
suspicious_assignment_formatting and suspicious_unary_op_formatting:
- Make sure the operator sequence is surrounded by white space.
suspicious_else_formatting:
- Always lint if there's a blank line before or after the else.
- Lint if an else appears immediately after a block comment, but a different line than the closing brace.

All lints now lint in macros and use suggestions. suspicious_else_formatting is still missing a suggestion for the case where the else is in a weird spot.

changelog: Lint suspicious formatting lints in macros

rustbot · 2024-06-23T03:52:07Z

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Alexendoo · 2024-07-07T14:23:33Z

clippy_utils/src/source.rs

+fn with_line_indent<T>(sm: &SourceMap, sp: Range<BytePos>, f: impl for<'a> FnOnce(&'a str) -> T) -> T {
+    let src = get_source_text(sm, sp);
+    let indent = if let Some(src) = &src
+        && let Some(line) = src.sf.lookup_line(src.range.start)
+        && let Some(start) = src.sf.lines().get(line)
+        && let Some(text) = src.sf.src.as_ref()
+    {
+        let text = if let Some(end) = src.sf.lines().get(line + 1) {
+            &text[start.to_usize()..end.to_usize()]
+        } else {
+            &text[start.to_usize()..]
+        };
+        &text[..text.len() - text.trim_start().len()]
+    } else {
+        ""
+    };
+    f(indent)
+}


This would be our third function that gets the indentation, or forth if you count the one on SourceMap too

The other SpanRangeExt methods look to be in a similar situation, could we replace the regular functions e.g. snippet_opt with ones that return a wrapper (Lrc<String>, Range<usize>) that has ToString, derefs to &str etc.? That would allow us to have a single set of functions that don't allocate without the callbacks which are slightly awkward

Could be Range<*const u8> instead of Range<usize> if we wanted to eagerly get rid of the extra dereference

I'm still in the middle of redoing how we handle this so things are still changing. Callbacks can be avoided now via:

if let Some(src) = span.get_source_text(cx) && let Some(src) = src.as_str() {}

The main reason this isn't simple is that the transition from span to source text can fail. Several of the functions are convenience wrappers around this. Going through some of them

check_source_text: makes it easier to write a short predicate on the text.

with_source_text and with_source_text_and_range: basically Option::map for the text.

source_text_to_string: snippet_opt, but works with more span-like types

write_source_text_to: same, but writes to something instead of creating a string

The remaining functions (map_range, with_leading_whitespace, trim_start) all adjust the span. This requires calculating a delta to apply to the real span so these are actually doing something.

~~with_line_indent could be changed to return Option<SomeTypeOwningPartOfTheSource>, but I would rather that be exposed differently.~~

Edit: with_line_indent never fails, so this would not translate to anything else nicely.

get_source_text already returns an Option, if we verify there and replace the pub fields with private + accessors we can treat SourceFileRange as an infallible str-ish

Then ToString would replace source_text_to_string, Display would replace write_source_text_to. get_source_text().map() could replace with_source_text and with_source_text_and_range, is_some_and for the check_ versions. Some of these are slightly longer to write but I think it's far clearer

That will add the minor cost of an extra range check and either an unwrap or an Lrc clone. I don't think this is ever needed on a hot path, but it's something to take into account.

Going function by function again:

check_source_text: This will be quite heavily used and it's existence can be justified on that alone. The fact that it performs better is another plus since it matters more for this use case.

with_source_text: I don't really care about. Both of it's current uses are questionable. This should really be named map_source_text.

source_text_to_string: This is used frequently enough that a separate function isn't a problem. Speed isn't as important here since we should only be using this to build suggestions. Ideally this would only be called inside span_lint_and_then.

write_source_text_to: Having a type which implements Display is definitely nicer to use.

For future work I want to find a nice way to expose the line spans as well. That's going to wait till I see more of how we would use it.

The extra check/unwrap could be removed with some unsafe, but I don't think it's crucial to do so. It would be faster than an allocation and convenient enough to use such that we can replace snippet_opt's return value with it without changing much code - I think any multipart suggestion where they're used directly would be most of it since that doesn't take impl ToString

I don't hate check_source_text, but I would still prefer .is_some_and as it's very clear what is happening and doesn't need a check_source_text_with_x variant

source_text_to_string is currently unused, but I assume you mean it to replace snippet_opt, I think if we get a nice string-like type there would be far fewer places that an owned String is actually required

Got rid of check_source_text_with_range since it's unlikely to be used much. The other added functions are range adjustments or have no failure states. Neither fit with returning a string-like type.

Jarcho · 2024-08-07T14:00:09Z

ping @Alexendoo.

Alexendoo · 2024-08-08T14:18:55Z

tests/ui/possible_missing_comma.rs

@@ -0,0 +1,75 @@
+//@no-rustfix


The tests with multiple suggestions should be rustfixable now

Could group them into a tests/ui/formatting dir too since it will be a good amount of files

Alexendoo · 2024-08-08T14:39:56Z