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.

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

Refactor Reconciler::handleLiteralEquality #7304

Merged
merged 19 commits into from Jan 14, 2022
Merged

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Jan 5, 2022

Sorry for this one, it won't be easy to review.

There is multiple goals here, all intertwined:

I wanted to keep a clear history of commits but this got out of control quickly. So I think I'll smash the majority of later commits together. I kept them for now for easier reading

I'll probably refactor the float case now that I started, to keep consistency. This has high chances of resolving some old bugs, but if I'm honest, it will probably create some (though they should be easier to fix). It will probably change some TypeDoesNotContainType into DocblockTypeContradiction or the other way around because I was more consistent with keeping the origin of the type than before.

I thought about pushing this on Psalm 5 instead, but I fear that if we do that, we'll have to move #7286, #7287 and #7288 too and I'm not sure if we should or if it's easy to do.

(My goal when I started those changes was to add Reconciler::RECONCILIATION_* in @param-out. If I knew I'd have to do all that, I may not have started!)

@weirdan weirdan self-requested a review January 5, 2022 17:58
@orklah
Copy link
Collaborator Author

orklah commented Jan 9, 2022

Note: the commit 72fd3de on master will most certainly be a conflict with this PR. It should not be too bad to resolve I think

@muglug
Copy link
Collaborator

muglug commented Jan 11, 2022

From a quick skim this all looks good. In my Rust-based fork I took the time to change string-based assertions to an enum (which in Psalm could be done with regular class inheritance)

pub enum Assertion {
    Any, // used for the null state
    IsType(TAtomic),
    IsNotType(TAtomic),
    Falsy,
    Truthy,
    IsEqual(TAtomic),
    IsNotEqual(TAtomic),
    IsEqualIsset,
    IsIsset,
    IsNotIsset,
    HasStringArrayAccess,
    HasIntOrStringArrayAccess,
    ArrayKeyExists,
    ArrayKeyDoesNotExist,
    InArray(TUnion),
    NotInArray(TUnion),
    HasArrayKey(String),
    NonEmptyCountable(bool),
    EmptyCountable,
    HasExactCount(usize),
    DoesNotHaveExactCount(usize),
}

Psalm's version would need a couple more of these like IsLooseEqual(..) and IsNotLooseEqual(..) to support things that are no longer supported in Hack, but it makes the logic around unpacking assertions quite a bit more clean, and vastly reduces string manipulation.

@orklah orklah added release:fix The PR will be included in 'Fixes' section of the release notes release:internal The PR will be included in 'Internal changes' section of the release notes labels Jan 14, 2022
@orklah orklah merged commit 93fe3e8 into vimeo:4.x Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants