MarketSquare / robotframework-robocop

Tool for static code analysis of Robot Framework language
Apache License 2.0
190 stars 38 forks source link

[Rule] Not used keyword #1017

Open bhirsz opened 11 months ago

bhirsz commented 11 months ago

This rule was previously rejected because Robocop does not support dynamic code. Keywords can be defined under variables and executed using Run Keyword, resource can be imported dynamically, Python library keywords cannot be discovered without initializing library first etc. Those kind of limitations lead to creation of Sherlock which focus on using log from actually execution to avoid such issues. But maintaining yet another project is not feasible for me and there is little to no progress here recently despite it is one of most wanted feature from users.

But thanks to our planned feature - community rules it is possible to define optional rule. Such rule, with known limitations and pitfalls (which I will list in this thread and in the documentation) can be enabled by the user. Also we will need access to all files in the project which can be done thanks to #1015.

bhirsz commented 11 months ago

What this rule will not do:

What this rule will do:

To be editied - I will update with details later

bhirsz commented 11 months ago

Why I have returned to this rule - recently I had to refactor multiple Robot Framework projects with parts of the code dead/not working (which made using Sherlock not an option). I grow tired of manually checking multiple keywords and I wrote some scripts for scanning the repo. With the exception of one project which uses dynamic keywords it worked suprisingly well and encouraged me to migrate the code to Robocop rule (+ add various possible improvements).

borutzki commented 10 months ago

This one would be useful. With one switch I could stop teammates from adding dead code at any occassion (like, keywords that "will be used in some tests we are working on").

Lakitna commented 7 months ago

If it proves to be impossible to find unused keywords across multiple files, this can still include a rule like no-unused-private-keyword. A private keyword should be used inside the current file (details). The rule can check if each private keyword in the file is used.

bhirsz commented 7 months ago

If it proves to be impossible to find unused keywords across multiple files, this can still include a rule like no-unused-private-keyword. A private keyword should be used inside the current file (details). The rule can check if each private keyword in the file is used.

To some extent it is possible - I have some example scripts that does detect such cases across multiple files. But I also planned to start from only detecting the keywords that should be used in the same file (determined by whether file is resource or test/task file). Then expand it - similarly how I'm improving unused-variable rule over the time.

My main limitation is my time, all my work for tools is on expense of my time. Recently I have bit more time but I'm working on bugs/high priority tasks firsts but hopefully I will also get to this issue.

And thanks for link to docs, I didn't remember the 'private' tag - it definietely should be handled accordingly.

borutzki commented 7 months ago

For reference, seems like some kind of logic for that is implemented in RobotCode plugin for VS Code, though IMO it’s not a speed demon at this point. But there’s some functionality that counts uses of each keyword.

Lakitna commented 6 months ago

If the main constraint is time, I can help out a bit. Though it'll be somewhere next month due to... you guessed it, time constraints. Though, mine are due to a thing at the end of this month.

I've written a few business-specific rules recently and don't expect big issues getting started with this. The only technical limitation I see right now is detecting uses across multiple files. Is there an example of using multiple files?

bhirsz commented 6 months ago

We don't have anything 'in production' for multiple files but we have layed groundwork for it. For example there are tests with example rule that scans multiple files here https://github.com/MarketSquare/robotframework-robocop/tree/master/tests/atest/rules/custom_tests/project_checker . In some way existing rules already use multiple files - they just don't retain information between files. If you save the data it's possible for example to have rules that count number of tests in whole project etc.

But it's only basic skeleton of the checker. There are several ways of implementing such rule (in some ways I already did that in Sherlock). But most likely it will require resolving and visiting possible imports in the given file. Sometimes there are nested imports (suite import common file which imports other imports etc). I would not worry for now about dynamic names or imports. At first iteration it would be enough to just scan given file for keyword definitions (but only for files/keywords that should be used within given file, such as private keywords). Then scan for all keyword usage and whenever it matches previously found keyword definition, increment keyword usage. At the end report all keywords that were not used. Such rule doesn't even need to be project rule at first since it will only scan for keywords inside the same file.

bhirsz commented 6 months ago

I have started the work in https://github.com/MarketSquare/robotframework-robocop/compare/feature/unused_keyword?expand=1

It actually already detects not used within the suites and is ready to be extended for private keywords. I will keep working on it slowly. Most likely I will release it soon as it reaches some kind of MVP. Then it could be updated in the next releases.

Lakitna commented 5 months ago

@bhirsz as expected, my agenda has opened up a bit. Anything I can do to help you with this?

bhirsz commented 5 months ago

Great! You may start by looking at what I did in my branch from previous post. For example try to extend it by keywords with embedded variables (if possible, it can be tricky) or checking how to extend it to multiple files. Even if coding would be too much then just the tests itselff (examples of what should be catched as not used and what not) would be immensely helpful.

bhirsz commented 5 months ago

I have resumed work on the branch. Fixed some issues were it wasn't detecting properly and added support for keywords with embedded arguments and private keywords. It's getting close to release state. For now it only supports suites and resources with private keywords but we can extend it over releases like previously discussed