Skip to content

feat: add phpstan method annotations - #3331

Open
stobrien89 wants to merge 1 commit into
aws:masterfrom
stobrien89:feat-method-param-annotations
Open

feat: add phpstan method annotations#3331
stobrien89 wants to merge 1 commit into
aws:masterfrom
stobrien89:feat-method-param-annotations

Conversation

@stobrien89

Copy link
Copy Markdown
Member

Issue #, if available:
#3309

Description of changes:
Adds phpstan-method annotations to service client methods for better static analysis support.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@andrew-demb andrew-demb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPStan seems to be working properly, but PHPStorm doesn't understand multiline array type declarations in "@phpstan-method" tags and adds a lot of inspection errors to the file. But it will affect only package development, not consumers.

Overall, the original goal was reached successfully.

Image Image

Comment thread tests/Build/PhpStan/phpstan.neon
@andrew-demb

Copy link
Copy Markdown

Comment thread build/PhpstanShapeFormatter.php Outdated
$segments[] = "{$key}?: {$rendered}";
}

// Append the unsealed marker so PHPStan/Psalm don't flag

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is where I ran into severe issues with AWS, passing completely ignored arguments to the API, and almost destroying production resources

@stobrien89 stobrien89 Jul 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this is the false-negative we accept with the unsealed marker. Sealing here would catch BucktBucket, but it would also flag the common pattern of dynamically-built array<string, mixed> values passed as $args (request context, tracing metadata) where PHPStan/Psalm can't prove key set membership at the call site. What do you think about sealing top-level members, but leaving nested members unsealed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius bumping the above ^

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was on vacation

but it would also flag the common pattern of dynamically-built array<string, mixed>

For this, tools like PHPStan/Psalm/Mago/Phan have the concept of a "baseline", which is a sufficient approach to catch this sort of detail.

What do you think about sealing top-level members, but leaving nested members unsealed?

I think it wouldn't be added value here.

People that use PHPStan/Psalm/etc. can ignore these issues themselves, as an opt-in, either via a pattern or via a baseline.

Reporting these problems is more valuable than taking an upfront decision to suppress them for consumers.

@stobrien89
stobrien89 force-pushed the feat-method-param-annotations branch 4 times, most recently from 0d57dfc to 149da8e Compare July 26, 2026 19:15
@stobrien89
stobrien89 force-pushed the feat-method-param-annotations branch from 149da8e to d0e84a8 Compare July 26, 2026 19:40
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.

4 participants