From f3ec727555a2b928186063f83c4f582c3dcec287 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Tue, 14 Jan 2025 00:08:27 +1300 Subject: [PATCH] LibWeb/Bindings: Support returning nullable types in dictionaries 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. --- .../LibWeb/Animations/AnimationEffect.idl | 4 +-- Libraries/LibWeb/Encoding/TextEncoder.idl | 4 +-- Libraries/LibWeb/HTML/Navigation.idl | 4 +-- Libraries/LibWeb/URLPattern/URLPattern.idl | 22 +++++++------- Libraries/LibWeb/WebAudio/AudioContext.idl | 4 +-- .../BindingsGenerator/IDLGenerators.cpp | 29 ++++++++++++------- 6 files changed, 37 insertions(+), 30 deletions(-) diff --git a/Libraries/LibWeb/Animations/AnimationEffect.idl b/Libraries/LibWeb/Animations/AnimationEffect.idl index a93b5bf710a..e993857cdf0 100644 --- a/Libraries/LibWeb/Animations/AnimationEffect.idl +++ b/Libraries/LibWeb/Animations/AnimationEffect.idl @@ -30,8 +30,8 @@ enum PlaybackDirection { "normal", "reverse", "alternate", "alternate-reverse" } // https://www.w3.org/TR/web-animations-1/#the-computedeffecttiming-dictionary dictionary ComputedEffectTiming : EffectTiming { - unrestricted double endTime; - unrestricted double activeDuration; + [GenerateAsRequired] unrestricted double endTime; + [GenerateAsRequired] unrestricted double activeDuration; double? localTime; double? progress; unrestricted double? currentIteration; diff --git a/Libraries/LibWeb/Encoding/TextEncoder.idl b/Libraries/LibWeb/Encoding/TextEncoder.idl index f10345790db..f346601c951 100644 --- a/Libraries/LibWeb/Encoding/TextEncoder.idl +++ b/Libraries/LibWeb/Encoding/TextEncoder.idl @@ -2,8 +2,8 @@ // https://encoding.spec.whatwg.org/#dictdef-textencoderencodeintoresult dictionary TextEncoderEncodeIntoResult { - unsigned long long read; - unsigned long long written; + [GenerateAsRequired] unsigned long long read; + [GenerateAsRequired] unsigned long long written; }; // https://encoding.spec.whatwg.org/#textencoder diff --git a/Libraries/LibWeb/HTML/Navigation.idl b/Libraries/LibWeb/HTML/Navigation.idl index 3bd3a28d7f1..5bc03f1820c 100644 --- a/Libraries/LibWeb/HTML/Navigation.idl +++ b/Libraries/LibWeb/HTML/Navigation.idl @@ -45,8 +45,8 @@ dictionary NavigationReloadOptions : NavigationOptions { }; dictionary NavigationResult { - Promise committed; - Promise finished; + [GenerateAsRequired] Promise committed; + [GenerateAsRequired] Promise finished; }; enum NavigationHistoryBehavior { diff --git a/Libraries/LibWeb/URLPattern/URLPattern.idl b/Libraries/LibWeb/URLPattern/URLPattern.idl index 84ef30079b6..fa007c8ef29 100644 --- a/Libraries/LibWeb/URLPattern/URLPattern.idl +++ b/Libraries/LibWeb/URLPattern/URLPattern.idl @@ -42,20 +42,20 @@ dictionary URLPatternOptions { // https://urlpattern.spec.whatwg.org/#dictdef-urlpatternresult dictionary URLPatternResult { - sequence inputs; + [GenerateAsRequired] sequence inputs; - URLPatternComponentResult protocol; - URLPatternComponentResult username; - URLPatternComponentResult password; - URLPatternComponentResult hostname; - URLPatternComponentResult port; - URLPatternComponentResult pathname; - URLPatternComponentResult search; - URLPatternComponentResult hash; + [GenerateAsRequired] URLPatternComponentResult protocol; + [GenerateAsRequired] URLPatternComponentResult username; + [GenerateAsRequired] URLPatternComponentResult password; + [GenerateAsRequired] URLPatternComponentResult hostname; + [GenerateAsRequired] URLPatternComponentResult port; + [GenerateAsRequired] URLPatternComponentResult pathname; + [GenerateAsRequired] URLPatternComponentResult search; + [GenerateAsRequired] URLPatternComponentResult hash; }; // https://urlpattern.spec.whatwg.org/#dictdef-urlpatterncomponentresult dictionary URLPatternComponentResult { - USVString input; - record groups; + [GenerateAsRequired] USVString input; + [GenerateAsRequired] record groups; }; diff --git a/Libraries/LibWeb/WebAudio/AudioContext.idl b/Libraries/LibWeb/WebAudio/AudioContext.idl index 7402b156bdb..36858285ccb 100644 --- a/Libraries/LibWeb/WebAudio/AudioContext.idl +++ b/Libraries/LibWeb/WebAudio/AudioContext.idl @@ -25,7 +25,7 @@ dictionary AudioContextOptions { }; dictionary AudioTimestamp { - double contextTime; + [GenerateAsRequired] double contextTime; // FIXME: Should be DOMHighResTimeStamp, but DOMHighResTimeStamp doesn't get parsed as a double during codegen - double performanceTime; + [GenerateAsRequired] double performanceTime; }; diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp index 3655463f9cc..f1938be7ef3 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp @@ -1764,7 +1764,7 @@ enum class WrappingReference { Yes, }; -static void generate_wrap_statement(SourceGenerator& generator, ByteString const& value, IDL::Type const& type, IDL::Interface const& interface, StringView result_expression, WrappingReference wrapping_reference = WrappingReference::No, size_t recursion_depth = 0) +static void generate_wrap_statement(SourceGenerator& generator, ByteString const& value, IDL::Type const& type, IDL::Interface const& interface, StringView result_expression, WrappingReference wrapping_reference = WrappingReference::No, size_t recursion_depth = 0, bool is_optional = false) { auto scoped_generator = generator.fork(); scoped_generator.set("value", value); @@ -1791,7 +1791,7 @@ static void generate_wrap_statement(SourceGenerator& generator, ByteString const return; } - if (type.is_nullable() && !is(type)) { + if ((is_optional || type.is_nullable()) && !is(type)) { if (type.is_string()) { scoped_generator.append(R"~~~( if (!@value@.has_value()) { @@ -1820,7 +1820,7 @@ static void generate_wrap_statement(SourceGenerator& generator, ByteString const } if (type.is_string()) { - if (type.is_nullable()) { + if (type.is_nullable() || is_optional) { scoped_generator.append(R"~~~( @result_expression@ JS::PrimitiveString::create(vm, @value@.release_value()); )~~~"); @@ -1837,16 +1837,16 @@ static void generate_wrap_statement(SourceGenerator& generator, ByteString const auto new_array@recursion_depth@ = MUST(JS::Array::create(realm, 0)); )~~~"); - if (!type.is_nullable()) { - scoped_generator.append(R"~~~( - for (size_t i@recursion_depth@ = 0; i@recursion_depth@ < @value@.size(); ++i@recursion_depth@) { - auto& element@recursion_depth@ = @value@.at(i@recursion_depth@); -)~~~"); - } else { + if (type.is_nullable() || is_optional) { scoped_generator.append(R"~~~( auto& @value@_non_optional = @value@.value(); for (size_t i@recursion_depth@ = 0; i@recursion_depth@ < @value@_non_optional.size(); ++i@recursion_depth@) { auto& element@recursion_depth@ = @value@_non_optional.at(i@recursion_depth@); +)~~~"); + } else { + scoped_generator.append(R"~~~( + for (size_t i@recursion_depth@ = 0; i@recursion_depth@ < @value@.size(); ++i@recursion_depth@) { + auto& element@recursion_depth@ = @value@.at(i@recursion_depth@); )~~~"); } @@ -2024,7 +2024,14 @@ static void generate_wrap_statement(SourceGenerator& generator, ByteString const dictionary_generator.append(R"~~~( JS::Value @wrapped_value_name@; )~~~"); - generate_wrap_statement(dictionary_generator, ByteString::formatted("{}{}{}", value, type.is_nullable() ? "->" : ".", member.name.to_snakecase()), member.type, interface, ByteString::formatted("{} =", wrapped_value_name), WrappingReference::No, recursion_depth + 1); + // NOTE: This has similar semantics as 'required' in WebIDL. However, the spec does not put 'required' on + // _returned_ dictionary members since with the way the spec is worded it has no normative effect to + // do so. We could implement this without the 'GenerateAsRequired' extended attribute, but it would require + // the generated code to do some metaprogramming to inspect the type of the member in the C++ struct to + // determine whether the type is present or not (e.g through a has_value() on an Optional, or a null + // check on a GC::Ptr). So to save some complexity in the generator, give ourselves a hint of what to do. + bool is_optional = !member.extended_attributes.contains("GenerateAsRequired") && !member.default_value.has_value(); + generate_wrap_statement(dictionary_generator, ByteString::formatted("{}{}{}", value, type.is_nullable() ? "->" : ".", member.name.to_snakecase()), member.type, interface, ByteString::formatted("{} =", wrapped_value_name), WrappingReference::No, recursion_depth + 1, is_optional); dictionary_generator.append(R"~~~( MUST(dictionary_object@recursion_depth@->create_data_property("@member_key@", @wrapped_value_name@)); @@ -2057,7 +2064,7 @@ static void generate_wrap_statement(SourceGenerator& generator, ByteString const } } - if (type.is_nullable() && !is(type)) { + if ((type.is_nullable() || is_optional) && !is(type)) { scoped_generator.append(R"~~~( } )~~~");