Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyInfo] Support for intersection types#42098
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
69d72d0 to38d8741Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suppose we need a way to differentiate unions from intersections.
Putting a preemptive "changes requested" until e figure out what we need here.
(I've no idea yet :) )
derrabus commentedJul 27, 2021
@nicolas-grekas Have you made up your mind yet? |
nicolas-grekas commentedJul 28, 2021
Nope. Maybe@dunglas has? Otherwise can you make a proposal? I'm AFK right now 😅 |
derrabus commentedJul 28, 2021
My proposal is this PR. Our current strategy of returning an array of types does not really work well anymore if we want to distinguish union from intersection types. I think, we can find a better representation for 5.4, but for 4.4, I just want PropertyInfo to not fall apart if we encounter an intersection. |
fabpot commentedAug 26, 2021
This should probably be for 5.4 instead. |
derrabus commentedAug 26, 2021
I tend to disagree here. The component would currently throw cryptic errors if presented with an intersection type. We can certainly argue aboutwhat it should do instead, but I don't think we should leave it as is in 4.4. |
38d8741 to2bf839dComparero0NL commentedAug 26, 2021
i think a list of types fits our semantic generally;
can we think of something that would conflict in practice? ref#37971 also :) i agree about the compat issue. |
fabpot commentedAug 26, 2021
ok, fair enough. |
Signed-off-by: Alexander M. Turek <me@derrabus.de>
2bf839d to9070047Compare| { | ||
| return [ | ||
| ['nothing',null], | ||
| ['collection', [newType(Type::BUILTIN_TYPE_OBJECT,false,'Traversable'),newType(Type::BUILTIN_TYPE_OBJECT,false,'Countable')]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i tend to believe the intersection type should be preserved as a single type;
[new Type(Type::BUILTIN_TYPE_OBJECT, false, 'Traversable&Countable')]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe on a higher branch, since this PR is motivated by the need to generate a proper message.
Follow up welcome.
nicolas-grekas commentedSep 7, 2021
Thank you@derrabus. |
Uh oh!
There was an error while loading.Please reload this page.
I was a bit unsure how to fix this one. PropertyInfo does not know about the concept of intersection types yet. With this PR, the component will behave exactly the same for an intersection type as for a union: You will get an array of all atomic types.