PHPLIB-1250 Split encoders and fix psalm issues #46

alcaeus · 2024-01-23T10:45:00Z

This PR splits the main BuilderEncoder into multiple files. This allows for more isolation of separate code (which I'm sure can be improved further), but also allows for easy introduction of custom codecs (currently not tested yet).

In addition, this PR cleans up a number of psalm issues and adds other issues to the baseline. I'll comment on those as necessary.

alcaeus · 2024-01-23T10:45:27Z

psalm-baseline.xml

-<files psalm-version="5.15.0@5c774aca4746caf3d239d9c8cadb9f882ca29352">
+<files psalm-version="5.20.0@3f284e96c9d9be6fe6b15c79416e1d1903dcfef4">
+ <file src="src/Builder/Encoder/AbstractExpressionEncoder.php">
+ <MixedAssignment>


A lot of MixedAssignment calls are the result of incomplete type annotations throughout the generated files.

alcaeus · 2024-01-23T10:46:02Z

psalm-baseline.xml

+ </MixedAssignment>
+ </file>
+ <file src="src/Builder/Encoder/FieldPathEncoder.php">
+ <NoInterfaceProperties>


Fixing this involves creating a new generator for field path classes so we can add a getName interface methods. It irks me that public properties cannot be part of the interface contract, but here we are.

alcaeus · 2024-01-23T10:46:54Z

psalm-baseline.xml

+ <code>$val</code>
+ <code>$val</code>
+ </MixedAssignment>
+ <UndefinedConstant>


Similar to the interface property above, this undefined constant stems from us checking for an interface, then accessing a class constant.

According to Object Interfaces: Constants, PHP 8.1 allows overriding constants. Is there any reason Psalm cannot support this?

Or does it stem from OperatorInterface not actually defining the constant, and it just being a convention in this library for implementing classes to do so? If so, perhaps we need to incorporate a new method for the encoding directive?

Removed this entry and added a constant to OperatorInterface.

alcaeus · 2024-01-23T10:48:39Z

psalm-baseline.xml

+ </MixedAssignment>
+ </file>
+ <file src="src/Builder/Projection/ElemMatchOperator.php">
+ <MixedArgumentTypeCoercion>


MixedArgumentTypeCoercion errors are due to the query expression not using the correct array element types but instead using the array type. I tried to fix this, but the code generator doesn't accept a type like array<int|string>. This will have to be looked at separately.

alcaeus · 2024-01-23T10:49:59Z

psalm-baseline.xml

+ <PropertyTypeCoercion>
+ <code>$expression</code>
+ </PropertyTypeCoercion>
+ <TooManyTemplateParams>


We use stdClass<foo|bar> to indicate an object with an unknown number of properties of a specific type. However, stdClass and object cannot be templated this way, which is why we're seeing this error. The stdClass{foo: int} syntax doesn't work for us here as we don't know the keys of the object, but only the expected value types. I've asked in the Symfony Slack to get more info about this and will update once I know more.

alcaeus · 2024-01-23T10:51:08Z

psalm-baseline.xml

+ </TooManyTemplateParams>
+ </file>
+ <file src="src/Builder/Type/OutputWindow.php">
+ <DocblockTypeContradiction>


This and the RedundantConditionGivenDocblockType checks are psalm telling us that the condition is impossible due to the docblock. However, since we're being extra cautious here and any user not using psalm is able to send any types they want, I think it's fair to keep this around unless we want to accept potential fatal errors when users abuse this.

GromNaN · 2024-01-23T10:50:04Z

src/Builder/Encoder/PipelineEncoder.php

+ return $value instanceof Pipeline;
+ }
+
+ public function encode(mixed $value): stdClass|array|string


This returns always a list.

Suggested change

public function encode(mixed $value): stdClass|array|string

public function encode(mixed $value): array

In this case, would list be preferable?

I changed the return type to array, and the resulting type to list<array|stdClass|string> as list itself wasn't sufficient. Since we don't have BuilderEncoder::encode typed any more strictly, this is the best we can do.

Correction: it's list<mixed>, as encodeIfSupported is used throughout the encoders, which results in an effective return type of mixed whenever we defer to the BuilderEncoder class.

GromNaN · 2024-01-23T10:51:46Z

src/Builder/Encoder/PipelineEncoder.php

+
+ public function encode(mixed $value): stdClass|array|string
+ {
+ if (! $this->canEncode($value)) {


We need a method encodeIfSupported to avoid repeating this 3 lines in all encoders.

Isn't encodeIfSupported() already provided via the EncodeIfSupported trait above? The trait implementation utilizes the canEncode() and encode() methods.

I understand that doesn't address the repetition of checking canEncode() in each encode() method, though.

GromNaN · 2024-01-23T10:56:23Z

src/Builder/BuilderEncoder.php

+ $encoder = $this->getEncoderFor($value);

- return $result;
+ return $encoder !== null && $encoder->canEncode($value);


There is syntax for that:

return (bool) $this->getEncoderFor($value)?->canEncode($value);

Updated, along with a later check.

GromNaN · 2024-01-23T10:57:45Z

src/Builder/BuilderEncoder.php

- $result->{$key} = $this->encodeIfSupported($value);
- }
+ if (! $encoder || ! $encoder->canEncode($value)) {
+ throw UnsupportedValueException::invalidEncodableValue($value);


The $encoder->canEncode($value) part is already checked inside $encoder->encode($value).

Yes, however there is no guarantee that somebody will never call encode with an invalid value, as the contract does not require calling canEncode for a value beforehand.

We could solve this with the introduction of a null type for the $value argument in the Encoder::encode definition, but that would require us to always accept null values in any encoder, which might still necessitate calling canEncode. void as PHP's true bottom type is unfortunately not available as an argument type, so there's no proper typing way around this.

src/Builder/Type/QueryObject.php

jmikola · 2024-01-23T15:49:11Z

src/Builder/Encoder/PipelineEncoder.php

+ return $value instanceof Pipeline;
+ }
+
+ public function encode(mixed $value): stdClass|array|string


In this case, would list be preferable?

jmikola · 2024-01-23T15:51:01Z

src/Builder/Encoder/PipelineEncoder.php

+
+ public function encode(mixed $value): stdClass|array|string
+ {
+ if (! $this->canEncode($value)) {


Isn't encodeIfSupported() already provided via the EncodeIfSupported trait above? The trait implementation utilizes the canEncode() and encode() methods.

I understand that doesn't address the repetition of checking canEncode() in each encode() method, though.

jmikola · 2024-01-23T15:58:21Z

psalm-baseline.xml

+ <code>$val</code>
+ <code>$val</code>
+ </MixedAssignment>
+ <UndefinedConstant>


According to Object Interfaces: Constants, PHP 8.1 allows overriding constants. Is there any reason Psalm cannot support this?

Or does it stem from OperatorInterface not actually defining the constant, and it just being a convention in this library for implementing classes to do so? If so, perhaps we need to incorporate a new method for the encoding directive?

src/Builder/BuilderEncoder.php

src/Builder/Encoder/OperatorEncoder.php

jmikola · 2024-01-23T16:34:23Z

src/Builder/Encoder/OperatorEncoder.php

+ $object = new stdClass();
+ $object->{$value->getOperator()} = $result;
+
+ return $object;


Do we want a one-liner for this?

Suggested change

$object = new stdClass();

$object->{$value->getOperator()} = $result;

return $object;

return (object) [$value->getOperator() => $result];

I'd like to microbench both implementations.

Deferring to @GromNaN for this

Since I was already microbenching array_key_exists, I also checked this. There's no significant performance difference (30 ms when testing 1M object creations), so there's no performance benefit in using either.

src/Builder/Encoder/QueryEncoder.php

jmikola · 2024-01-23T16:39:27Z

src/Builder/Encoder/VariableEncoder.php

+ throw UnsupportedValueException::invalidEncodableValue($value);
+ }
+
+ return '$$' . $value->name;


In FieldPathEncoder you have the following:

// TODO: needs method because of interface

Is this also affected? If so, can we cover both in a single JIRA ticket?

@alcaeus: ☝️

Tracked in PHPLIB-1379.

jmikola

LGTM, but not my outstanding question about TODO comments.

jmikola · 2024-01-26T18:58:46Z

src/Builder/Type/OperatorInterface.php

@@ -9,5 +9,8 @@
 */
 interface OperatorInterface
 {
+ /** To be overridden by implementing classes */
+ public const ENCODE = Encode::Single;


Noted that you're defaulting to Single here instead of introducing an invalid case. My thinking was the invalid case would make it easier to catch mistakes when failing to override this, since Single would just happen to work (at least for encoding) provided the class only had a single property.

You can add a Undefined case to the enum. We could also use null, but the const may be typed one day.

I'm personally a fan of Undefined but won't push for it.

Introduced Undefined value to the enum. Agree with not abusing null for this to keep the constant strictly typed.

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

alcaeus requested review from jmikola and GromNaN January 23, 2024 10:45

alcaeus commented Jan 23, 2024

View reviewed changes

GromNaN reviewed Jan 23, 2024

View reviewed changes

jmikola reviewed Jan 23, 2024

View reviewed changes

alcaeus force-pushed the phplib-1250-split-encoders branch 2 times, most recently from b5dba67 to f65ba10 Compare January 26, 2024 08:46

alcaeus requested review from jmikola and GromNaN January 26, 2024 08:47

alcaeus force-pushed the phplib-1250-split-encoders branch from f65ba10 to 75da7ee Compare January 26, 2024 08:48

GromNaN approved these changes Jan 26, 2024

View reviewed changes

jmikola approved these changes Jan 26, 2024

View reviewed changes

alcaeus and others added 17 commits January 29, 2024 08:49

Split encoders into multiple files

8dcc886

Remove unnecessary factory method

02bfae8

Add missing template annotation

54d0aab

Add MixedArgument and MixedAssignment to psalm baseline

9444978

Fix type issues in query classes

8c11b8c

Extract common builder code and fix template annotations

9192f90

Update psalm baseline

6297c92

Define ENCODE constant in OperatorInterface

81cb397

Simplify canEncode check in BuilderEncoder

a913b72

Use strict comparison

5ce111a

Fix checkstyle errors

254cc4a

Fix wrong object type check

5795818

Update todo for properties in interfaces

ba2ac3c

Fix return type of PipelineEncoder::encode

a9b02b2

Update comment in OperatorEncoder

24ae4d3

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

Optimise cached encoders structure

ab93a1e

Add undefined case for Encode enum

c49a256

alcaeus force-pushed the phplib-1250-split-encoders branch from 6dfae19 to c49a256 Compare January 29, 2024 07:49

alcaeus merged commit 397ff5d into mongodb:0.1 Jan 29, 2024
5 checks passed

alcaeus deleted the phplib-1250-split-encoders branch January 29, 2024 07:59

GromNaN mentioned this pull request Jan 29, 2024

PHPLIB-1363 PHPLIB-1355 Add enum for $type query and $meta expression #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1250 Split encoders and fix psalm issues #46

PHPLIB-1250 Split encoders and fix psalm issues #46

alcaeus commented Jan 23, 2024

alcaeus Jan 23, 2024

alcaeus Jan 23, 2024

alcaeus Jan 23, 2024

jmikola Jan 23, 2024

alcaeus Jan 25, 2024

alcaeus Jan 23, 2024

alcaeus Jan 23, 2024

alcaeus Jan 23, 2024

GromNaN Jan 23, 2024

jmikola Jan 23, 2024

alcaeus Jan 25, 2024

alcaeus Jan 26, 2024

GromNaN Jan 23, 2024

jmikola Jan 23, 2024

GromNaN Jan 23, 2024

alcaeus Jan 25, 2024

GromNaN Jan 23, 2024

alcaeus Jan 25, 2024

jmikola Jan 23, 2024

jmikola Jan 23, 2024

jmikola Jan 23, 2024

jmikola Jan 23, 2024

GromNaN Jan 24, 2024

alcaeus Jan 25, 2024

alcaeus Jan 26, 2024

jmikola Jan 23, 2024

jmikola Jan 26, 2024

alcaeus Jan 29, 2024 •

edited by jmikola

Loading

jmikola left a comment

jmikola Jan 26, 2024

GromNaN Jan 26, 2024 •

edited

Loading

jmikola Jan 27, 2024 •

edited

Loading

alcaeus Jan 29, 2024

	public function encode(mixed $value): stdClass\|array\|string
	public function encode(mixed $value): array

PHPLIB-1250 Split encoders and fix psalm issues #46

PHPLIB-1250 Split encoders and fix psalm issues #46

Conversation

alcaeus commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Jan 29, 2024 • edited by jmikola Loading

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

jmikola Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Jan 29, 2024 •

edited by jmikola

Loading

GromNaN Jan 26, 2024 •

edited

Loading

jmikola Jan 27, 2024 •

edited

Loading