Problem/Motivation

https://github.com/shipmonk-rnd/dead-code-detector looks like an interesting package, it has Symfony support already and perhaps we could provide any additional Drupal support that would be needed.

Steps to reproduce

Proposed resolution

Install the package
See what it reports when run against core

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3581155

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes

longwave opened merge request !15194

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

It doesn't understand things like attribute objects or hook classes, but otherwise it's catching a bunch of stuff. To pick some random examples:

  Line   core/modules/system/tests/src/Kernel/Block/SystemMenuBlockTest.php
 ------ ----------------------------------------------------------------------------------------
  55     Property Drupal\Tests\system\Kernel\Block\SystemMenuBlockTest::$block is never read
         🪪  shipmonk.deadProperty.neverRead
  55     Property Drupal\Tests\system\Kernel\Block\SystemMenuBlockTest::$block is never written
         🪪  shipmonk.deadProperty.neverWritten
  69     Property Drupal\Tests\system\Kernel\Block\SystemMenuBlockTest::$linkTree is never read
         🪪  shipmonk.deadProperty.neverRead

These are all correct as far as I can see.

  Line   core/modules/views/src/Controller/ViewAjaxController.php
 ------ ------------------------------------------------------------------------------
  51     Property Drupal\views\Controller\ViewAjaxController::$renderer is never read
         🪪  shipmonk.deadProperty.neverRead

Same.

  Line   core/modules/views/src/Form/ViewsForm.php
 ------ -------------------------------------------------------------------
  47     Property Drupal\views\Form\ViewsForm::$urlGenerator is never read
         🪪  shipmonk.deadProperty.neverRead

Same.

  Line   core/modules/workspaces/src/Hook/EntityTypeInfo.php
 ------ ------------------------------------------------------------------------------
  26     Property Drupal\workspaces\Hook\EntityTypeInfo::$workspaceInfo is never read
         🪪  shipmonk.deadProperty.neverRead

This one is wrong, because it considers the hook implementation methods as dead code.

Marking NR to discuss what to do next here; should we try to fix some of these, maybe in bulk by scoping this somehow? Tests are probably easier to reason about first, in other code we might need to consider subclasses and BC.

Should we add these to the baseline to try and prevent more creeping in when we refactor things? We would need to solve any false positives first, however.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Maybe we could first try Rector dead code ruleset - https://getrector.com/find-rule?activeRectorSetGroup=core&rectorSet=core...

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@mondrake I think those are complementary, so we could try that in a separate issue - the Rector rules look to detect dead/useless code within methods, while this package detects entire dead methods and member variables.

smustgrave’s picture

smustgrave commented

Came here after reviewing #3581404: Remove unused properties from kernel tests and definitely seems like an interesting library.

From what I can tell they do releases every 1-2 months
Low issue queue
I like the part that it has some twig interaction. Always wished we had some kind of twig linting and know that's not the same but least they would get scanned.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Should we add these to the baseline to try and prevent more creeping in when we refactor things? We would need to solve any false positives first, however.

I think this is the way forward, yes. I agree this would be good to do.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems like this got a number of +1 moving to NW to fix up the pipeline. @longwave feel free to ping me when ready.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We need to figure out the false positives and remove them first ideally, otherwise it will start failing on new MRs that have similar false positives. This might take a while.

In the meantime we can break out more child issues if we can figure out a reasonable way to scope them.

quietone’s picture

quietone commented

I used shipmonk/dead-code-detector a few days ago to cleanup the new Admin theme, #3582178: Remove dead code in Admin. Other than the false positives it was straightforward to make an MR to make the fixes. If that MR proves to not introduce errors, then I support adding this tool.

I have a bias against increasing the size of the baseline, so I would prefer to fix these first before adding the rule to core.

quietone’s picture

quietone commented

The number of instances of each identifier discovers

grep -A2 shipmonk.deadConstant core/.phpstan-baseline.php | grep path | sort -u | wc -l
22
grep -A2 shipmonk.deadEnumCase core/.phpstan-baseline.php | grep path | sort -u | wc -l
1
grep -A2 shipmonk.deadProperty.neverRead core/.phpstan-baseline.php | grep path| sort -u | wc -l
359
grep -A2 shipmonk.deadProperty.neverWritten core/.phpstan-baseline.php | grep path | sort -u | wc -l
79

edit: paste the correct code and output.

quietone’s picture

quietone commented

I'd fix them by identifier. And then in each group by the same distinctions usually used for coding standards. That is separate tests from non-tests. And sub-divide those as needed, so maybe tests in core/tests and tests in modules or themes.

  • shipmonk.deadConstant
  • shipmonk.deadEnumCase
  • shipmonk.deadProperty.neverRead
  • shipmonk.deadProperty.neverWritten
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

So that means 3 issues for the simplest ones, deadConstant, deadEnumCase, deadProperty.neverWritten? Those should be reviewable still I think. The neverRead is probably way too big to be easy to review.

Are those numbers after #3581407: Remove unused properties from unit tests and #3581404: Remove unused properties from kernel tests?