feat: check-in `cargo dylint` with `format_error` lint #13750

BugenZhao · 2023-11-30T09:05:50Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Check-in cargo dylint lints. See lints/README.md for more information.

As described in #13725, it's not a good practice to directly format an error with Display or Debug. However, it could be quite painful for developers to manually find and correct them.

In this PR, we leverage cargo dylint to write a custom lint called format_error and check the usages. The experience for both development and check is quite similar to clippy, so it can be easily continuously integrated.

To keep the PR clean, I haven't integrated it to CI or RiseDev now. Will do this in next PRs.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

xxchan · 2023-11-30T09:24:03Z

lints/Cargo.toml

+path = "ui/format_error.rs"
+
+[dependencies]
+clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "a585cda701581a16894858dc088eacd5a02fc78b" } # should match the toolchain version


Maybe we will need to add a script or checklist comments in rust-toolchain for updating toolchain now 🤣

BTW, why does it need to match

The internal API of rustc is not stable, which rust-clippy depends on.

Maybe we will need to add a script or checklist comments in rust-toolchain for updating toolchain now 🤣

It's okay. Mismatched version of clippy_utils usually won't compile.

codecov · 2023-11-30T09:32:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4b02d3a) 68.22% compared to head (f9d9c79) 68.22%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13750   +/-   ##
=======================================
  Coverage   68.22%   68.22%           
=======================================
  Files        1523     1523           
  Lines      262075   262075           
=======================================
  Hits       178801   178801           
  Misses      83274    83274

Flag	Coverage Δ
rust	`68.22% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xxchan

I wanted to rubber stamp, but one thing makes me hesitant: clippy is already very slow, and dylint needs to rebuild the project once again. How to resolve this issue?

liurenjie1024 · 2023-12-01T02:04:01Z

I wanted to rubber stamp, but one thing makes me hesitant: clippy is already very slow, and dylint needs to rebuild the project once again. How to resolve this issue?

+1. Is it really worth it?

TennyZhuang · 2023-12-01T03:50:06Z

Perhaps one lint is not worth it, but I can think of many rules, so I weakly support this.

TennyZhuang · 2023-12-01T03:50:53Z

and dylint needs to rebuild the project once again. How to resolve this issue

parallelize them?

BugenZhao · 2023-12-01T03:57:42Z

I wanted to rubber stamp, but one thing makes me hesitant: clippy is already very slow, and dylint needs to rebuild the project once again. How to resolve this issue?

+1. Is it really worth it?

It's not a question of worth or not because I can't think of any other way to prevent the loss of error information. 😕

In theory, the slowness of clippy is unrelated to the performance of our lints. Note that clippy has hundreds of lints, while we only have one at present. Thus, the process only involves running cargo check and this single lint. If check can be cached, then it would be fast. I'll do further local testing.

If the running time is really a problem, I guess linting in main-cron or even manually is still good.

BugenZhao · 2023-12-01T05:10:35Z

Whether or not we should integrate dylint to the development workflow is a topic in next PRs. In any case, checking in lints to our repository enable collaboration on improving the quality of RisingWave's codebase. I don't find any reason that I should keep this private on my own computer. 😄

stdrc

The concept looks really good to me. Previously we have some times when we wanted to add some custom lint rules but gave up. Such a dylint integration will make future custom linting easy. Thx!

stdrc · 2023-12-01T05:18:03Z

lints/src/utils/format_args_collector.rs

+
+impl EarlyLintPass for FormatArgsCollector {
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
+        if let ExprKind::FormatArgs(args) = &expr.kind {


Is format_args a special macro which the compiler is aware of?

I guess so, at the early pass.

liurenjie1024

LGTM

liurenjie1024 · 2023-12-01T06:02:37Z

lints/rust-toolchain

@@ -0,0 +1,3 @@
+[toolchain]
+channel = "nightly-2023-10-21"                      # should be identical to the root one


Is there anyway to sync this with root one automatically?

I haven't found one. But as mentioned by @xxchan at #13750 (comment), there's more stuff to do when bumping the toolchain. Maybe we need a dedicated script or instruction for this.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

xxchan · 2023-12-01T11:11:00Z

Whether or not we should integrate dylintto the development flow is a

topic in next PRs. In any case, checking in lints to our repository enable collaboration on improving the quality of RisingWave's codebase LGTM. Agree to check in and run daily and manually. Btw, by slowness I did mean cargo check is slow, and dylint doesn’t share build with clippy is worse.

…

On Fri, 1 Dec 2023 at 16:46, Bugen Zhao ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lints/src/utils/format_args_collector.rs <#13750 (comment)> : > +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_session::impl_lint_pass; +use rustc_span::{hygiene, Span}; + +/// Collects [`rustc_ast::FormatArgs`] so that future late passes can call +/// [`clippy_utils::macros::find_format_args`] +#[derive(Default)] +pub struct FormatArgsCollector { + format_args: FxHashMap<Span, Rc<FormatArgs>>, +} + +impl_lint_pass!(FormatArgsCollector => []); + +impl EarlyLintPass for FormatArgsCollector { + fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { + if let ExprKind::FormatArgs(args) = &expr.kind { I guess so, at the early pass. — Reply to this email directly, view it on GitHub <#13750 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJBQZNIQDP5LT3VABJCEJXDYHGKPFAVCNFSM6AAAAABAA2IVDCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJZGI4TENBYGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

xxchan · 2023-12-01T11:15:26Z

If the running time is really a problem

I think it is, as I already run clippy locally very rarely now..

…

On Fri, 1 Dec 2023 at 19:10, xxchan ***@***.***> wrote: > Whether or not we should integrate dylintto the development flow is a topic in next PRs. In any case, checking in lints to our repository enable collaboration on improving the quality of RisingWave's codebase LGTM. Agree to check in and run daily and manually. Btw, by slowness I did mean cargo check is slow, and dylint doesn’t share build with clippy is worse. On Fri, 1 Dec 2023 at 16:46, Bugen Zhao ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In lints/src/utils/format_args_collector.rs > <#13750 (comment)> > : > > > +use rustc_lint::{EarlyContext, EarlyLintPass}; > +use rustc_session::impl_lint_pass; > +use rustc_span::{hygiene, Span}; > + > +/// Collects [`rustc_ast::FormatArgs`] so that future late passes can call > +/// [`clippy_utils::macros::find_format_args`] > +#[derive(Default)] > +pub struct FormatArgsCollector { > + format_args: FxHashMap<Span, Rc<FormatArgs>>, > +} > + > +impl_lint_pass!(FormatArgsCollector => []); > + > +impl EarlyLintPass for FormatArgsCollector { > + fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { > + if let ExprKind::FormatArgs(args) = &expr.kind { > > I guess so, at the early pass. > > — > Reply to this email directly, view it on GitHub > <#13750 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AJBQZNIQDP5LT3VABJCEJXDYHGKPFAVCNFSM6AAAAABAA2IVDCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJZGI4TENBYGA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

BugenZhao added 4 commits November 30, 2023 16:45

feat: check-in cargo dylint

d07303c

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

refine docs

6a095de

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

add license header

6b1a8ba

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

update help

f5632d8

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao requested review from liurenjie1024, TennyZhuang, wangrunji0408 and xxchan November 30, 2023 09:05

BugenZhao requested a review from a team as a code owner November 30, 2023 09:05

github-actions bot added the type/feature label Nov 30, 2023

xxchan reviewed Nov 30, 2023

View reviewed changes

stdrc approved these changes Dec 1, 2023

View reviewed changes

BugenZhao mentioned this pull request Dec 1, 2023

refactor(error): simplify all boxed error wrapper definition #13725

Merged

4 tasks

liurenjie1024 approved these changes Dec 1, 2023

View reviewed changes

update lint to match to_string

f9d9c79

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao mentioned this pull request Dec 1, 2023

refactor(error): clean-up direct error formatting (part 1) #13763

Merged

9 tasks

BugenZhao added this pull request to the merge queue Dec 1, 2023

Merged via the queue into main with commit 61d364c Dec 1, 2023
31 of 32 checks passed

BugenZhao deleted the bz/dylint-checkin branch December 1, 2023 09:12

xxchan mentioned this pull request Dec 4, 2023

nexmark q19 and q20 blackhole performance degradation on affinity testbed between 1130-1203 #13773

Closed

BugenZhao mentioned this pull request Dec 4, 2023

refactor: reinvestigate error handling and reporting in the system #11443

Open

BugenZhao mentioned this pull request Jan 22, 2024

feat(ci): integrate with cargo-dylint #14713

Merged

4 tasks

BugenZhao mentioned this pull request Feb 28, 2024

lints: uncovered patterns of format_error #15330

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: check-in `cargo dylint` with `format_error` lint #13750

feat: check-in `cargo dylint` with `format_error` lint #13750

BugenZhao commented Nov 30, 2023 •

edited

Loading

xxchan Nov 30, 2023

xxchan Nov 30, 2023

BugenZhao Dec 1, 2023

BugenZhao Dec 1, 2023

codecov bot commented Nov 30, 2023 •

edited

Loading

xxchan left a comment •

edited

Loading

liurenjie1024 commented Dec 1, 2023

TennyZhuang commented Dec 1, 2023

TennyZhuang commented Dec 1, 2023

BugenZhao commented Dec 1, 2023

BugenZhao commented Dec 1, 2023 •

edited

Loading

stdrc left a comment

stdrc Dec 1, 2023

BugenZhao Dec 1, 2023

liurenjie1024 left a comment

liurenjie1024 Dec 1, 2023

BugenZhao Dec 1, 2023

xxchan commented Dec 1, 2023 via email

xxchan commented Dec 1, 2023 via email

		@@ -0,0 +1,3 @@
		[toolchain]
		channel = "nightly-2023-10-21" # should be identical to the root one

feat: check-in cargo dylint with format_error lint #13750

feat: check-in cargo dylint with format_error lint #13750

Conversation

BugenZhao commented Nov 30, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 30, 2023 • edited Loading

Codecov Report

xxchan left a comment • edited Loading

Choose a reason for hiding this comment

liurenjie1024 commented Dec 1, 2023

TennyZhuang commented Dec 1, 2023

TennyZhuang commented Dec 1, 2023

BugenZhao commented Dec 1, 2023

BugenZhao commented Dec 1, 2023 • edited Loading

stdrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan commented Dec 1, 2023 via email

xxchan commented Dec 1, 2023 via email

feat: check-in `cargo dylint` with `format_error` lint #13750

feat: check-in `cargo dylint` with `format_error` lint #13750

BugenZhao commented Nov 30, 2023 •

edited

Loading

codecov bot commented Nov 30, 2023 •

edited

Loading

xxchan left a comment •

edited

Loading

BugenZhao commented Dec 1, 2023 •

edited

Loading