--- title: T-style triage meeting 2024-10-30 tags: ["T-style", "triage-meeting", "minutes"] date: 2024-10-30 discussion: https://rust-lang.zulipchat.com/#narrow/channel/346005-t-style/topic/meeting.202024-10-30 url: https://hackmd.io/y3UeO7ArRWmYE9WXyqMCUw --- # T-style meeting agenda - Meeting date: 2024-10-30 ## Attendance - People: TC, CE, Caleb, Josh ## Meeting roles - Facilitator: TC - Minutes: TC ## Check-in (Not minuted.) ## Scheduled design meetings None. Update these [here](https://github.com/orgs/rust-lang/projects/38/views/1). ## Proposed design meetings None. Update these [here](https://github.com/orgs/rust-lang/projects/38/views/1). ## Announcements or custom items (Meeting attendees, feel free to add items here!) ## Nominated RFCs, PRs, and issues ### "LHS/RHS Indentation changes for 2024" style-team#198 **Link:** https://github.com/rust-lang/style-team/issues/198 Caleb: I'm of the opinion we should pull this back a bit for binary expressions. CE: For the purposes of this meeting, binary expression means non-equality. Do you have any examples of how this makes things look better? Caleb: I have some examples: ```diff { if self.shape[(r as f32 + self.x_offset) as usize] [(c as f32 + self.y_offset) as usize] - != 0 + != 0 { // hello } ``` CE: What interesting is that the LHS is an lval, which would be the same as for an assignment. What's on the LHS is necessarily a subset of what's the RHS. So that may be why this works. TC: What I'd actually prefer, as compared to the above, is: ```rust { if self.shape[(r as f32 + self.x_offset) as usize] [(c as f32 + self.y_offset) as usize] != 0 { // hello } ``` TC: Not saying this is better, but this is the other consistent option: ```rust { if self.shape[(r as f32 + self.x_offset) as usize] [(c as f32 + self.y_offset) as usize] != 0 { // hello } ``` CE: Another caleb example of a binop that looks better: ```rust // 2015-2021 impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info + long_long_long_long_long_long_long_long_long_long_long_long_long_long_long .as_mut() .another_call() .get_extra_info(); } } ``` ```diff .as_mut() .expect("thing must exist") .extra_info - + long_long_long_long_long_long_long_long_long_long_long_long_long_long_long - .as_mut() - .another_call() - .get_extra_info(); + + long_long_long_long_long_long_long_long_long_long_long_long_long_long_long + .as_mut() + .another_call() + .get_extra_info(); } } ``` Caleb: Caleb's example that he thought was confusing w/ binops: ```rust fn snippet() { let new_veto_trim_value = (kind == FullCodeCharKind::InString || (config.style_edition() >= StyleEdition::Edition2024 && kind == FullCodeCharKind::InStringCommented)) && !line.ends_with('\\'); } ``` ```diff let new_veto_trim_value = (kind == FullCodeCharKind::InString || (config.style_edition() >= StyleEdition::Edition2024 && kind == FullCodeCharKind::InStringCommented)) - && !line.ends_with('\\'); + && !line.ends_with('\\'); } ``` Caleb: This causes me to have to scan diagonally, and I lose what the changed line is related to. TC: The first principle is to do no harm, so it seems clear we may want to limit this. CE: Agreed. It seems clear Caleb seems a problem here, and there seems a direction we could limit this. Josh: I've definitely run into cases that seems may be improved by this kind of change, but I'd also like to prefer doing no harm here. CE: You say that you have examples that would be made better, but you may want to recheck that these aren't affected by the runaway cases that Caleb has mentioned. Caleb: Within rustfmt we are doing this based on expression kind variants. Part of me is concerned about what could happen across a much larger set of code bases. I'm worried that there are cases that could be far worse than what I've seen so far. Doing it for assignment expression kinds and skipping it entirely for binary expressions or doing it for binary expressions if the Lh and RH did not have a nested binary expression with it -- those are options I'd be comfortable with on the rustfmt side. CE: I'd kind of just not want to do it for binary expressions. Josh: I share the concern about this perhaps being the tip of the iceberg. The cases we've seen suggests that our framing of the heuristic is failing at not wandering. We generalized from `=` to binary operators in general, and the heuristic we used is not successfully avoiding the repeated indentation here. CE: The real heuristic would have to be based on something in the AST. Something like using the the rightmost node, when you descend into the tree, when you don't, etc. CE: Maybe today, we could use the heuristic we have, as long as it's limited to lvals. Josh: For this example: ```diff= let value = abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ * abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ - - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ / - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ + - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ * - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ * - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ / - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ / - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ + - abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ; + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ / + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ + + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ * + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ * + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ / + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ / + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ + + abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ; } ``` CE: I'd like to see the indentation be a function of the operator precedence. Josh: We could insert all the parens that would be needed if there were precedence, then we could indent based on those, then delete the parens. CE: I'm not sure that mechanistically, that would work, but perhaps we could work something out. Caleb: For posterity, the pathological case shared on Zulip (and again below) stemmed from some expanded code, refs https://github.com/hyperium/http/pull/304 ```diff fn main() { - ((((((((((((((((((((((((((((((((((((((((((0) + 1) + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1) - + 1); + ((((((((((((((((((((((((((((((((((((((((((0) + 1) + 1) + 1) + 1) + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + 1) + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1) + + 1) + 1); } ``` Example for assignment operators: ```diff impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = - long_long_long_long_long_long_long_long_long_long_long_long_long_long_long; + long_long_long_long_long_long_long_long_long_long_long_long_long_long_long; } } ``` CE: It seems we have consensus on doing this revert. Caleb: So there are two things to do here. One is that we need to move some text from the binary operators section to the section on assignments. Two is that there is a rustfmt change here to adjust what we're doing for assignments. CE: We'd still be applying this to compound assignment operators, yes? Caleb: Yes. CE: I wouldn't expect any more issues from that, because of the constraints on what's on the LHS. Josh: Does this apply to `let`? CE: It would. Josh: Those can have non-trivial patterns. CE: I don't think rustfmt implements those, and we wouldn't want to add that at the last minute. ```rust fn main() { if let Some(ReallyLongPathReallyLongPathReallyLongPathReallyLongPathReallyLongPath::< sadasdhasjdakdhaskjdhakskdah, >) = 1 {} } ``` TC: We've been talking a lot about design axioms on the lang side, and this raises that for me here too. I'd like to have a design axiom like "the level of the indentation should follow the depth of the AST", and if we had such a design axiom, we could reject the heuristic here on that basis. Caleb/Josh: +1. Josh: I've submitted the PR for fixing the style guide: https://github.com/rust-lang/rust/pull/132369 ### "Style Guide prescriptions on ABI explicitness" style-team#199 **Link:** https://github.com/rust-lang/style-team/issues/199 Caleb: If this goes forward, we could remove some language and follow the AST here. CE: We don't need to wait on the RFC here. (The meeting ended here.) --- ### "Tracking Issue for Rust 2024: rustfmt enable `overflow_delimited_expr`" rust#123751 **Link:** https://github.com/rust-lang/rust/issues/123751 ### "Tracking Issue for Rust 2024: rustfmt style editions" rust#123799 **Link:** https://github.com/rust-lang/rust/issues/123799 ### "Tracking Issue for Rust 2024: rustfmt use version sort" rust#123800 **Link:** https://github.com/rust-lang/rust/issues/123800 ### "Tracking Issue for Rust 2024: rustfmt change sort to Unicode-aware "non-lowercase before lowercase"" rust#123802 **Link:** https://github.com/rust-lang/rust/issues/123802 ### "Tracking Issue for Rust 2024: rustfmt improve impl generics indentation " rust#124758 **Link:** https://github.com/rust-lang/rust/issues/124758 ### "Tracking Issue for Rust 2024: rustfmt trailing comment alignment" rust#124760 **Link:** https://github.com/rust-lang/rust/issues/124760 ### "Tracking Issue for Rust 2024: rustfmt raw identifier sorting" rust#124764 **Link:** https://github.com/rust-lang/rust/issues/124764 ### "Tracking Issue for Rust 2024: rustfmt closure body braces" rust#124767 **Link:** https://github.com/rust-lang/rust/issues/124767 ### "`edition.md` is causing many conflicts" style-team#192 **Link:** https://github.com/rust-lang/style-team/issues/192 ### "Formating of `match` should consider consistency across match arms" style-team#196 **Link:** https://github.com/rust-lang/style-team/issues/196 ## Pending PRs on the style-team repo None. ## RFCs waiting to be merged None. ## `S-waiting-on-team` None. ## Proposed FCPs **Check your boxes!** ### "style-guide: Note that we don't account for comments in every possible place" rust#121762 **Link:** https://github.com/rust-lang/rust/pull/121762 ## Active FCPs None. ## P-critical issues None. ## Check-out (Not minuted.)