-
Notifications
You must be signed in to change notification settings - Fork 11
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
My feedback for 0.1.1 #189
Comments
Comments are unfortunately not in the AST or HIR, so everything would need to be relexed for comments to be available. There's an AST utils function
I believe clippy-like utils functions are planned, like afaik,
Interesting, for me just
|
Thank you very much. This feedback is super valuable! ❤️
You can disable the warning by setting the
That's awesome!! I'm currently starting to collect a few example lints in the https://github.com/rust-marker/marker-example-lints repo. Would it be alright if that lint was added to the repo?
That's a good point, the caching check currently doesn't consider that path lint crates can change between runs. Nice catch! Just created an issue for this: #190
Every lint crate can only have one lint pass, the crate should then distribute the control flow internally. So, it's one lint pass for all lints. There is no proven structure for this yet, and something worth investigating. You mentioned the inventory crate earlier, it's something I will take a look at :)
Syntactic lifetimes, ie ones written by the user, should be available. Currently, I'm not planning to include the semantic ones, for a few reasons:
For me, the main question is: how much they are needed? Very few lints in Clippy require this information. Maybe it would be sufficient to provide a simple representation for semantic generics, without the entire lifetime analysis. 🤔
In addition to what @Centri3 said: Comments are a bit weird, since they don't follow the normal AST structure, they can basically show up wherever they want to. Someone wrote a related block regarding comments in JavaParser: https://tomassetti.me/java-comments-parsing/ . The basic problem was deciding where they belong to. There are no direct plans to include them, but also no decision to exclude them.
Currently, there is no concept for that, but I'm hoping to add something for that. However, my current plan will also require some support from rustc, so it'll take time, and it's not the highest prio.
Yeah, it's used to determine the lint level (and fulfill lint expectations once RFC-2383 is stabilized). Good point that the documentation is missing 👍
In Clippy, there was a discussion to potentially move a way from those, for two reasons:
The builder variant could be interesting. I don't like the required
Something like this is planned to be provided by
Yeah, this is a real limitation and it sounds like there is no plan to stabilize custom tool names anytime soon (See: rust-lang/rust#66079). Once Marker is more grown up, I hope to change the error to a new lint in rustc. But that's just an idea, which will require discussions with the compiler team. For now, the best way is to have a #[cfg_attr(marker, allow(marker::duck_lint))]
Absolutely true, I'm hoping to improve this for the next version. cc: #169
This mostly comes down to personal preference or inexperience. Thank you for this detailed feedback, this was exactly what I was hoping for! You touched on a few lints that you would like to implement. If you have the time, could you maybe create a User Story for them? |
@xFrednet I tried everything I could think of, but #![feature(register_tool)]
#![register_tool(marker)]
fn main() {
let path = std::path::Path::new("foo.txt");
#[allow(marker::to_string_on_cow_str)]
let _ = path.to_string_lossy().to_string();
} (btw I'd really like to avoid using the two attributes at the top if possible). The lint is not ignored, it's always reported when I run |
Hmmm, now that you mention it, I'm unsure if Marker actually registers the lint with rustc... This would mean that the lint level is not tracked right now... I will investigate this, thank you for pointing that out! Once the issue is fixed, there would be two options:
|
@xFrednet I think if Having a single-line So... if |
@xFrednet Regarding the points in #189 (comment).
Yes, sure.
It makes sense to describe the best practices for organizing a set of lints. Maybe something like "lint guidelines" for different use cases. But the best would be if
I think it is just not important where the comments are attached to. I know from This shows the AST representation of the Rust file where you can see what AST nodes the comments are attached to. In a nutshell, there isn't a strict single "right way" to put comments in the AST. Rust analyzer prefers nesting them under the item that follows the comment. For example, if there is a comment preceding a function or variable declaration, then it's nested under that item's AST node. The only thing is that this must be documented to let users of
I am not sure what exact differencies you imply between syntactic and semantic lifetimes. Users will definitely not need any results of borrow-checking analysis. I think the following set of operations should cover the use cases.
Linting the lifetimes usages on its own is definitely a very narrow and small area, so it's not worth spending too much time here. My comment was basically about the expectation to see at least some lifetime info in the generics of the
Sure will write them up when feeling inspired. Followup questionsWhat will happen if the lint does something stupid in the
|
TBH, IDK. The
Currently, I would copy a structure similar to Clippy. The structure would be:
This is the best I can come up with for now, until we have more crates that build on top of Marker's API. This structure is also used in marker-example-lints.
I would split lint crates based on the author/target. For example:
Here is an example:
Lint levels can be configured individually, like normal rustc lints. The lint crate authors have to make sure that each lint only lints one behavior. How lint levels can be defined is a bit up to rustc or generally speaking, the driver to decide. RFC 3389 might also be relevant here.
This depends on the chosen architecture and lint structure. This should be documented.
I have to think about it (And probably create an issue for further discussion) Thank you for the reference of
Every lifetime, actually written by the user, is represented in the syntactic representation of the node. Requesting the syntactic type of
I just tested it, and it's a bug, that the lifetime, is not included in the syntactic type (#208). It works for explicitly named ones, though: `_cow: Cow<'a, str>`
Lint crates are like proc macros, as they can basically do whatever they want. At the start, I tried to have lint crates sandboxed using web assembly, but this sadly doesn't work, as pointers don't work across the WASM boarder (At least from my testing).
Possible and currently unsound: #10
IDK, will probably terminate the process
It will block Marker, having a timeout might be a good feature, for the future:
Totally fine, the documentation (somewhere) defines that a lint crate is always called with the same thread.
Works as normal, this is kind of intentional, as I want to allow marker to be used for other things besides linting. A library could also use a lint crate to generate code, based on the loaded AST structure. Ideally, I would like crates to be more sandboxed with the user specifying permissions, but that is currently sadly not possible. (As far as I can tell)
Possible! Same problem as proc macros. |
I see, thank you for addressing my comments. I am closing this issue since I consider the questions resolved, and there were created separate issues for this feedback |
First of all, that's a great start! Writing custom lints feels rather nice for me, and doesn't need to depend on nightly at least from the interface side.
I used the
0.1.1
tooling to implement a lint that suggests usinginto_owned()
instead ofto_string()
forCow<'_, str>
.Here was my code:
Details
And it produced a clippy-style error output, which is awesome:
When running
cargo marker
(withrust-toolchain
file removed (#188)) on my huge private repo of hundreds of cratescargo marker
didn't choke, however, it spitted millions of warnings about the usage of async functions and blocks. And my customto_string_on_cow_str
lint also threw the warnings in the places where I expected them. So, currently, the lack of support for async is the most obvious blocker for me.The problems that I saw when using marker.
Feedback loop
I had to run
cargo clean
each time I change the lint and want to check how it works. My feedback loop was:cargo clean
cargo marker
If I omit the step
2
I still see the output from the old version of my lint code.I remember there was a different problem in
cargo-clippy
a long ago, which was also worked around withcargo clean
to bust the incremental compilation cache (rust-lang/rust-clippy#2604 (comment)). It isn't what happens here, but I hopemarker
won't have the same caveat.marker_api
export_lint_pass
could be a derive macro, or an attribute macro, which would reduce the boilerplate.It's not very clear how lints and lint passes should interact. Should I try to fit all lints in a single pass, or is it fine to have a single lint for a single pass struct? If yes, then maybe the code for a single lint and a single pass could be simplified? For example, the need to return the slice of lints in
info
method feels inconvenient. Could marker somehow benefit from theinventory
crate to avoid that?Lifetime parameters
There is no info about the lifetimes passed in generic args. For example, I expected that
Cow
's generic parameters would contain the lifetime parameter as their first argument, but there are only type arguments. Are there plans to provide access to lifetime parameters info in the AST?Comments
There is no way to lint code comments. I, for example, would like to enforce some conventions around TODO/FIXME comments and their format. Are there plans to provide access to code comments (not doc comments but regular comments) in the AST?
Macros
I would like to be able to lint the callsites of declarative macros. For example detect code like this:
tracing::error!(?err, "Error")
. Is it possible to see unexpanded macros? Will there be a concept of an early lint pass?emit_lint
The
emit_lint
method is the central method and requires documentation. It is not clear to me what thenode: impl Into<EmissionNode>
means. Is this used to somehow mark the place where the user may put an#[allow(marker::lint_name)]
to ignore the lint? How does theEmissionNode
interact with thespan
?The 5-th argument to
emit_lint
will not be used in 90% of cases, as to my naive first look. It's better to provide the most succinct API for general usage. I don't like thatemit_node
takes 5 arguments (that's too much). Ideally, there could be several methods for emitting lints with different levels of complexity. E.g.emit_lint(lint, msg, span)
,emit_lint_with_decor(lint, msg, span, decor_fn)
, etc. but it would be much better to have a builder API (cctyped_builder
orbuildstructor
crates for that, or handroll your own) for emitting the lint:A similar comment applies to
resolve_ty_ids().contains()
pattern. I think this is going to be repetitive, and it deserves a method e.g..is_type(str, type_id)
.Ignoring the lints
One of the main blocker for using
marker
in production for me is the inability to ignore the lints. I can't put#[allow(marker::lint_name)]
anywhere in my code because I get this:Unfortunately, the custom tool name feature is not stable in Rust, and I am not sure how ignoring the lints may then be used in stable. Maybe having a syntax with comments e.g.
// @allow marker::to_string_on_cow_str
could be used as a workaround? Do you have ideas on how this can be done in stable, or we will need to wait until custom tools are stabilized in Rust?Errors
I guess it's obvious that error output in
cargo-marker
is far from ideal. The error messages and context when something is messed up in configs need improvements. It's very easy to mess something up in Cargo.toml and get a rawNoTargetDir
output.In general, I recommend using libraries for logging and error handling such as:
tracing
thiserror
miette
Other questions
I skimmed through the code of cargo marker and found out it uses
clap
's builder API. I wonder why not to use itsderive
attributes? They are much more convenient because they provide strong typing, type safety and the lack of occasional dynamic invariants in the code.The text was updated successfully, but these errors were encountered: