Add a way to set the lint level on the function/statement level, without the register_tool feature

[xFrednet]

Rustc currently forbids lint names starting with a tool name, like marker::example. The register_tool feature (rust-lang/rust#66079) can be used to add marker as a tool. However, this would require every create that wants to set lint levels on a function/statement level to use nightly.

Ideally, I would like the error, to be reduced to a normal lint trigger, until the register_tool feature is discussed, one distant day. However, Marker still needs a solution meanwhile for crates that can't update to a new version of rustc.

I think the following two solutions should be investigated:

1. Conditional compilation, similar or the same like: https://github.com/trailofbits/dylint#conditional-compilation
2. See how rustc handles a prefix, like marker_ instead. This could be a viable solution, if this only triggers the unknown_lint lint of rustc

All working solutions should be documented :)

[NathanFrasier]

I'll take a look at this, although my immediate feeling is I will end up in over my head. I'll document anything I find here, but I wouldn't be expecting a PR soon.

[xFrednet]

Perfect! Next week, I plan to create some simpler issues, probably focussed on utilities for Marker, if that's something you would be interested in :). Any documentation for this one, would already be a big help :)

[xFrednet]

For the second point in the issue description, note that that should only be an unstable opt-in option from the user side. Maybe via an environment value or setting in the configuration :)

[Veetaha]

Not sure if you already knew it or if it helps, but beware that there is a hacky workaround that is definitely discouraged from anyone on the rustc developers team. But.. in the absence of a better solution it seems to be a lesser evil than installing a nightly toolchain.

rustc allows the usage of unstable features in the presence of RUSTC_BOOTSTRAP=1 environment variable. This way you can activate unstable features using a stable toolchain.

[NathanFrasier]

I really don't like that, because that would mean the feature would become available, but linted code would still need to use the unstable feature, by putting the register_tool into their code. so it's a really unpleasant user burden. I think that probably makes that approach a non-starter for this project, simply because the user would also need to do it. we could maybe do some kind of macro to only use the feature on linting runs, but that just feels like the wrong approach.

I'm going to try to use the conditional compilation approach, because it seems the most palatable, and it seems more stable than relying on undocumented rustc behavior. besides, if we do need the bootstrap hack, we'd probably only want it to happen on linting runs anyway, so we are back to conditional compilation.

[xFrednet]

I'm with @NathanFrasier on this one. However, it's an interesting hack to know regardless :)

Btw @NathanFrasier, did you make any progress on this one? Otherwise, I could take over. It's totally fine if you're still researching and want to take more time :). I just got back from a long weekend, and will try to catch up on everything first, and there are plenty of other issues for me to tackle :D

[NathanFrasier]

I didn't get terribly far, I spent a little bit of time setting up a more complete testing environment to test the issue, and I have that now, but I haven't really dug into the code yet. I'll probably continue to look into this regardless, my next step was probably to figure out how set a config flag during the driver run so that a conditional compile would have a flag to trigger off. That's just step 1 however, because that doesn't address the core issue of how to get cargo marker to accept the attributes without register_tool.

I'd like to keep looking into this, because it feels like a relatively concrete way to try and learn some of the internals on a deeper level. That said, you can get started on an actual implementation. My hope is that doing this will give me a better overview and I can at least be a competent reviewer of the final PR :)

If you'd like to give me more time and see what I come up with, I'm fine with that too. There's plenty of work to go around I suppose.

[xFrednet]

I didn't get terribly far, I spent a little bit of time setting up a more complete testing environment to test the issue.

Could you describe your setup? This sounds interesting, I'm not quite sure how to test this in the CI yet. I'm guessing it's just a local setup, but it would be interesting :)

I'd like to keep looking into this, because it feels like a relatively concrete way to try and learn some of the internals on a deeper level. That said, you can get started on an actual implementation. My hope is that doing this will give me a better overview and I can at least be a competent reviewer of the final PR :)

Okay, then I'll slowly start looking into this. Reviews are really valuable! ❤️

Writing more documentation about the structure and inner works of Marker is on the todo list. If you have any questions, I'm always happy to answer them :)

[NathanFrasier]

Could you describe your setup? This sounds interesting, I'm not quite sure how to test this in the CI yet. I'm guessing it's > just a local setup, but it would be interesting :)

Sure, although I don't think it's really anything special. I've got my local clone of the marker repo it's entirety. Then, as a sibling directory to that, I have a playground project that I've set up. playground is a bog standard cargo init repo, with the exception that I've created a workspace subcrate for playground-lints. This lints crate is where I will be putting testing lints, and the main crate is the linting target. I set things up this way because I imagine most projects big enough to have custom lints, are probably workspace projects anyway, and it mirrors what I expect to do for work whenever I get back to looking at custom lints. I used the https://github.com/rust-marker/lint-crate-template template to create my lint crate. (BTW this template crate should be featured much more prominently in the "Getting Started" section of the README).

I removed the ui tests from the linting crate, mostly because I'm trying to keep things minimal for now, and I also got a touch lazy.

To actually run my lints against my code, I use a normal cargo marker cmd call. To get new versions I just re-run cargo install with the path. This could obviously be improved, but it's a starting point.

[xFrednet]

That sounds like a good setup for cargo marker tests. I believe cargo uses a similar structure for testing. Marker should probably also have a setup like that. Thank you for sharing!

[NathanFrasier]

So I added a simple config flag, I've got my fork here: NathanFrasier:experiment/add-cfg-flag

I think this can actually be sufficient for our use case, because you can use the cfg flag both to conditionally compile the lint attribute, ie: #[cfg_attr(marker, allow(marker::my_lint))] but also the register_tool call, ie: #![cfg_attr(marker, feature(register_tool))] and #![cfg_attr(marker, register_tool(marker))]

The only reason I'm not sure is that I was having trouble getting the tests to behave in a way that I thought made sense. There is very possibly an issue still present with this implementation. It seemed like in some cases the attribute was passed, but in others it wasn't, code that should have produced warnings was sometimes caught by the linter and sometimes not. I don't think the issues I saw would be caused by #200 but I'm not sure.

A couple things to note about this bare bones approach.

1. In a certain sense, this still kind of requires users to "use nightly" by leveraging the nightly run of the linter. the register_tool call very much only works because cargo-marker already needs to always run nightly.
2. The conditional config for the lint seems very reasonable for a user to put in their code, however the other attributes feel a bit excessive, perhaps a more user ergonomic solution would be better.

I intend to keep testing with this, and possibly iterate on it, however I wanted my findings thus far to be documented.

[xFrednet]

Having a simple marker for cfgs is awesome! That's better than I expected.

Additional, I would like the option to filter for specific lint crates, similar to how dylint provides the dylint_lib. I'm guessing these can be added like features using --cfg 'dylint_lib="lint-crate-1"' --cfg 'dylint_lib="lint-crate-2"'. The question would be, which name we give the cfg set, I've been thinking about the following names:

• lints: Could be, but feels a bit off, since it actually checks the lint crates
• lint_crate: Works but seems long and verbose
• marks: I would find it a bit funny, to call lint crates marks, which also sounds similar to marker. It's short, but currently they're always called lint crates. Not sure if we want to add a new term to marker.

We could merge the PR for #200 today, it would probably be good to do that beforehand anyways. That will also help with testing.

@NathanFrasier do you want to give the additional filtering a try? Then I would start on the most fun part of this issue: Documentation. This will take some time, as I want to first setup an mdbook to then add a chapter about this. I would also leave the exact cfg names blank for now, until we made a decision on the naming :)

[xFrednet]

Oh as an additional idea, we could see if we can programmatically add marker as a tool. This would at least remove the reliance on the register_tool feature, even though it would still be unstable.

---

Edit:
It looks like this is currently not possible. However, it should be simple to add to rustc, the main question is, if such a change would be accepted. I think the best bet is to hope on some feedback in #66079.

The relevant function in rustc to find all tool names is rustc_resolve::registered_tools()

[NathanFrasier]

Having a simple marker for cfgs is awesome! That's better than I expected.

Yeah, I thought it was pretty elegant actually.

Additional, I would like the option to filter for specific lint crates, similar to how dylint provides the dylint_lib. I'm guessing these can be added like features using --cfg 'dylint_lib="lint-crate-1"' --cfg 'dylint_lib="lint-crate-2"'. The question would be, which name we give the cfg set, I've been thinking about the following names:

I actually think we might be able to just use marker for both. I assume the cfg! family of macros only check for the presence of the flag, and we could absolutely just supply the lib names with it. I want to test this more thoroughly before I commit to it as a plan, but I'd love to only add one config to check for users.

We could merge the PR for #200 today, it would probably be good to do that beforehand anyways. That will also help with testing.

Reviewed, for whatever my opinion is worth.

@NathanFrasier do you want to give the additional filtering a try?

Before I really continue adding more complexity, I'd really like to see a clean test case of what I currently have. I have no problem continuing to work on this, I just will need a bit of time, since I'm only volunteering nights to this. My tentative plan is to ensure that the current config flag works as expected, then attempt to just supply a --cfg=marker="crate_name" flag for each lint crate and see if that still works in the basic case. and then see if that allows for further filtering.

Then I would start on the most fun part of this issue: Documentation

I'm ok writing docs, if you set up an mdbook, I can try to write something up. Again, it's just going to be somewhat time limited.

[NathanFrasier]

Additional, I would like the option to filter for specific lint crates, similar to how dylint provides the dylint_lib.

I am admittedly, having a hard time seeing the utility of this. I'm all for supplying users with the info, I'm just struggling to see a user story where they would need this.

My (possibly wrong) understanding is that dylint needs it because each lint crate gets it's own lint run, and you need to only allow your attribute on the run of that lint crate, and remove it on all others. Would marker be subject to the same limitation?

[xFrednet]

I actually think we might be able to just use marker for both. I assume the cfg! family of macros only check for the presence of the flag, and we could absolutely just supply the lib names with it. I want to test this more thoroughly before I commit to it as a plan, but I'd love to only add one config to check for users.

That's a really cool idea! I didn't even think of that, that's why discussions are super helpful!

We could merge the PR for #200 today, it would probably be good to do that beforehand anyways. That will also help with testing.

Reviewed, for whatever my opinion is worth.

It's definitely helpful, I'll take a look at it :)

@NathanFrasier do you want to give the additional filtering a try?

Before I really continue adding more complexity, I'd really like to see a clean test case of what I currently have. I have no problem continuing to work on this, I just will need a bit of time

Then I would start on the most fun part of this issue: Documentation

I'm ok writing docs, if you set up an mdbook, I can try to write something up. Again, it's just going to be somewhat time limited.

Perfect, thank you! Then I'll look into setting up a mdbook!

[xFrednet]

Small update from the rust tracking issue (rust-lang/rust#66079) it sounds like anything in that regard is a bust, without a proper RFC. Sadly annoying, but nothing we can do about it right now.

The conditional compilation approach will be the best one. I also want to allow users to use an unstable hack, but the cfg solution, will be the main and recommended one :)

[xFrednet]

@NathanFrasier I've assigned you to the issue for overview purposes, hope that's okay with you :)

[NathanFrasier]

So I've finally gotten around to figuring out why the testing was so inconsistent. Now that I've had some time to just do some basic tests, it looks like my plan does pan out, with the slight caveat that I need to pass "--cfg=marker" and "--cfg=\"crate_name\"" in order to get the expected behavior. With that setup, you can use #![cfg_attr(marker, allow(foo))] and #![cfg_attr(marker = "my_crate", allow(foo))] and they both do the expected thing.

The only thing I'm struggling with right now is how to get the strings to put in these config flags. Ideally we use the crate name, but during the main function of the driver, it looks like we only have absolute paths to these crates. I think to get crate names, I just need to find a way to call cargo metadata on them, but I'm not entirely sure if that's the best approach, or maybe I'm just blind and there's an easy way to get these names that I just didn't see.

[xFrednet]

Do you really use "--cfg="crate_name""? It sounds like that would add the lint_crate as a normal cfg, like #[cfg(lint_crate)] would also work, which is something we probably don't want.

The only thing I'm struggling with right now is how to get the strings to put in these config flags. Ideally we use the crate name, but during the main function of the driver, it looks like we only have absolute paths to these crates.

Right now, the driver only has access to the absolute paths. Using cargo metadata would ignore lint crates specified with console arguments. I would suggest updating the format of the MARKER_LINT_CRATES environment value. This is the value, where the absolute paths are retrieved from. We can update it to use key value pairs or something similar. That should make is easy to get the names of the lint crates in the driver :)

[NathanFrasier]

Do you really use "--cfg="crate_name""? It sounds like that would add the lint_crate as a normal cfg, like #[cfg(lint_crate)] would also work, which is something we probably don't want.

Oops, no, I don't do that. That's the problem writing github comments after midnight. I pass "--cfg=marker=\"crate_name\"" like expected.

I'll see about updating that environment variable and try to get a PR up today.

[xFrednet]

Perfect, I've changed some of the code that reads the MARKER_LINT_CRATES value in the last PR. I would recommend rebasing on master, before you update the format. Then everything should be good to go :)

[xFrednet]

This had been closed by #214