Run deadcode on edx-platform

[dianakhuang]

Run https://github.com/albertas/deadcode on edx-platform.

Acceptance Criteria

• Get results of deadcode
• Report them on this ticket
• Present them at the next DEPR meeting so we can discuss any interesting findings.

[mehedikhan72]

Hi, @dianakhuang. I've been trying to contribute to open Edx for a while now and this seems like something comfortable enough for me to get started with. Should I work on this issue?

And I'm quite new to open source so if I breach any common practices or make any mistakes, please correct me. I'm here to learn and add value to this repo.

[dianakhuang]

@mehedikhan72 we would welcome the help! Feel free to come visit us at the deprecation working group as well to talk over your findings and : https://discuss.openedx.org/t/announcing-the-deprecation-working-group/6173

I sketched out this ticket during a meeting, so feel free to ask for more information/details. This version of the ticket is very bare bones.

[mehedikhan72]

@dianakhuang Thank you. I appreciate that. I ran the following command on the edx-platform.
deadcode .
And it's been running and doing nothing for almost 50 minutes. I understand that edx-platform is a huge repo but is that supposed to take that long?

[mehedikhan72]

@dianakhuang the results are here. I've attached the txt file with this comment. Please check it out and let me know if it's alright.

deadcode-edx-platform-results.txt

[dianakhuang]

Hi @mehedikhan72 ! We discussed this during the DEPR working group meeting, and unfortunately, the results in that document are a little too noisy. It's still including tests, for example, and Django views that are not clearly referenced by other files.

If you still have time to work on this, could you look into how to configure deadcode to remove that noise? We would love to see what comes out of that.

[mehedikhan72]

Hi @dianakhuang! I think I can configure it such that tests are ignored. However, how do I know which django views are not clearly referenced by other files? If you could clairfy me on that, I could run the deadcode again. Thanks!

[dianakhuang]

I am not sure if there's a good way to find out if specific views get referenced elsewhere, but for example:

./lms/djangoapps/instructor/views/api.py:1845:4: DC100 Global reset_student_attempts_for_entrance_exam is never used
./lms/djangoapps/instructor/views/api.py:2060:4: DC100 Global rescore_entrance_exam is never used
./lms/djangoapps/instructor/views/api.py:2116:4: DC100 Global list_background_email_tasks is never used
./lms/djangoapps/instructor/views/api.py:2138:4: DC100 Global list_email_content is never used
./lms/djangoapps/instructor/views/api.py:2286:4: DC100 Global list_instructor_tasks is never used

I believe these endpoints can be called from the frontend, it's just that deadcode can't see those code paths, since they're coming from the frontend.

One resource that might be helpful is this blog post about using a different, but similar tool with Django.

[mehedikhan72]

Hi @dianakhuang . I have the updated results on edx-platform, both using vulture and deadcode. Tests are ignored this time.

What should I do next regarding this? Do you want me to start working on removing unused variables? Since functions might be connected with API endpoints, I think removing unused variables might be a good place to start with.

vulture-edx-platform.txt
edx-platform-deadcode-v2.txt

[dianakhuang]

Thanks @mehedikhan72 ! We'll take a look at these new results and discuss during the DEPR working group meeting today.

[jmbowman]

There's a script at https://github.com/openedx/edx-platform/blob/master/scripts/vulture/find-dead-code.sh for fine-tuning the vulture settings a bit to eliminate false positives, do you want to try that and see how it compares? I don't think we've run it in a while, so it might need some updates.

[mehedikhan72]

@dianakhuang alright, please keep me updated and let me know what I can do next.

Hi @jmbowman , I'll give it a look and let you know what I find.

[mehedikhan72]

Hi @jmbowman , this script produces only a few dead code instances since it excludes dead code instances with minimum confidence.

Previously the script was checking on cms, lms, common and openedx folders only. I checked on conf, docs, pavelib and xmodule as well.

Here are the results:
vulture-report.txt

From what I've read so far, removing codes where vulture gives a 90% and 100% confidence is a safe option considering, it shows a 90% confidence for unused imports and a 100% confidence for codes that cannot run logically.

Should I make a PR getting rid of these?

[dianakhuang]

This is great @mehedikhan72 ! Much less noisy than the previous ones. I do think there's a few false positives floating still around in there (the safe_lxml ones come to mind). If you want to try making these changes and seeing how the tests react, we'd welcome it, but I think we might need to pick over these a bit carefully to make sure they're actually ready to be removed.

[mehedikhan72]

@dianakhuang did you see the latest one where deadcodes with a high confidence is shown only? It ignores less confident results.

And how did you know that the safe_lxml ones are false positives? I need to be able to identify them in order to exclude them. Thanks.

[dianakhuang]

@mehedikhan72 Sorry it's taken so long to get back to you. I've been a bit busy the last couple of weeks. Talked it over with the team, and we're thinking it would make sense for a PR of the 100% confidence suggestions at first if you still have the time and interest in doing so.

I only knew that the safe_lxml ones are false positives because I've been in that code in the past few months, and we're doing a weird monkey-patching thing. I don't think there's any obvious way to tell otherwise, unfortunately.

[mehedikhan72]

@dianakhuang no worries. I'm working on it.