Skip to content

Remove mic-drop hack from if analysis#7484

Merged
muglug merged 9 commits intomasterfrom
muglug-remove-mic-drop
Jan 28, 2022
Merged

Remove mic-drop hack from if analysis#7484
muglug merged 9 commits intomasterfrom
muglug-remove-mic-drop

Conversation

@muglug
Copy link
Copy Markdown
Collaborator

@muglug muglug commented Jan 25, 2022

Handling of if conditions had a path I called "mic drop" that dealt with the following code

if (<expr>) {
  <some code>
  return;
} // no elseif or else

While a useful shortcut, this created a bunch of special cases that don't work if an empty else {} is appended to the if condition — many examples in tests/TypeReconciliation/TypeAlgebraTest.php fail when that empty else {} is used.

This PR contains the necessary fixes to make most everything work as expected without the mic drop hack.

@muglug
Copy link
Copy Markdown
Collaborator Author

muglug commented Jan 25, 2022

The endtoend test failure with PHPUnit is, I think, a true positive. I'll update the baseline in that repo.

@muglug muglug 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 25, 2022
orklah
orklah reviewed Jan 25, 2022
View reviewed changes
Comment thread tests/Loop/DoTest.php
break;
}

/** @psalm-suppress MixedArgument */
Copy link
Copy Markdown
Collaborator

@orklah orklah Jan 25, 2022

Choose a reason for hiding this comment

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

It makes $a mixed here?

It seemed correctly inferred to hello before

Copy link
Copy Markdown
Collaborator Author

@muglug muglug Jan 25, 2022

Choose a reason for hiding this comment

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

Yeah, unfortunately that was dodgy too: https://psalm.dev/r/a1a5253dc3. Since assignments in conditionals are a known pain, I thought this was a reasonable compromise (there are many assignment in conditional tests, and this was the only one affected).

Copy link
Copy Markdown
Collaborator

@orklah orklah Jan 25, 2022

Choose a reason for hiding this comment

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

Seems reasonable

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 25, 2022

Seems great!

While you're in this code, I'd be grateful if you could leave a little comment to explain the role of the different scope and contexts objects for analyzing the if().

I tried to check this code a few times but I get lost with the if_context, outer_scope, post_if_context etc...

@muglug muglug force-pushed the muglug-remove-mic-drop branch 3 times, most recently from 45ba737 to 9461caa Compare January 26, 2022 20:07
@muglug muglug force-pushed the muglug-remove-mic-drop branch from 25c8471 to 9c6f380 Compare January 28, 2022 01:59
@muglug muglug merged commit faaf769 into master Jan 28, 2022
@muglug muglug deleted the muglug-remove-mic-drop branch January 28, 2022 23:30
@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 29, 2022

Thanks!

@orklah orklah mentioned this pull request May 17, 2022
Closed
@theodorejb theodorejb mentioned this pull request Oct 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@orklah orklah orklah left review comments

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@muglug @orklah