Skip to content

Lint idea: Comments that contain TODO or FIXME #1267

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

Closed
carols10cents opened this issue Oct 6, 2016 · 18 comments
Closed

Lint idea: Comments that contain TODO or FIXME #1267

carols10cents opened this issue Oct 6, 2016 · 18 comments
Labels
A-lint Area: New lints E-hard Call for participation: This a hard problem and requires more experience or effort to work on L-correctness Lint: Belongs in the correctness lint group T-AST Type: Requires working with the AST

Comments

@carols10cents
Copy link
Member

Hi! This paper gave me some ideas for some lints. Here's one:

Warn for any usage of TODO or FIXME in a comment!

This could be any comment anywhere, or that paper pointed out that these comments in error handling code was a sign of code that could potentially cause bugs down the line.

@mcarton
Copy link
Member

mcarton commented Oct 6, 2016

That's a tricky one, Clippy does not see non-doc comments.

@mcarton mcarton added E-hard Call for participation: This a hard problem and requires more experience or effort to work on T-AST Type: Requires working with the AST A-lint Area: New lints L-correctness Lint: Belongs in the correctness lint group labels Oct 6, 2016
@llogiq
Copy link
Contributor

llogiq commented Oct 6, 2016

We should however be able to get the full source code for the crate by walking the source map. That may or may not include comments, however.

@mcarton
Copy link
Member

mcarton commented Oct 6, 2016

We'd have to then parse that to get all the comments somehow. Looks like syntax::parse::token::Token does not even keep the comments, just that there is a comment.
You might think “that's easy, we just need a very little subset of the language”. So did I when I first wrote the markdown lint 😅

@llogiq
Copy link
Contributor

llogiq commented Oct 6, 2016

I did not say that it was easy. Just that it was probably possible. 😄

@mcarton
Copy link
Member

mcarton commented Oct 6, 2016

And I did not say it was impossible. Just that is was E-hard 😄

@carols10cents
Copy link
Member Author

Oh geez! I did not even realize that :) eeeeeep.

@mcarton
Copy link
Member

mcarton commented Oct 16, 2016

@blaenk
Copy link

blaenk commented Jan 6, 2017

This is one feature I like from eslint and I agree it would be great to have. In eslint the rule is called no-warning-comments, in case any of you may derive some ideas from it.

@Manishearth
Copy link
Member

I don't think we should keep this open since rustfmt already can report TODOs and is configurable about it.

@blaenk
Copy link

blaenk commented Jan 16, 2017

Fair enough, I guess you're set on this.

The reason I was hoping to see it in clippy was because it would indirectly integrate with existing editor integration for reporting errors/warnings/notes in-editor. I have rustfmt integration in my editor (emacs) but the extent to which I use it is typically a keybind that simply formats the current file (or even auto trigger it on-save).

For example since eslint also supports this feature, TODOs, FIXMEs, and any other marker I wish to define will show up in the editor as a warning (or error if I set it to be an error) and I'll also see it in the list of errors/warnings in the editor. Using rustfmt for this purpose will either require each editor to add support for it specifically or we'll need to run it from the terminal in a way that is not as ergonomic. To be honest it feels pretty weird that it's implemented in rustfmt at all, as I tend to associate left-over TODOs/FIXMEs with linting than code style formatting.

I looked at the rest of the options rustfmt has and these two stand out as drastically different from the rest in that the majority of the options affect transformations on the code (which is why it's normal to just have a bind or on-file-save format the code) and these two are for reporting to the user that these items were found.

@shssoichiro
Copy link
Contributor

The reason I was hoping to see it in clippy was because it would indirectly integrate with existing editor integration for reporting errors/warnings/notes in-editor.

I personally don't see this as a blocker, because AFAIK, all major IDEs provide this capability either directly or through a popular plugin. IntelliJ highlights TODOs and FIXMEs and marks them on the scrollbar, Sublime Text's sublime-linter plugin has a plugin for annotations which covers this, and Atom certainly has something like this too, although I don't know the name of it off hand.

I think it would be nice to have clippy report this with its other warnings, but it seems like it would provide too little benefit compared to the difficulty of the task and the code complexity that would be added to clippy.

@ririsoft
Copy link

ririsoft commented Jul 9, 2021

This has now been removed from rustfmt and implemented in clippy.

@apollo-bobby
Copy link

This has now been removed from rustfmt and implemented in clippy.

That refers to the todo! macro as opposed to "TODO"s in comments.

@BatmanAoD
Copy link
Member

Is there any possibility of re-opening this now that rustfmt no longer has this capability?

@lomirus
Copy link

lomirus commented Nov 21, 2023

Adding lint by #[todo("We should xxx")]?

@BatmanAoD
Copy link
Member

@lomirus Are you suggesting that as an alternative to linting against comments?

@lomirus
Copy link

lomirus commented Nov 22, 2023

@BatmanAoD Yes. But Rust does not currently support this syntax, and I'm not sure if that's beyond the scope of this issue or clippy.

@BatmanAoD
Copy link
Member

Hm, I don't really see how that would be preferable to implementing the originally-requested feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-hard Call for participation: This a hard problem and requires more experience or effort to work on L-correctness Lint: Belongs in the correctness lint group T-AST Type: Requires working with the AST
Projects
None yet
Development

No branches or pull requests

10 participants