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

Allow static operations to have the same name as regular operations #1097

Closed
lucacasonato opened this issue Feb 7, 2022 · 10 comments · Fixed by #1098
Closed

Allow static operations to have the same name as regular operations #1097

lucacasonato opened this issue Feb 7, 2022 · 10 comments · Fixed by #1098

Comments

@lucacasonato
Copy link
Member

WebIDL currently specifies that:

The identifier of a static operation also must not be the same as the identifier of a regular operation defined on the same interface.

In light of whatwg/fetch#1389, I'd like to remove this limitation. The limitation was originally added in 082298c by @heycam. Does anyone remember why?

Is anyone opposed to this change?

@annevk
Copy link
Member

annevk commented Feb 7, 2022

Nothing in https://lists.w3.org/Archives/Public/public-script-coord/2013OctDec/ looks relevant.

Is there precedent in JS? @codehag, do you know?

Overall though this seems fine to me. Static members are distinct enough to not cause confusion I think.

@codehag
Copy link

codehag commented Feb 7, 2022

With regards to static methods sharing a name with an instance method of a builtin -- as far as I know, no. But for adding an options bag to an existing static method, I believe we have done that and we have definitely discussed doing it, for example for map.emplace, which could instead be map.get(key[, options]).

@domenic
Copy link
Member

domenic commented Feb 7, 2022

/cc @yuki3

@yuki3
Copy link

yuki3 commented Feb 8, 2022

I have a few concerns.

(1) Web IDL was originally (and hopefully have been) designed to be language-neutral (to support more language bindings than just an ECMAScript binding only). ECMAScript is fine with this proposal, but what about other languages? I understand that many people (or almost all people) think that it's unlikely that Web IDL will support other language bindings, but it's technically possible in the future, I think. And some programming languages wouldn't be okay with this proposal.

(2) There are tons of JavaScript programming beginners. Is this proposal helpful or rather confusing for those beginners? Especially in raw JavaScript which is type-less, could this proposal be error-prone and/or making debugging harder?

I don't know very much about the background. Why do we want this change in the first place?

By the way, thank you Domenic for notifying me on this topic.

@annevk
Copy link
Member

annevk commented Feb 8, 2022

Response objects have a json() instance method to read its response body and parse it into a JSON object. There's a proposal to add a static method to make it more easy to create Response objects that have a JSON response body. Given the names of current static methods on Response objects that exist for the creation of Response objects, json() would be a good name there too.

In usage I'm not sure this ends up being confusing, but I suppose it could be depending on how familiar the reader is with static methods. Response.json() is for creation, responseInstance.json() is for reading.


I think @yuki3 has found the reason this was likely removed. Java and Rust don't support this. I couldn't find anything conclusive on C++. I doubt this is a problem for Wasm. @lukewagner?

Nowadays I'm not sure that is of concern, except for possibly Wasm. I think we're more aligned on the idea that Web IDL is for the web and non-web languages would be better of with APIs suited to their design.


So the trade-off here is "most logical name" vs "potential confusion" and "potential implementation inconvenience" (as implementations do want to use Web IDL for non-web languages for internal usage so they need some kind of mapping).

Unless there is "potential confusion" I'd argue "most logical name" wins, but there might be arguments I've missed.

@lucacasonato
Copy link
Member Author

@annevk You mention Rust: https://crates.io/crates/web-sys would definitely have to deal with this change. Because of that, what are your thoughts on this @alexcrichton? Do you have ideas on how this could be dealt with in web-sys?

Regarding the trade-off, I agree that "potential implementation inconvenience" all the way at the bottom of the list. "most logical name" definitely goes first.

@yuki3
Copy link

yuki3 commented Feb 8, 2022

Just in case that my point was misunderstood, my main concern is about the possibilities that in the future we will have "Java binding", "Rust binding", etc. in addition to the currently-existing "ECMAScript binding" in Web IDL, rather than implementation languages. The current Web IDL supports ECMAScript binding only, but we can have more language binding support in the future.

Response.json() is for creation, responseInstance.json() is for reading.

To me, Response.createJson for example looks as much "logical" as json does. There must be a lot of other names with the same amount of "logical". I personally do not like taking a risk just for this naming nor being unfair to non-JS languages.

@alexcrichton
Copy link

My experience working on web-sys, WebIDL bindings for the web for Rust-compiled-to-wasm, is that naming is generally not that big of a concern. There's already a pretty large difference between WebIDL and what Rust can do. For example Rust doesn't have optional arguments. This means that something like new DomPoint(), to pick a random example from the docs, has lots of constructors to expose all the various variants to Rust.

Another example is that WebIDL doesn't have a concept of ownership or mutability. This means that Rust ends up needing an allow-list for methods that don't actually mutate their arguments so we can bind them in Rust as something like &[u8] instead of &mut [u8] (as we need to otherwise pessimistically assume that JS will modify its arguments). Now that being said I haven't kept up with WebIDL in some time now so this may have changed in the intervening years!

Overall it feels like 95% of the bindings translation from WebIDL to Rust is generally handling semantics in WebIDL that are bridging JS and Rust semantics. Naming is a pretty small portion of that so while this is something that we'd have to account for it's not really any worse or better than things like ChannelMergerNode::remove_event_listener_with_event_listener_and_event_listener_options, and even that's pretty tame compared to some of the other generated methods.

@domenic
Copy link
Member

domenic commented Feb 8, 2022

I think also that if we have future Java bindings or something, it'd be fine for exceptional cases like this to be exposed to Java as Response.static_json() or something. I.e., making hypothetical future Java users pay a small prince in ugliness, seems like a good tradeoff in exchange for making the API more intuitive for current JavaScript users.

In other words, (a) I don't think this makes other language bindings impossible; (b) I think it's fine, and in fact a good idea, to be unfair to other-language bindings, since JavaScript is very intertwined with Web IDL already.

@yuki3
Copy link

yuki3 commented Feb 8, 2022

Thanks for the comments. Then, I have no objection.

EdgarChen pushed a commit that referenced this issue May 10, 2022
tidoust added a commit to w3c/webref that referenced this issue May 18, 2022
See whatwg/webidl#1097 for context.

This update adds a final check before reporting duplicated members to make
sure that the members are not a static and a regular operation with the
same identifier (now allowed in Web IDL).

This is needed to account for the recent IDL update in the Fetch API:
whatwg/fetch@b3bfd0c
tidoust added a commit to w3c/webref that referenced this issue May 19, 2022
See whatwg/webidl#1097 for context.

This update adds a final check before reporting duplicated members to make
sure that the members are not a static and a regular operation with the
same identifier (now allowed in Web IDL).

This is needed to account for the recent IDL update in the Fetch API:
whatwg/fetch@b3bfd0c
webkit-commit-queue pushed a commit to andreubotella/WebKit that referenced this issue Mar 9, 2023
https://bugs.webkit.org/show_bug.cgi?id=239672

Reviewed by Darin Adler and Chris Dumez.

WebIDL used to disallow static operations from having the same name as
a regular operation of the same interface. WebKit never included this
as a check in the parser or code generator; instead, the code
generator would treat all operations with the same name as part of the
same overload set, and would produce the wrong generated code.

However, WebIDL changed to allow static and regular operations with
the same name in whatwg/webidl#1097, which
would not share an overload set. This change implements this in
WebKit.

* Source/WebCore/bindings/scripts/CodeGenerator.pm:
(LinkOverloadedOperations):
* Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:
(WebCore::JSTestObjDOMConstructor::construct):
(WebCore::jsTestObjPrototypeFunction_instanceAndStaticMethodWithTheSameIdentifierBody):
(WebCore::JSC_DEFINE_HOST_FUNCTION):
(WebCore::jsTestObjConstructorFunction_instanceAndStaticMethodWithTheSameIdentifierBody):
* Source/WebCore/bindings/scripts/test/TestObj.idl:

Canonical link: https://commits.webkit.org/261472@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

6 participants