Skip to content

Commit a717cb7

Browse files
authored
Adding linting to qsc_formatter and fixing the warnings (#2339)
This PR resolves issue #2276. There were several straightforward lints that made perfect sense to fix (e.g. [adding the must_use](https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate) and the [backticks](https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown)). But there were a few that I wasn't sure about: * too_many_lines: For these, I preferred to allow it rather than fix it. For the tests, I think that's the right approach. For the `apply_rules`, it's because I think the method is more readable as a very long function, especially given that it's the match statement that's taking up most of the lines. * match_same_arms: This one also relates to `apply_rules`. My opinion is that completely fixing the branches ends up going from many *straightforward* conditions to less (but still many) *complicated* conditions. So I fixed some of the arms but then disabled the lint for the rest. If you want, I can fix the rest of the arms instead. * trivially_copy_pass_by_ref: I'm not 100% sure I agree with removing the reference and using a copy instead, but clippy probably knows much better than me, so I listened to it and changed the function parameters from `&TokenKind` and `&Keyword` to `TokenKind` or `Keyword`.
1 parent 48100c2 commit a717cb7

File tree

5 files changed

+75
-55
lines changed

5 files changed

+75
-55
lines changed

compiler/qsc_formatter/Cargo.toml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,13 @@ qsc_frontend = { path = "../qsc_frontend" }
1717
expect-test = { workspace = true }
1818
indoc = { workspace = true }
1919

20+
[lints]
21+
workspace = true
22+
2023
[lib]
2124
doctest = false
2225

2326
[[bin]]
2427
name = "qsc_formatter"
2528
bench = false
2629
test = false
27-

compiler/qsc_formatter/src/bin/qsc_formatter.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ impl FileWalker {
101101
let formatted = format_str(&file_as_string);
102102
if file_as_string != formatted {
103103
match std::fs::write(path, formatted) {
104-
Ok(_) => {
104+
Ok(()) => {
105105
self.changed_files.push(path.display().to_string());
106106
}
107107
Err(e) => {
@@ -139,6 +139,7 @@ impl Display for OutputFormatting {
139139
}
140140
}
141141

142+
#[allow(clippy::too_many_lines)]
142143
fn main() -> Result<(), String> {
143144
use OutputFormatting::*;
144145

@@ -169,7 +170,7 @@ fn main() -> Result<(), String> {
169170
"{Passing}Updated {} files:",
170171
file_walker.changed_files.len()
171172
);
172-
for f in file_walker.changed_files.iter() {
173+
for f in &file_walker.changed_files {
173174
println!("\t{Passing}{f}");
174175
}
175176
print!("{Reset}");
@@ -181,7 +182,7 @@ fn main() -> Result<(), String> {
181182
"{Error}{} files are in need of formatting:",
182183
file_walker.changed_files.len()
183184
);
184-
for f in file_walker.changed_files.iter() {
185+
for f in &file_walker.changed_files {
185186
println!("\t{Error}{f}");
186187
}
187188
println!("{Error}Run the formatter with the `--write` option to correct formatting for the above files.{Reset}");
@@ -193,7 +194,7 @@ fn main() -> Result<(), String> {
193194
}
194195
if are_skipped_files {
195196
println!("{Skip}Skipped {} files:", file_walker.skipped_files.len());
196-
for f in file_walker.skipped_files.iter() {
197+
for f in &file_walker.skipped_files {
197198
println!("\t{Skip}{f}");
198199
}
199200
print!("{Reset}");

compiler/qsc_formatter/src/formatter.rs

Lines changed: 44 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ mod tests;
1818

1919
/// Applies formatting rules to the give code str and returns
2020
/// the formatted string.
21+
#[must_use]
2122
pub fn format_str(code: &str) -> String {
2223
let mut edits = calculate_format_edits(code);
2324
edits.sort_by_key(|edit| edit.span.hi); // sort edits by their span's hi value from lowest to highest
@@ -34,6 +35,7 @@ pub fn format_str(code: &str) -> String {
3435

3536
/// Applies formatting rules to the given code str, generating edits where
3637
/// the source code needs to be changed to comply with the format rules.
38+
#[must_use]
3739
pub fn calculate_format_edits(code: &str) -> Vec<TextEdit> {
3840
let tokens = concrete::ConcreteTokenIterator::new(code);
3941
let mut edits = vec![];
@@ -171,7 +173,7 @@ enum SpecDeclState {
171173

172174
/// Enum for a token's status as a delimiter.
173175
/// `<` and `>` are delimiters only with type-parameter lists,
174-
/// which is determined using the TypeParameterListState enum.
176+
/// which is determined using the `TypeParameterListState` enum.
175177
#[derive(Clone, Copy)]
176178
enum Delimiter {
177179
// The token is an open delimiter. i.e. `{`, `[`, `(`, and sometimes `<`.
@@ -227,6 +229,7 @@ struct Formatter<'a> {
227229
import_export_state: ImportExportState,
228230
}
229231

232+
#[allow(clippy::too_many_lines)]
230233
impl Formatter<'_> {
231234
fn apply_rules(
232235
&mut self,
@@ -243,7 +246,7 @@ impl Formatter<'_> {
243246
// when we get here, neither left nor right should be whitespace
244247

245248
let are_newlines_in_spaces = whitespace.contains('\n');
246-
let does_right_required_newline = matches!(&right.kind, Syntax(cooked_right) if is_newline_keyword_or_ampersat(cooked_right));
249+
let does_right_required_newline = matches!(right.kind, Syntax(cooked_right) if is_newline_keyword_or_ampersat(cooked_right));
247250

248251
// Save the left token's status as a delimiter before updating the delimiter state
249252
let left_delim_state = Delimiter::from_concrete_token_kind(
@@ -271,7 +274,8 @@ impl Formatter<'_> {
271274
matches!(right.kind, Comment),
272275
);
273276

274-
match (&left.kind, &right.kind) {
277+
#[allow(clippy::match_same_arms)]
278+
match (left.kind, right.kind) {
275279
(Comment | Syntax(DocComment), _) => {
276280
// remove whitespace at the ends of comments
277281
effect_trim_comment(left, &mut edits, self.code);
@@ -407,19 +411,29 @@ impl Formatter<'_> {
407411
self.indent_level,
408412
);
409413
}
410-
(_, Keyword(Keyword::Until))
411-
| (_, Keyword(Keyword::In))
412-
| (_, Keyword(Keyword::As))
413-
| (_, Keyword(Keyword::Elif))
414-
| (_, Keyword(Keyword::Else))
415-
| (_, Keyword(Keyword::Apply)) => {
414+
(
415+
_,
416+
Keyword(
417+
Keyword::Until
418+
| Keyword::In
419+
| Keyword::As
420+
| Keyword::Elif
421+
| Keyword::Else
422+
| Keyword::Apply,
423+
),
424+
) => {
416425
effect_single_space(left, whitespace, right, &mut edits);
417426
}
418-
(_, Keyword(Keyword::Auto))
419-
| (_, Keyword(Keyword::Distribute))
420-
| (_, Keyword(Keyword::Intrinsic))
421-
| (_, Keyword(Keyword::Invert))
422-
| (_, Keyword(Keyword::Slf)) => {
427+
(
428+
_,
429+
Keyword(
430+
Keyword::Auto
431+
| Keyword::Distribute
432+
| Keyword::Intrinsic
433+
| Keyword::Invert
434+
| Keyword::Slf,
435+
),
436+
) => {
423437
effect_single_space(left, whitespace, right, &mut edits);
424438
}
425439
(Close(Delim::Brace), _)
@@ -555,7 +569,7 @@ impl Formatter<'_> {
555569
}
556570
}
557571

558-
/// Updates the type_param_state of the FormatterState based
572+
/// Updates the `type_param_state` of the `FormatterState` based
559573
/// on the left and right token kinds.
560574
fn update_type_param_state(
561575
&mut self,
@@ -615,7 +629,7 @@ impl Formatter<'_> {
615629
}
616630

617631
/// Updates the indent level and manages the `delim_newlines_stack`
618-
/// of the FormatterState.
632+
/// of the `FormatterState`.
619633
/// Returns the current newline context.
620634
fn update_indent_level(
621635
&mut self,
@@ -678,7 +692,7 @@ fn get_token_contents<'a>(code: &'a str, token: &ConcreteToken) -> &'a str {
678692

679693
// Rule Conditions
680694

681-
fn is_bin_op(cooked: &TokenKind) -> bool {
695+
fn is_bin_op(cooked: TokenKind) -> bool {
682696
matches!(
683697
cooked,
684698
TokenKind::Bar
@@ -698,16 +712,15 @@ fn is_bin_op(cooked: &TokenKind) -> bool {
698712
| TokenKind::RArrow
699713
| TokenKind::WSlash
700714
| TokenKind::WSlashEq
701-
| TokenKind::Keyword(Keyword::And)
702-
| TokenKind::Keyword(Keyword::Or)
715+
| TokenKind::Keyword(Keyword::And | Keyword::Or)
703716
)
704717
}
705718

706-
fn is_prefix_with_space(cooked: &TokenKind) -> bool {
719+
fn is_prefix_with_space(cooked: TokenKind) -> bool {
707720
matches!(cooked, TokenKind::TildeTildeTilde)
708721
}
709722

710-
fn is_prefix_without_space(cooked: &TokenKind) -> bool {
723+
fn is_prefix_without_space(cooked: TokenKind) -> bool {
711724
matches!(
712725
cooked,
713726
TokenKind::ColonColon
@@ -717,22 +730,22 @@ fn is_prefix_without_space(cooked: &TokenKind) -> bool {
717730
)
718731
}
719732

720-
fn is_prefix(cooked: &TokenKind) -> bool {
733+
fn is_prefix(cooked: TokenKind) -> bool {
721734
is_prefix_with_space(cooked)
722735
|| is_prefix_without_space(cooked)
723736
|| matches!(cooked, TokenKind::DotDotDot)
724737
}
725738

726-
fn is_suffix(cooked: &TokenKind) -> bool {
739+
fn is_suffix(cooked: TokenKind) -> bool {
727740
matches!(cooked, TokenKind::Bang | TokenKind::Comma)
728741
}
729742

730-
fn is_prefix_keyword(keyword: &Keyword) -> bool {
743+
fn is_prefix_keyword(keyword: Keyword) -> bool {
731744
use Keyword::*;
732745
matches!(keyword, Not | AdjointUpper | ControlledUpper)
733746
}
734747

735-
fn is_keyword_value(keyword: &Keyword) -> bool {
748+
fn is_keyword_value(keyword: Keyword) -> bool {
736749
use Keyword::*;
737750
matches!(
738751
keyword,
@@ -742,7 +755,7 @@ fn is_keyword_value(keyword: &Keyword) -> bool {
742755
)
743756
}
744757

745-
fn is_newline_keyword_or_ampersat(cooked: &TokenKind) -> bool {
758+
fn is_newline_keyword_or_ampersat(cooked: TokenKind) -> bool {
746759
use Keyword::*;
747760
matches!(
748761
cooked,
@@ -770,22 +783,22 @@ fn is_newline_keyword_or_ampersat(cooked: &TokenKind) -> bool {
770783
)
771784
}
772785

773-
fn is_starter_keyword(keyword: &Keyword) -> bool {
786+
fn is_starter_keyword(keyword: Keyword) -> bool {
774787
use Keyword::*;
775788
matches!(
776789
keyword,
777790
For | While | Repeat | If | Within | New | Return | Fail
778791
)
779792
}
780793

781-
fn is_newline_after_brace(cooked: &TokenKind, delim_state: Delimiter) -> bool {
794+
fn is_newline_after_brace(cooked: TokenKind, delim_state: Delimiter) -> bool {
782795
is_value_token_right(cooked, delim_state)
783796
|| matches!(cooked, TokenKind::Keyword(keyword) if is_starter_keyword(keyword))
784797
|| matches!(cooked, TokenKind::Keyword(keyword) if is_prefix_keyword(keyword))
785798
}
786799

787800
/// Note that this does not include interpolated string literals
788-
fn is_value_lit(cooked: &TokenKind) -> bool {
801+
fn is_value_lit(cooked: TokenKind) -> bool {
789802
matches!(
790803
cooked,
791804
TokenKind::BigInt(_)
@@ -795,7 +808,7 @@ fn is_value_lit(cooked: &TokenKind) -> bool {
795808
)
796809
}
797810

798-
fn is_value_token_left(cooked: &TokenKind, delim_state: Delimiter) -> bool {
811+
fn is_value_token_left(cooked: TokenKind, delim_state: Delimiter) -> bool {
799812
// a closed delim represents a value on the left
800813
if matches!(delim_state, Delimiter::Close) {
801814
return true;
@@ -811,7 +824,7 @@ fn is_value_token_left(cooked: &TokenKind, delim_state: Delimiter) -> bool {
811824
}
812825
}
813826

814-
fn is_value_token_right(cooked: &TokenKind, delim_state: Delimiter) -> bool {
827+
fn is_value_token_right(cooked: TokenKind, delim_state: Delimiter) -> bool {
815828
// an open delim represents a value on the right
816829
if matches!(delim_state, Delimiter::Open) {
817830
return true;

0 commit comments

Comments
 (0)