Page MenuHomePhabricator

[analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script
ClosedPublic

Authored by Charusso on Nov 1 2019, 8:11 PM.

Details

Summary

This patch introduces a way to apply the fix-its by the Analyzer:
-analyzer-config apply-fixits=true.

The fix-its should be testable, therefore I have copied the well-tested
check_clang_tidy.py script. The idea is that the Analyzer's workflow
is different so it would be very difficult to use only one script for
both Tidy and the Analyzer, the script would diverge a lot.
Example test: // RUN: %check-analyzer-fixit %s %t -analyzer-checker=core

When the copy-paste happened the original authors were:
@alexfh, @zinovy.nis, @JonasToth, @hokein, @gribozavr, @lebedev.ri

Diff Detail

Event Timeline

Charusso created this revision.Nov 1 2019, 8:11 PM

Hey! I would like to reuse your script without any reinvention. It serves every needs, but at some point we start to heavily diverge. When I started with the Tidy I really enjoyed that script, and most of the people I know both develop Tidy and the Analyzer, so I would keep the design of the script to make it consistent to use.

Could you please let me use it as it is?

Charusso retitled this revision from [analyzer] FixItHint: apply and test hints with the Clang Tidy's script to [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script.Nov 1 2019, 8:30 PM
Charusso added a reviewer: dcoughlin.
zinovy.nis added inline comments.Nov 3 2019, 7:04 AM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
179

Isn't llvm_unreacheable too pessimistic? May be use diagnostics instead?

Charusso updated this revision to Diff 227640.Nov 3 2019, 6:22 PM
Charusso marked 2 inline comments as done.
  • Remove llvm_unreacheable from error-handling.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
179

Good idea, thanks! I was too optimistic so llvm_unreacheable should never fire.

lebedev.ri resigned from this revision.Nov 5 2019, 1:02 AM
lebedev.ri added inline comments.
clang/test/Analysis/check_analyzer_fixit.py
1 ↗(On Diff #227640)

This does work with python3?

NoQ accepted this revision.Nov 13 2019, 1:53 PM

I like this!

@gribozavr: It looks like @Charusso has independently implemented what we've talked about before. Do you agree that the duplication is inevitable here, or should we try to re-use code?

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
93

I'll be perfectly happy with removing FixitsAsRemarks entirely. Your new mechanism is superior.

clang/test/Analysis/check_analyzer_fixit.py
50–51 ↗(On Diff #227640)

I think there must be an os.path function for this.

59 ↗(On Diff #227640)

Mmm, what does this do?

This revision is now accepted and ready to land.Nov 13 2019, 1:53 PM

This patch introduces a way to apply the fix-its by the Analyzer:

I'm not sure this option is very useful... I don't know of anyone who uses the same option in Clang or ClangTidy. The only way I know people apply fixits is with the help of IDEs. I am also skeptical that people want to apply *all* fixits. Usually people want to pick a few specific ones, or all fixits of a certain kind; but not everything.

What workflow are you thinking of for this option?

NoQ added a comment.Nov 22 2019, 12:20 PM

This patch introduces a way to apply the fix-its by the Analyzer:

I'm not sure this option is very useful... I don't know of anyone who uses the same option in Clang or ClangTidy. The only way I know people apply fixits is with the help of IDEs. I am also skeptical that people want to apply *all* fixits. Usually people want to pick a few specific ones, or all fixits of a certain kind; but not everything.

What workflow are you thinking of for this option?

For now this is definitely for testing purposes only. This patch doesn't expose the option in any of the interfaces that are intended for actual users to use (-analyzer-config is not one of them).

I don't have any immediate plans on exposing this option to users. That said, the user can always apply fixits of a specific checker by only running that checker (or by only enabling fixits of this checker).

The only way I know people apply fixits is with the help of IDEs.

This depends on the infrastructure available. Talking specifically about clang-tidy in our environment, I know of at least three other modes that are being frequently used:

  • in a code review tool (allows to apply manually selected fixes one-by-one);
  • in a code browsing tool (allows to apply manually selected fixes or all fixes of a certain category - e.g. from performance-related checks - to a file or directory);
  • a script that applies pre-generated fixes to a set of files or all repository.

I am also skeptical that people want to apply *all* fixits. Usually people want to pick a few specific ones, or all fixits of a certain kind; but not everything.

While "all fixits" may be not particularly useful, "apply all fixes enabled for my project" is a reasonable function when the project is generally kept in a clean shape.

zinovy.nis accepted this revision.Dec 1 2019, 10:31 AM
JonasToth resigned from this revision.Jan 4 2020, 2:39 PM
Charusso updated this revision to Diff 241492.Jan 30 2020, 9:33 AM
Charusso marked 8 inline comments as done.
  • Change to 4-column space standard.
  • Simplify obtaining the Clang include directory.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
93

Okai, challenge accepted. Thanks!

clang/test/Analysis/check_analyzer_fixit.py
1 ↗(On Diff #227640)

I think it should. It is only made for running by the lit which is left in Python 2.

50–51 ↗(On Diff #227640)

I hope it is os.path.normpath().

59 ↗(On Diff #227640)

I think an empty truncate() does not do anything, so removed.

Charusso updated this revision to Diff 241493.Jan 30 2020, 9:37 AM
Charusso edited the summary of this revision. (Show Details)
  • Rename the script from check_analyzer_fixit.py to check-analyzer-fixit.py

Thanks for the reviews! Sorry for the delay.

A few nits.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
93

One variable per definition would result in a more readable code, IMO.

114

nit: I'd suggest naming the method closer to the name of the corresponding field, e.g. enableApplyFixIts. Why isn't this setApplyFixIts(bool) btw?

156–157

Cosmetics: I'd suggest to make the message less verbose. Specifically, to change

An error occured during fix-it replacement:
   <replacement>
The error message: <error message>

to

Error applying replacement <replacement>: <error message>
199

s/fix-it replacements/fix-it/

Charusso updated this revision to Diff 246209.Feb 24 2020, 7:43 AM
Charusso marked 4 inline comments as done.
Charusso retitled this revision from [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script to [analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script.
  • Fix.

Thanks for the reviews! Are we good to go?

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
114

We do not support the disable way of options, so let us make it enableApplyFixIts().

NoQ added inline comments.Feb 25 2020, 3:02 AM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
93

So, can we remove FixitsAsRemarks now or is anything still blocking it?

Charusso marked 2 inline comments as done.Feb 25 2020, 3:05 AM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
93
NoQ accepted this revision.Feb 25 2020, 3:15 AM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
93

Oh crap, how did i miss this? Thanks!!

Charusso marked an inline comment as done.Mar 3 2020, 9:29 PM

Thanks everyone! I hope the Analyzer developers start to use the wonderful features from Clang-Tidy.

Charusso marked 2 inline comments as done.Mar 3 2020, 9:30 PM
This revision was automatically updated to reflect the committed changes.