Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a --show-source setting #698

Merged
merged 20 commits into from
Nov 18, 2022
Merged

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Nov 12, 2022

See #525.

cargo run -- --no-cache --select C resources/test/fixtures/ --show-source

image

@harupy harupy changed the title POC of --showsource` POC of --show-source Nov 12, 2022
@@ -13,6 +13,7 @@ edition = "2021"
name = "ruff"

[dependencies]
annotate-snippets = { version = "0.9.1", features = ["color"] }
Copy link
Contributor Author

@harupy harupy Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustc also uses this crate: rust-lang/rust#59346 (comment)

Copy link
Contributor Author

@harupy harupy Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like that uses https://crates.io/crates/ariadne under the hood.

@harupy harupy changed the title POC of --show-source Add --show-source flag Nov 12, 2022
@harupy harupy changed the title Add --show-source flag Add --show-source setting Nov 12, 2022
@harupy harupy changed the title Add --show-source setting Implement a --show-source setting Nov 12, 2022
@charliermarsh
Copy link
Member

\cc @squiddy

@charliermarsh
Copy link
Member

Wow nice. Is it possible to include the fix annotations / suggestions here too, like Clippy does?

@harupy
Copy link
Contributor Author

harupy commented Nov 12, 2022

That's what I'm investigating now :)

@charliermarsh
Copy link
Member

I do like the look and feel! And I love not implementing this ourselves.

@harupy
Copy link
Contributor Author

harupy commented Nov 13, 2022

@charliermarsh It looks like clippy relies on rustc_errors (rustc's private crate) for creating an error report.

src/message.rs Outdated Show resolved Hide resolved
@charliermarsh
Copy link
Member

Could we vendor that crate? Or is it too closely coupled to Clippy / rustc to be extracted?

@harupy
Copy link
Contributor Author

harupy commented Nov 13, 2022

This is rustc_errors' Cargo.toml:
https://github.com/rust-lang/rust/blob/fb6667a233fa677f6fbfa6067913ef3d32480317/compiler/rustc_errors/Cargo.toml

As you can see, it depends on other private crates:

[package]
name = "rustc_errors"
version = "0.0.0"
edition = "2021"

[lib]

[dependencies]
tracing = "0.1"
rustc_ast = { path = "../rustc_ast" }
rustc_ast_pretty = { path = "../rustc_ast_pretty" }
rustc_error_messages = { path = "../rustc_error_messages" }
rustc_serialize = { path = "../rustc_serialize" }
rustc_span = { path = "../rustc_span" }
rustc_macros = { path = "../rustc_macros" }
rustc_data_structures = { path = "../rustc_data_structures" }
rustc_target = { path = "../rustc_target" }
rustc_hir = { path = "../rustc_hir" }
rustc_lint_defs = { path = "../rustc_lint_defs" }
unicode-width = "0.1.4"
atty = "0.2"
termcolor = "1.0"
annotate-snippets = "0.9"
termize = "0.1.1"
serde = { version = "1.0.125", features = [ "derive" ] }
serde_json = "1.0.59"

[target.'cfg(windows)'.dependencies]
winapi = { version = "0.3", features = [ "handleapi", "synchapi", "winbase" ] }

src/message.rs Outdated
..Default::default()
},
};
DisplayList::from(snippet).to_string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if we're going to use this crate for --show-source, we should probably use the same crate and style even when --show-source is false (and just omit the source code). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I think we can set slices to an empty vector when --show-source is false.

@harupy harupy marked this pull request as ready for review November 13, 2022 07:34
@harupy
Copy link
Contributor Author

harupy commented Nov 13, 2022

Updated the screenshot.

src/cli.rs Outdated
@@ -94,6 +94,9 @@ pub struct Cli {
/// The name of the file when passing it through stdin.
#[arg(long)]
pub stdin_filename: Option<String>,
/// Show violations with source code.
#[arg(long)]
pub show_source: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add this to pyproject.toml too, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I forgot to update user.rs. Updated it.

src/message.rs Outdated
location: check.location,
end_location: check.end_location,
})
.len();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be .chars().len(), or we need to use the Rope in locator to get the start and end character positions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/message.rs Outdated
);
let slices = if self.show_source && self.source.is_some() && self.range.is_some() {
vec![Slice {
source: self.source.as_ref().unwrap(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe if let to avoid some of the unwraps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Will fix.

@charliermarsh
Copy link
Member

Sweet! Will try to merge this tomorrow (or maybe the next day -- I have to move apartments tomorrow so might be busy). I just want to do some performance testing before merging.

@charliermarsh
Copy link
Member

(Sorry, I'm a little behind, I haven't had a chance to benchmark yet.)

@harupy
Copy link
Contributor Author

harupy commented Nov 18, 2022

@charliermarsh Not a problem at all :)

@charliermarsh charliermarsh merged commit e81efa5 into astral-sh:main Nov 18, 2022
@harupy
Copy link
Contributor Author

harupy commented Nov 19, 2022

@charliermarsh Thanks for the clean-up and merge! This feature does help when verifying new lint rules.

@harupy harupy deleted the show-source branch November 19, 2022 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants