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

LibURL+LibWeb: Add a stub for URLPattern.exec, along with some corresponding IDL generator fixes #3237

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

shannonbooth
Copy link
Contributor

@shannonbooth shannonbooth commented Jan 13, 2025

Sitting on top of #3224. I've split this out as I am not fully convinced on the approach for the last IDL generator fix included in this merge request in particular.

This fixes a further 4 of the ~5-6 issues with the IDL generator for the URLPattern interface, and implements enough of the URLPattern types and IDL generator fixes so that we can get the IDL for URLPattern.exec compiling. Unfortunately, there are still some further compilation issues to fix for this IDL interface, along with a case where it is returning a subtlely wrong results.

@shannonbooth shannonbooth force-pushed the urlpattern-add-some-idl branch 2 times, most recently from 832b358 to 2d50623 Compare January 13, 2025 11:40
@tcl3
Copy link
Member

tcl3 commented Jan 13, 2025

I've not looked through all of the changes yet, but looking at the change that adds required to return type dictionaries, I think it would be better if this could be done in a way that didn't require altering the IDL.

The WebIDL spec says this (from 2.7. Dictionaries):

Note that specifying dictionary members as required only has an observable effect when converting other representations of dictionaries (like a JavaScript value supplied as an argument to an operation) to an IDL dictionary. Specification authors should leave the members optional in all other cases, including when a dictionary type is used solely as the return type of operations.

@shannonbooth
Copy link
Contributor Author

shannonbooth commented Jan 13, 2025

I've not looked through all of the changes yet, but looking at the change that adds required to return type dictionaries, I think it would be better if this could be done in a way that didn't require altering the IDL.

The WebIDL spec says this (from 2.7. Dictionaries):

Note that specifying dictionary members as required only has an observable effect when converting other representations of dictionaries (like a JavaScript value supplied as an argument to an operation) to an IDL dictionary. Specification authors should leave the members optional in all other cases, including when a dictionary type is used solely as the return type of operations.

right, from the specs point of view it is not observable and is the editorial convention they chose. The alternative is to do some tricks in the generared code so that it can handle implicitly an optional argument by overloads or metaprogramming. I am just not sure that is worth the effort since if we slap on a required we can treat the dictionary member like we handle when it is an input 🤔

edit: I think the tradeoff of doing it this way is:

  • Positive: I think it should allow us to generate simpler code in IDL bindings, even if we could achieve this with no runtime cost through constexpr stuff / templates / function overloads.
  • Negative: It's kind of weird, and not exactly matching the IDL is not great. May cause some confusion in the future. It's kind of like an extended attribute.

@shannonbooth shannonbooth marked this pull request as draft January 13, 2025 20:43
@shannonbooth shannonbooth force-pushed the urlpattern-add-some-idl branch from 2d50623 to 8480b57 Compare January 13, 2025 22:17
@shannonbooth shannonbooth marked this pull request as ready for review January 13, 2025 22:17
@shannonbooth
Copy link
Contributor Author

Thinking about this some more, as an idea, a variant of reusing the 'required' keyword might be to have a [Required] extended attribute. The benefit here would be that it's a bit more obvious to a reader of a IDL interface why it's there. That still have the downside of potential confusion in future interfaces though. And like using 'required' it's still quite weird, maybe weirder.

Tradeoff to me seems to be between extra complexity in the generated code, or some type of hint in the IDL to help our implementation.

@shannonbooth shannonbooth force-pushed the urlpattern-add-some-idl branch from 8480b57 to 99b951c Compare January 15, 2025 22:27
The URLPattern spec is intended to be implemented inside of LibURL, with
LibWeb only responsible for the IDL conversion layer, in a similar
manner to how URL is implemented.
This is the return value of a URLPattern after `exec` is called on it.
It conveys information about the named (or unammed) regex groups
matched for each component of the URL. For example,

```
let p = new URLPattern({ hostname: "{:subdomain.}*example.com" });
const result = pattern.exec({ hostname: "foo.bar.example.com" });
console.log(result.hostname.groups.subdomain);
```

Will log 'foo.bar'.
This fixes a compile error of multiple variables of the same name within
the same scope for the URLPattern IDL, which has a dictionary return
type that contains multiple dictionaries of the same type. Conveniently,
this also makes the complicated generated code of the URLPattern
interface easier to read by adding some more structure :^)
@shannonbooth shannonbooth force-pushed the urlpattern-add-some-idl branch 4 times, most recently from 120c201 to 1dbbac6 Compare January 19, 2025 03:17
We were previously assuming that dictionary members were always
required when being returned.

This is a bit of a weird case, because unlike _input_ dictionaries
which the spec marks as required, 'result' dictionaries do not seem to
be marked in spec IDL as required. This is still fine from the POV that
the spec is written as it states that we should only be putting the
values into the dictionary if the value exists.

We could do this through some metaprogramming constexpr type checks.
For example, if the type in our C++ representation was not an
Optional, we can skip the has_value check.

Instead of doing that, change the IDL of the result dictionaries to
annotate these members so that the IDL generator knows this
information up front. While all current cases have every single
member returned or not returned, it is conceivable that the spec
could have a situation that one member is always returned (and
should get marked as required), while the others are optionally
returned. Therefore, this new GenerateAsRequired attribute is
applied for each individual member.
This is not a very pleasant fix, but matches a similar const_cast that
we do to return JS objects returned in a union. Ideally we would
'simply' remove the const from the value being visited in the variant,
but that opens up a whole can of worms where we are currently relying on
temporary lifetime extension so that interfaces can return a Variant of
GC::Ref's to JS::Objects.
Just enough to get the IDL compiling :^)
@shannonbooth shannonbooth force-pushed the urlpattern-add-some-idl branch from 1dbbac6 to 56ca945 Compare January 19, 2025 03:18
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 this pull request may close these issues.

2 participants