Skip to content

Fix ClassMustBeFinal final insertion for #11460#11855

Open
HenkPoley wants to merge 1 commit into
vimeo:6.xfrom
HenkPoley:fix/classmustbefinal-final-insertion
Open

Fix ClassMustBeFinal final insertion for #11460#11855
HenkPoley wants to merge 1 commit into
vimeo:6.xfrom
HenkPoley:fix/classmustbefinal-final-insertion

Conversation

@HenkPoley

@HenkPoley HenkPoley commented May 19, 2026

Copy link
Copy Markdown
Contributor

ClassMustBeFinal used a text search over the class statement snippet and treated the relative match offset as a file offset. During --alter this could insert "final" near the start of the file, such as before <?php, before a namespace/use statement, or inside leading text.

The fixes implemented:

  • uses an absolute file offset instead of a relative snippet offset
  • inserts before readonly class, producing final readonly class
  • avoids adding manipulations for suppressed ClassMustBeFinal issues

It also fixes remaining cases not covered by PR #11542:

  • preserves attribute placement by inserting after attributes and before class
  • avoids duplicate indentation by not preserving indentation for "final "
  • avoids matching "class" inside docblocks or other non-declaration text
  • there are PHPUnit tests

The insertion point is now derived from the actual class declaration token in the snippet, then translated back to the corresponding file offset before the file manipulation is added. This should fix Issue #11460.


This is LLM-assisted code, GPT-5.5 Low in Codex. I did review it, and looks like it does what it should do. I see token-get-all() is also used to (re-)parse PHP in some other gnarly spots. Further fixes of the regex code from the original PR did not seem to be a viable approach.

ClassMustBeFinal used a text search over the class statement snippet and treated the relative match offset as a file offset. During --alter this could insert "final" near the start of the file, such as before <?php, before a namespace/use statement, or inside leading text.

This keeps the fixes intended by PR vimeo#11542:

- uses an absolute file offset instead of a relative snippet offset
- inserts before readonly class, producing final readonly class
- avoids adding manipulations for suppressed ClassMustBeFinal issues

It also fixes remaining cases not covered by PR vimeo#11542:

- preserves attribute placement by inserting after attributes and before class
- avoids duplicate indentation by not preserving indentation for "final "
- avoids matching "class" inside docblocks or other non-declaration text

The insertion point is now derived from the actual class declaration token in the snippet, then translated back to the corresponding file offset before the file manipulation is added.
@HenkPoley

Copy link
Copy Markdown
Contributor Author

Hmm, the CircleCI failure is inside src/Psalm/Type/Atomic/TLiteralFloat.php; which I did not patch.

Am I supposed to base my patch on top of the last non-CircleCI-failing push?

@HenkPoley

HenkPoley commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Just to show how annoying this bug is with a bog standard Laravel install:

composer global require laravel/installer
# Just accept the defaults to create a new Laravel project folder
laravel new barebones-laravel
cd barebones-laravel
# Add laravel-psalm to make working with Laravel's magic less annoying
composer config minimum-stability dev && composer config prefer-stable true
composer require --dev psalm/plugin-laravel:^4.*
./vendor/bin/psalm-laravel init
# So we can track what Psalm butchers
git init
git add .
git commit -m 'Initial commit'
# optional
./vendor/bin/psalm-laravel analyze
# Now muck up the PHP code with `psalter` 👇
./vendor/bin/psalter --issues=all
# Search for 'final' by typing /final
git diff

Excerpts where final is inserted incorrectly, without any patching:

diff --git a/app/Models/User.php b/app/Models/User.php
index 68f3a66..cfc4480 100644
--- a/app/Models/User.php
+++ b/app/Models/User.php
@@ -1,4 +1,4 @@
-<?php
+final <?php
 
 namespace App\Models;
 
diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php
index 452e6b6..925a894 100644
--- a/app/Providers/AppServiceProvider.php
+++ b/app/Providers/AppServiceProvider.php
@@ -1,4 +1,4 @@
-<?php
+final <?php
 
 namespace App\Providers;
 
diff --git a/database/factories/UserFactory.php b/database/factories/UserFactory.php
index c4ceb07..a89da1b 100644
--- a/database/factories/UserFactory.php
+++ b/database/factories/UserFactory.php
@@ -1,6 +1,6 @@
 <?php
 
-namespace Database\Factories;
+namespace Database\Factoriefinal s;
 
 use App\Models\User;
 use Illuminate\Database\Eloquent\Factories\Factory;
diff --git a/database/seeders/DatabaseSeeder.php b/database/seeders/DatabaseSeeder.php
index 6b901f8..3af438d 100644
--- a/database/seeders/DatabaseSeeder.php
+++ b/database/seeders/DatabaseSeeder.php
@@ -1,4 +1,4 @@
-<?php
+final <?php
 
 namespace Database\Seeders;

@HenkPoley

Copy link
Copy Markdown
Contributor Author

I verified that my PR still places the final keyword in the correct place when using Psalm 7.0.0-beta19@7e751c06a756fa64dc4c759c09fe4a173afcb43; also known as Psalm 7.0.0-beta19.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant