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

Parentheses in callable syntax are not interpreted correctly #74

Open
mabar opened this issue Apr 14, 2021 · 11 comments
Open

Parentheses in callable syntax are not interpreted correctly #74

mabar opened this issue Apr 14, 2021 · 11 comments

Comments

@mabar
Copy link

mabar commented Apr 14, 2021

Parentheses in callables in some cases create unexpected results, like (Closure(): int|null) changing to (Closure(): int)|null
https://phpstan.org/r/f0db1360-c01a-4884-8c17-1caa7d464852

@ondrejmirtes ondrejmirtes transferred this issue from phpstan/phpstan Apr 14, 2021
@JanTvrdik
Copy link
Collaborator

Parentheses in callable syntax are not interpreted correctly

That's nonsence. The syntax is ambigous, there cannot be a correct way to interpret it.

Maybe related #18 (it was a long time ago, so I'm not 100 % sure)

@JanTvrdik
Copy link
Collaborator

Also note that top-level parentheses always have no effect, i.e. (Closure(): int|null) will aways be identical to Closure(): int|null.
It's the same as in math, 1 equals to (1) equals to ((1))

@mabar
Copy link
Author

mabar commented Apr 14, 2021

Please check the f() and g() examples. Closure(): (int|null)|null is shown by phpstan as (Closure(): int|null)|null. If you use same syntax as is displayed by phpstan, result is (Closure(): int)|null.

I don't mind if parentheses are supported (even useless) or forbidden, but now it fails silently and phpstan is using in errors syntax which is not supported by phpstan parser.

@JanTvrdik
Copy link
Collaborator

So the issue in how we print the type?

@mabar
Copy link
Author

mabar commented Apr 14, 2021

And failing silently in case of not supported syntax.
(Closure(): int|null)|null -> (Closure(): int)|null

Ideally Closure():int|null should fail too, because it's unclear whether user wanted Closure(): (int|null) or Closure(): (int)|null

@JanTvrdik
Copy link
Collaborator

And failing silently in case of not supported syntax.

I don't understand. (Closure(): int|null)|null is supported and parsed well. What would you expect?
It's parsed as (((Closure(): int)|null)|null) and further normalized by removing the duplicated null in union type.

should fail too, because it's unclear

I think ut's allowed mostly for null|Closure():int. Anyway I think that's was #18 was supposed to solve.

@mabar
Copy link
Author

mabar commented Apr 14, 2021

I don't think so. (Closure(): int|string)|null turns into (Closure(): int)|string|null

https://phpstan.org/r/04ea77ae-c31d-4036-807a-9de7274fe15e

@JanTvrdik
Copy link
Collaborator

JanTvrdik commented Apr 14, 2021

Yes, that is correct, even though it can be confusing. What would you expect? Note that the parentheses here are useless.

(A|B)|C is the same as A|B|C and also same as (A)|B|C. Here the A is Closure(): int.

@mabar
Copy link
Author

mabar commented Apr 14, 2021

(Closure(): int|string)|null turns into (Closure(): int)|string|null

One is closure which returns int|string or null, second is closure which returns int or string or null.

In following example is expected closure which returns int|string or null and as such is the code treated. If it was the same, this code would not fail.
https://phpstan.org/r/58aa23f2-e350-4760-b8f0-2c5cc1bd66b8
http://sandbox.onlinephpfunctions.com/code/108b92d91ec897d01054c530ab461f47c0ef515a

Also thanks, just found another bug due to testing the Closure(): (int|string)|null syntax
https://phpstan.org/r/fe407cba-17cf-4f56-8a0a-07d68cb41712
http://sandbox.onlinephpfunctions.com/code/3072c54f802558ace446341a9326c44a7a37b794

@JanTvrdik
Copy link
Collaborator

One is closure which returns int|string or null

Based on what? Is that you wish? Again parsing Closure(): int|string as (Closure(): int)|string is not a bug. It's very much intentional. See

'callable(): Foo|Bar',
new UnionTypeNode([
new CallableTypeNode(
new IdentifierTypeNode('callable'),
[],
new IdentifierTypeNode('Foo')
),
new IdentifierTypeNode('Bar'),
]),
],

@simPod
Copy link

simPod commented Apr 14, 2021

I've run this through typescript

(a: (() => number)|string) => {
    if (typeof a === 'string') {
        return;
    }

    const x:never = a;
}

and it interprets (Closure(): int)|string not as Closure(): int|string but as Closure(): int or string. Isn't the intention here based on wrong assumption?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants