Skip to content

Extract wildcard GADT constraints more directly #14832

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
Merged
abgruszecki merged 2 commits into from
Apr 9, 2022
Merged

Extract wildcard GADT constraints more directly #14832

abgruszecki merged 2 commits into from
Apr 9, 2022

Conversation

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Apr 3, 2022

No description provided.

@dwijnand dwijnand marked this pull request as ready for review April 3, 2022 15:34
@dwijnand dwijnand requested a review from abgruszecki April 3, 2022 15:34
@dwijnand
Copy link
Member Author

dwijnand commented Apr 3, 2022

I'm not sure how paranoid to be about things. For instance, whether to still special case argP being a TypeBounds. Personally I would do it this way, to avoid the noise and allow for the discovery of cases that need handling. But I'm happy to add the if back, returning true.

abgruszecki
abgruszecki reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

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

The code change looks good, but I'm a bit worried about not having an additional test. Do we have tests both for argS and argP being TypeBounds?

@dwijnand
Copy link
Member Author

dwijnand commented Apr 4, 2022 •
edited
Loading

Well the test case from #14132 is still there, and exercises the argS side. For argP I'm not sure. Something like case _: Box[_ <: String] => isn't going to work anyways, but is it something that could cause a problem? I guess I could give it a try and see if I can see it add bad constraints.

@dwijnand dwijnand assigned dwijnand and unassigned abgruszecki Apr 4, 2022
@dwijnand
Add a test case there the pattern has a bound
9bc8c9b 
Looking at the log of constr, gadts, and gadtsConstr there seems to be
no change in behaviour compared to before the implementation change in
PatternTypeConstrainer.
@dwijnand dwijnand assigned abgruszecki and unassigned dwijnand Apr 7, 2022
@abgruszecki abgruszecki merged commit ebe3d54 into scala:main Apr 9, 2022
@dwijnand dwijnand deleted the extract-wildcard-gadt-constraints branch April 9, 2022 14:43
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@abgruszecki abgruszecki abgruszecki approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@abgruszecki abgruszecki

Labels

None yet

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@dwijnand @abgruszecki @Kordyjan