Drop `Violation::placeholder` #1996

not-my-profile · 2023-01-19T14:15:05Z

This PR removes the following associated function from the Violation trait:

fn placeholder() -> Self;

ruff previously used this placeholder approach for the messages it
listed in the README and displayed when invoked with --explain <code>.

This approach is suboptimal for three reasons:

The placeholder implementations are completely boring code since they
just initialize the struct with some dummy values.
Displaying concrete error messages with arbitrary interpolated values
can be confusing for the user since they might not recognize that the
values are interpolated.
Some violations have varying format strings depending on the
violation which could not be documented with the previous approach
(while we could have changed the signature to return Vec this
would still very much suffer from the previous two points).

We therefore drop Violation::placeholder in favor of a new macro-based
approach, explained in commit 4/5.

For the details please refer to the individual commit messages.

charliermarsh · 2023-01-19T15:22:53Z

ruff_dev/src/generate_rules_table.rs

+        let fix_token = match rule.autofixable() {
+            Autofixability::Never => "",
+            Autofixability::Sometimes => "🛠 (sometimes)",
+            Autofixability::Always => "🛠",


I'd prefer to leave this for now. We should make it clearer eventually, but I worry that as-is it'll hurt the readability and simplicity of the table.

I'm happy to play around with some alternate formats in a future PR.

(Unrelatedly: the use of the actual format strings is not only simpler for maintenance but so much clearer for users. Nice job.)

Fair enough I have removed the display of "(sometimes)".

charliermarsh · 2023-01-19T15:25:12Z

src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs

@@ -1,3 +1,23 @@
+//! Checks for unused loop variables.


Should these include the rule code at the end, like you did in the DTZ examples?

I don't think so. I think my goal is to centrally define all rule codes within a single file (including many to many mappings) and have our documentation generator combine these doc comments with the codes from that file.

I just included the codes in the datetimez comments since they were there previously ... but I don't think it really matters there since I think the datetimez rule implementations should ultimately be replaced by #1507.

(I still have to think about how to best implement the documentation generation from such doc comments ... but that's out of the scope of this PR.)

I mostly find those comments useful for grep. E.g., if someone reports a bug in SIM109, it's useful to be able to search /// SIM109 and jump to the definition. I think there's some value in making that easy, since even if we move away from those codes internally, they'll still pop up in issue reports and such. (It doesn't matter to me that they're in the rustdoc or not.)

Right I think we could end up having a cargo dev jump SIM109 command that would print where the respective rule is defined (if we end up no longer defining them next to the functions).

It doesn't matter to me that they're in the rustdoc or not.

The plan is that such doc comments will become the user documentation.

charliermarsh · 2023-01-19T15:29:29Z

ruff_macros/src/derive_message_formats.rs

+    quote! {
+        #func
+        fn message_formats() -> &'static [&'static str] {
+            &[#strings]


What's the failure mode in the event that a violation doesn't contain a format string?

Compilation fails with a nice error message:

error: expected last expression to be a format! macro or a match block --> src/violations.rs:539:9 | 539 | "foo".to_string() | ^^^^^

This commit series removes the following associated function from the Violation trait: fn placeholder() -> Self; ruff previously used this placeholder approach for the messages it listed in the README and displayed when invoked with --explain <code>. This approach is suboptimal for three reasons: 1. The placeholder implementations are completely boring code since they just initialize the struct with some dummy values. 2. Displaying concrete error messages with arbitrary interpolated values can be confusing for the user since they might not recognize that the values are interpolated. 3. Some violations have varying format strings depending on the violation which could not be documented with the previous approach (while we could have changed the signature to return Vec<Self> this would still very much suffer from the previous two points). We therefore drop Violation::placeholder in favor of a new macro-based approach, explained in commit 4/5. Violation::placeholder is only invoked via Rule::kind, so we firstly have to get rid of all Rule::kind invocations ... this commit starts removing the trivial cases.

While ruff displays the string returned by Violation::message in its output for detected violations the messages displayed in the README and in the `--explain <code>` output previously used the DiagnosticKind::summary() function which for some verbose messages provided shorter descriptions. This commit removes DiagnosticKind::summary, and moves the more extensive documentation into doc comments ... these are not displayed yet to the user but doing that is very much planned.

ruff_dev::generate_rules_table previously documented which rules are autofixable via DiagnosticKind::fixable ... since the DiagnosticKind was obtained via Rule::kind (and Violation::placeholder) which we both want to get rid of we have to obtain the autofixability via another way. This commit implements such another way by adding an AUTOFIX associated constant to the Violation trait. The constant is of the type Option<AutoFixkind>, AutofixKind is a new struct containing an Availability enum { Sometimes, Always}, letting us additionally document that some autofixes are only available sometimes (which previously wasn't documented). We intentionally introduce this information in a struct so that we can easily introduce further autofix metadata in the future such as autofix applicability[1]. [1]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_errors/enum.Applicability.html

The idea is nice and simple we replace: fn placeholder() -> Self; with fn message_formats() -> &'static [&'static str]; So e.g. if a Violation implementation defines: fn message(&self) -> String { format!("Local variable `{name}` is assigned to but never used") } it would also have to define: fn message_formats() -> &'static [&'static str] { &["Local variable `{name}` is assigned to but never used"] } Since we however obviously do not want to duplicate all of our format strings we simply introduce a new procedural macro attribute #[derive_message_formats] that can be added to the message method declaration in order to automatically derive the message_formats implementation. This commit implements the macro. The following and final commit updates violations.rs to use the macro. (The changes have been separated because the next commit is autogenerated via a Python script.)

# This commit has been generated via the following Python script: # (followed by `cargo +nightly fmt` and `cargo dev generate-all`) # For the reasoning see the previous commit(s). import re import sys for path in ( 'src/violations.rs', 'src/rules/flake8_tidy_imports/banned_api.rs', 'src/rules/flake8_tidy_imports/relative_imports.rs', ): with open(path) as f: text = '' while line := next(f, None): if line.strip() != 'fn message(&self) -> String {': text += line continue text += ' #[derive_message_formats]\n' + line body = next(f) while (line := next(f)) != ' }\n': body += line # body = re.sub(r'(?<!code\| |\.push\()format!', 'format!', body) body = re.sub( r'("[^"]+")\s*\.to_string\(\)', r'format!(\1)', body, re.DOTALL ) body = re.sub( r'(r#".+?"#)\s*\.to_string\(\)', r'format!(\1)', body, re.DOTALL ) text += body + ' }\n' while (line := next(f)).strip() != 'fn placeholder() -> Self {': text += line while (line := next(f)) != ' }\n': pass with open(path, 'w') as f: f.write(text)

not-my-profile · 2023-01-19T16:01:05Z

I have just changed the associated constant introduced in the 3rd commit from:

const AUTOFIXABLE: Autofixability = Autofixability::Never;

to

const AUTOFIX: Option<AutofixKind> = None;

so that we can easily introduce another field for applicability (#1997) in the future :)

charliermarsh · 2023-01-19T16:04:08Z

You're doing so much good work on pushing these abstractions forward. These are highly complex changes and you're handling them expertly. It's much appreciated.

not-my-profile force-pushed the drop-placeholder branch 5 times, most recently from 774e3e2 to ba68ce8 Compare January 19, 2023 14:58

charliermarsh reviewed Jan 19, 2023

View reviewed changes

not-my-profile added 6 commits January 19, 2023 16:45

refactor: Move a bunch of pandas-vet logic to rules::pandas_vet

f434c60

not-my-profile force-pushed the drop-placeholder branch from ba68ce8 to cb7463b Compare January 19, 2023 15:48

charliermarsh merged commit 3c3da8a into astral-sh:main Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop `Violation::placeholder` #1996

Drop `Violation::placeholder` #1996

not-my-profile commented Jan 19, 2023 •

edited

Loading

charliermarsh Jan 19, 2023

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023 •

edited

Loading

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023 •

edited

Loading

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023 •

edited

Loading

charliermarsh Jan 19, 2023

not-my-profile commented Jan 19, 2023 •

edited

Loading

charliermarsh commented Jan 19, 2023

Drop Violation::placeholder #1996

Drop Violation::placeholder #1996

Conversation

not-my-profile commented Jan 19, 2023 • edited Loading

charliermarsh Jan 19, 2023

Choose a reason for hiding this comment

charliermarsh Jan 19, 2023

Choose a reason for hiding this comment

not-my-profile Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Jan 19, 2023

Choose a reason for hiding this comment

not-my-profile Jan 19, 2023

Choose a reason for hiding this comment

charliermarsh Jan 19, 2023

Choose a reason for hiding this comment

not-my-profile Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Jan 19, 2023

Choose a reason for hiding this comment

not-my-profile Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Jan 19, 2023

Choose a reason for hiding this comment

not-my-profile commented Jan 19, 2023 • edited Loading

charliermarsh commented Jan 19, 2023

Drop `Violation::placeholder` #1996

Drop `Violation::placeholder` #1996

not-my-profile commented Jan 19, 2023 •

edited

Loading

not-my-profile Jan 19, 2023 •

edited

Loading

not-my-profile Jan 19, 2023 •

edited

Loading

not-my-profile Jan 19, 2023 •

edited

Loading

not-my-profile commented Jan 19, 2023 •

edited

Loading