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

Idea: FIXME lints #2360

Closed
Centril opened this issue Jan 15, 2018 · 18 comments
Closed

Idea: FIXME lints #2360

Centril opened this issue Jan 15, 2018 · 18 comments
Labels
A-lint Area: New lints C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-AST Type: Requires working with the AST

Comments

@Centril
Copy link
Contributor

Centril commented Jan 15, 2018

I am unsure of whether this is the right place for this issue.

The idea was originally brought up at: rust-lang/rfcs#2281 (comment)

I haven't seen it personally yet (I'm still in college after all), but I've heard plenty of stories about how kludges and temporary workarounds end up being permanent simply because no one got around with putting a more robust fix in place. We can't fix developer laziness, but perhaps something like this could help people not forget tasks they defer to later, in general.

warning: FIXME not resolved for 2 years

One could perhaps write:

#[cfg_attr(feature = "cargo-clippy", FIXME("2018-01-15", "<description of what to fix>"))]
@oli-obk oli-obk added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-AST Type: Requires working with the AST A-lint Area: New lints C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. labels Jan 16, 2018
@eaglgenes101
Copy link

When I wrote of my original suggestion, I was thinking of it being rolled together with the RFC I suggested it on, based on time rather than availability or stability of features. But since it's been split off, I'll go along with it.

That said, giving it a fresh look, the most obvious problem would be to get people on board the new format, or to make the tools smart enough to figure things out for the end user. This might need some more thought than I first imagined.

@shaleh
Copy link

shaleh commented Feb 23, 2018

If clippy learned Git, you could find when the FIXME was added and compare that to current time. Which means the developer does not need to use a new format.

Or perhaps this is better done in a different linter.

@clarfonthey
Copy link
Contributor

Perhaps adding a fixme attribute to the compiler itself is a good idea?

@Centril
Copy link
Contributor Author

Centril commented Feb 24, 2018

@shaleh That seems like an effective idea, but I'd prefer not to have to jump between tools, it is better to just have to execute one command for all lints in my opinion. Also, not all projects use git =)

@shaleh
Copy link

shaleh commented Feb 24, 2018

I used Git as an example. A similar solution would work with other revision systems.

Teaching clippy about revision control is not that hard.

@Manishearth
Copy link
Member

I think this is out of scope for Clippy, at least the integration with VCSes

@shaleh
Copy link

shaleh commented Feb 24, 2018

I can understand that.

@theduke
Copy link

theduke commented Jul 30, 2019

This might be somewhat controversial (mainly because this could be seen as out of scope for Clippy), but apart from a FIXME attribute, I'd actually love a lint that detects FIXME in comments.
If you run on CI you can ensure that you never merge a branch with pending FIXMEs.

Is this something that would be accepted?

@shaleh
Copy link

shaleh commented Jul 30, 2019

I am a fan of those @theduke. Emacs marks FIXME, TODO, and XXX in bold, bright colors for me.

@flip1995
Copy link
Member

Is this something that would be accepted?

Yes, this could be implemented as a restriction lint.

@phansch
Copy link
Member

phansch commented Jul 31, 2019

It's worth noting that only doc comments are accessible to Clippy.
(Unless we parse all source files ourselves, but that's not a good idea for various reasons)

@theduke
Copy link

theduke commented Aug 2, 2019

@phansch that sadly means the idea is void, since most FIXME s would be in non-doc comments.

@martin-t
Copy link
Contributor

martin-t commented Feb 9, 2020

Something similar could be achieved using rustfmt's report_fixme. You should be able to set it to "Unnumbered", to force people to create issues for FIXMEs, then filter your project's issues by date.

The question is how to get more projects to discover and use it.

It's worth noting that only doc comments are accessible to Clippy.
(Unless we parse all source files ourselves, but that's not a good idea for various reasons)

@phansch Then how does rustfmt get access to comments? It seems to just use libsyntax,

@flip1995
Copy link
Member

flip1995 commented Feb 9, 2020

IIUC, rustfmt checks the text of files directly to find FIXMEs/TODOs, without parsing the file. This is not something Clippy should do IMO.

I think we can close this, since rustfmt implements this already?

@DeltaF1
Copy link

DeltaF1 commented Dec 15, 2022

rustfmt has removed FIXME support rust-lang/rustfmt#5102.

Now that Span has access to the source text "including spaces and comments", is this a viable thing to build into clippy?

@Manishearth
Copy link
Member

Manishearth commented Dec 15, 2022

The span thing has nothing to do with clippy capabilities. We can get the source text already.

But I'm not sure if we should be relexing everything to get the comments. It feels a bit weird in clippy, I'm not convinced this is the right place for it.

(the original proposal is kinda doable)

@DeltaF1
Copy link

DeltaF1 commented Dec 16, 2022

Makes sense, thanks :) So if in a hypothetical future syn gave access to (non-doc) comments programmatically then that would be a viable path to doing this, but for now it's not clippy's job to re-parse the source files.

@Manishearth
Copy link
Member

syn also has nothing to do with clippy's capabilities. we are not a proc macro, we do not use the user facing infra for operating on rust code, we use that of the compiler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-AST Type: Requires working with the AST
Projects
None yet
Development

No branches or pull requests