Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for DNF types #159

Merged
merged 24 commits into from
Dec 8, 2022
Merged

Add support for DNF types #159

merged 24 commits into from
Dec 8, 2022

Conversation

IonBazan
Copy link
Contributor

@IonBazan IonBazan commented Nov 21, 2022 •
edited

Q A
Documentation no
Bugfix no
BC Break no (not yet)
New Feature yes
RFC no
QA no

Description

Basic support for DNF types (as per #142)

Code review welcome, I'd like to know if this is the right direction. Collaboration is welcome too 🙏🏻 .

@Ocramius Ocramius added this to the 4.8.0 milestone Nov 22, 2022
Ocramius
Ocramius requested changes Nov 22, 2022
View reviewed changes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Let's get psalm happy, before continuing this :D

src/Generator/TypeGenerator/AtomicType.php Outdated Show resolved Hide resolved
src/Generator/TypeGenerator/BaseComplexType.php Outdated Show resolved Hide resolved
src/Generator/TypeGenerator/BaseComplexType.php Outdated Show resolved Hide resolved
test/Generator/TypeGenerator/UnionTypeTest.php Outdated Show resolved Hide resolved
@IonBazan IonBazan force-pushed the feature/dnf branch 6 times, most recently from a6fdf0c to 6f0e7ee Compare November 23, 2022 09:18
@IonBazan IonBazan marked this pull request as ready for review November 23, 2022 09:20
@IonBazan IonBazan changed the title Add basic DNF type objects Add support for DNF types Nov 23, 2022
@IonBazan
Copy link
Contributor Author

IonBazan commented Nov 23, 2022 •
edited

The tests are passing on both PHP 8.1 and 8.2. I added some more test cases too.

Not sure if you want to keep CompositeTypeTest as it overlaps with what TypeGeneratorTest checks but it's definitely much easier to debug things having a separate test.

There is a lot of mess and code duplication in toString() and __toString() methods - not quite sure how to handle that nicely but I'm open to suggestions. Technically I could move everything back to the TypeGenerator and allow storing TypeGenerator[] in types but I feel it would give it too many responsibilities. Besides, it doesn't have a common interface with AtomicType (and shouldn't).

Idk why is psalm crashing 🤷🏻‍♂️

Ocramius
Ocramius reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Overall looking good from the tests: implementation will need a lot of careful reading / focus :)

Otherwise nice work!

I'll check it out locally when I can, and get it shipped.

src/Generator/TypeGenerator/TypeInterface.php Outdated Show resolved Hide resolved
@Ocramius
Copy link
Member

The crash in psalm is... novel :O

I wonder if we should just try going vimeo/psalm:^5.0.0-rc1?

@IonBazan
Copy link
Contributor Author

Psalm v5 crashes there too so no solution for that so far 😝

@Ocramius Ocramius mentioned this pull request Nov 25, 2022
Merged
@IonBazan IonBazan force-pushed the feature/dnf branch 4 times, most recently from 1fbc23a to 49d964c Compare November 28, 2022 13:43
@IonBazan
Copy link
Contributor Author

IonBazan commented Nov 28, 2022 •
edited

Well, Psalm is kinda fixed now - produces many more errors but at least the analysis completes. You may want to add them to baseline later.
image

Locked at dev-master as v5.0.0-rc1 does not allow PHP 8.2.

@Ocramius Ocramius assigned Ocramius and unassigned IonBazan Dec 7, 2022
IonBazan and others added 4 commits December 7, 2022 16:50
@IonBazan @Ocramius
Add better validation for parenthesis
3aa1243 
Signed-off-by: Ion Bazan <ion.bazan@gmail.com>
@IonBazan @Ocramius
Add more docs
750e81d 
Signed-off-by: Ion Bazan <ion.bazan@gmail.com>
@IonBazan @Ocramius
fix psalm
ad794d0 
Signed-off-by: Ion Bazan <ion.bazan@gmail.com>
@Ocramius
Upgrading to vimeo/psalm:5.1.0, which no longer crashes when inspec…
d490d77 
…ting DNF types

Disjunctive Normal Form types are hard, yo!

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Copy link
Member

Ocramius commented Dec 7, 2022

@IonBazan I'm picking this up: vimeo/psalm:^5.1.0 fixed the issue with scanning DNF types, apparently :)

@IonBazan
Copy link
Contributor Author

IonBazan commented Dec 7, 2022

Great, thanks! Let me know if any help is needed.

@Ocramius
Ignore test assets from static analysis
37f505f 
This stuff is a bunch of junk: on purpose.

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Ensuring TypeGenerator\TypeInterface#__toString() is pure and `non-…
7c57401 
…empty-string`

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Making CompositeType::fromString() pure, as it doesn't lead to side…
eb6f0c5 
… effects

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
CompositeTypes#getTypes() always retrieves a non-empty-list
c1a05a7 
Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Documenting CompositeTypes#getSeparator() so non-empty-string is …
47d09c8 
…always guaranteed to be generated

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Adding template types to ClassReflection
3f62095 
Since `vimeo/psalm:^5`, inheriting from a class that declares
generic types also does require generic types to be declared
while extending.

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Re-generating psalm baseline with vimeo/psalm:^5 and provided confi…
750aa25 
…g and type improvements

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
Ocramius
Ocramius reviewed Dec 7, 2022
View reviewed changes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Still working on it - got stuck on paid work :P

src/Generator/TypeGenerator/AtomicType.php Outdated Show resolved Hide resolved
src/Generator/TypeGenerator/CompositeType.php Outdated Show resolved Hide resolved
src/Generator/TypeGenerator/CompositeType.php Outdated Show resolved Hide resolved
src/Generator/TypeGenerator/CompositeType.php Outdated Show resolved Hide resolved
@Ocramius
Ensuring string casts lead to non-empty-string in TypeGenerator i…
ec2cf52 
…nternals

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Declaring a concept of IntersectionType containing sorted `AtomicTy…
e2ad144 
…pe` instances

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Declaring a concept of UnionType containing sorted AtomicType and…
7dcbb4d 
… `IntersectionType` instances

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Enforce consistency of intersected types, simplified sorting accordingly
0a1445d 
Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Noting how IntersectionType#$types is sorted
56d7de0 
Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Better abstraction around how union and intersection types are valida…
ffbd004 
…ted internally

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Rewrote CompositeType and TypeGenerator to leverage the new union…
aa62c5f 
…/intersection type abstractions

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Rewrote TypeGenerator to not convert to string and back for each type
83916dd 
This massively reduces the internal complexity
of the `TypeGenerator`, since we convert
`ReflectionType` symbols **directly** to our `@internal`
data structures, ready to be rendered.

Before this, we would cast the whole `ReflectionType` to a `string` that
fits our need, and then we would go back to parsing that string, with
a substantial overhead (especially considering the newly introduced
validation rules around DNF types).

Note that this introduces new Psalm violations that were added to `psalm-baseline.xml`,
but which are solved by my work @ vimeo/psalm#8722

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Applied CS fixes (spacing, imported symbols, docblock structure)
9cd7b31 
Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2022

@IonBazan as you can see, I ended up using a radically different representation of internal types, mostly so that it can be understood at runtime.

Shipping this now :)

@Ocramius
Disabling CS rule enforcing Abstract prefix on abstract classes: …
e22449c 
…not useful in this codebase

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
@Ocramius Ocramius merged commit 27f73dd into laminas:4.8.x Dec 8, 2022
@Ocramius Ocramius mentioned this pull request Dec 8, 2022
Closed
1 task
@IonBazan IonBazan deleted the feature/dnf branch December 8, 2022 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ocramius Ocramius Ocramius requested changes

Assignees

@Ocramius Ocramius

Labels
Enhancement
Projects
None yet
Milestone
4.8.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@IonBazan @Ocramius