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

Promise<T> uses return-type in an inconsistent way; union types don't use type at all #537

Open
cscott opened this issue Jan 28, 2021 · 4 comments

Comments

@cscott
Copy link
Contributor

cscott commented Jan 28, 2021

Usually the rule seems to be that the top-most parent of a Type sets the type field all the way down. attribute-type, for example, gets propagated down into sequence types, etc.

However, union types have the type property set to null, and these fields are not recursed into when setting the attribute-type value or whathaveyou.

Also Promise<T> sets the type property of the T type to return-type, and again that is robust and doesn't get overwritten with attribute-type --- but only when T is void.

In my PHP port, this leads to the somewhat baroque logic in the setType method here:
https://github.com/cscott/WebIDL/blob/main/src/Grammar.pegphp#L54
which has to take care not to overwrite this single type in the Promise<void> child set here:
https://github.com/cscott/WebIDL/blob/main/src/Grammar.pegphp#L764

In this repo's test cases, you can see the unusual behavior here, for Promise<void> (which gets return-type not attribute-type):
https://github.com/w3c/webidl2.js/blob/gh-pages/test/syntax/baseline/promise-void.json#L18
and here (Promise<DOMString> gets attribute-type not return-type):
https://github.com/w3c/webidl2.js/blob/gh-pages/test/syntax/baseline/generic.json#L60

And here is where you can see a union getting null as it's type instead of inheriting attribute-type:
https://github.com/w3c/webidl2.js/blob/gh-pages/test/syntax/baseline/uniontype.json#L18

This isn't necessarily a bug per se -- my PHP port can emulate this behavior consistently, and it's only two or three lines of code -- but it does seem... "non-orthogonal"... so I thought I'd mention it in case you want to clean it up.

I'd suggest consistently recursing into type children including union types, and getting rid of the return-type marker on Promise<void> and letting that be recursed into like all other types.

@saschanaz
Copy link
Member

Thank you for raising this!

This could be fixed, but I wonder we could remove the whole type field instead. Its original purpose was to be used in syntax highlighting in ReSpec but the actual implementation there doesn't use it at all.

Pinging @sidvishnoi to confirm this.

@cscott
Copy link
Contributor Author

cscott commented Jan 28, 2021

Well, it's still useful to distinguish interface from callback interface etc. It would be ironic if Type was the only object type which didn't have a type. :)

But I think t.type = 'type' could be done consistently to tag all the type objects and you're right that we probably don't need return-type, attribute-type, etc.

@saschanaz
Copy link
Member

saschanaz commented Jan 28, 2021

Ah yes, I mean only return-type and attribute-type etc. It's redundant since there is .parent field anyway. (Might be still needed since JSON does not support circular reference but I don't know any of such use case.)

@sidvishnoi
Copy link
Member

@saschanaz Confirming ReSpec is not using return-type and attribute-type in syntax highlighting as well as xref linking.
Though other types are thoroughly used, just to be clear.

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

Successfully merging a pull request may close this issue.

3 participants