Generate getters for all WebIDL dictionary types and deprecate builder-pattern style setters #3993

daxpedda · 2024-06-19T16:12:12Z

This introduces getters for all WebIDL dictionary types with the signature get_field(&self) -> Option<Type>.
This is unconventional for Rust as usually getters are called just field() and setters are called set_field(), unfortunately we can only do this in the future with a breaking change.

I also went ahead and deprecated the builder-pattern style setters in favor of exposing our currently already generated setters.

Replaces #3933.
Fixes #1793.

crates/webidl/src/util.rs

Liamolucko · 2024-06-23T01:50:21Z

crates/webidl/src/lib.rs

+        let return_ty =
+            optional_return_ty(idl_type.to_syn_type(TypePosition::Return).unwrap().unwrap());


Is it really necessary for all getters to return Option, even those for required fields? #3937 was only talking about fields with default values, required fields should be fine since their values have to be passed into the dictionary's new function. For example, AudioBufferOptions::new requires length and sample_rate to be passed in unconditionally due to them being required.

On a semi-related note, it seems like required/optional fields aren't annotated for returned dictionaries, which means that we'll end up with their getters returning Option even if the fields are always guaranteed to be present. From https://webidl.spec.whatwg.org/#idl-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.

I think what the spec means here is that it doesn't introduce any extra info beyond what's already in the spec text: it's just a spec-level assertion, unlike marking fields required on dictionaries passed as arguments which causes a runtime check that those fields are present.

But it's annoying for us because it means there's no machine-readable way to tell, similar to [Throws]. I'm not sure if it'd be better to add a similar annotation here or just let everything be optional.

I think another part of the reason is because of dictionaries that are used in both argument and return positions, where fields might be optional when used as an argument but required as a return value. And there are instances of this, e.g. MediaStreamContraints uses fields with default values, which effectively means that they're optional when passed as an argument but required when returned.

So in scenarios like that the getters would have to return Option. (Note that web-sys only contains 5 dictionaries that are used as both arguments and return values right now: MediaStreamConstraints, MediaTrackConstraints, RTCConfiguration, RTCRtpParameters, and VideoColorSpaceInit.)

But it's annoying for us because it means there's no machine-readable way to tell, similar to [Throws]. I'm not sure if it'd be better to add a similar annotation here or just let everything be optional.

I think it would be nice to add an annotation for these use-cases, but we would have to do it now because it would be a breaking change later. Considering you already looked at this, do you think its feasible to quickly identify those dictionaries and mark them correctly or is it too big of a task?

And there are instances of this, e.g. MediaStreamContraints uses fields with default values, which effectively means that they're optional when passed as an argument but required when returned.

Considering that we don't have two different types, one for use as an argument and a different one for use as a return value, I agree that in this case it just has to return Option.

I will go ahead and at least let required fields not return Option.

I will go ahead and at least let required fields not return Option.

Done.

I think it would be nice to add an annotation for these use-cases, but we would have to do it now because it would be a breaking change later. Considering you already looked at this, do you think its feasible to quickly identify those dictionaries and mark them correctly or is it too big of a task?

There aren't too many dictionaries that are used as return types, so I think it should be feasible; this is the full list:

AuthenticationExtensionsClientOutputs

ComputedEffectTiming

DOMQuadJSON

DisplayNameResult

EffectTiming

FontFaceSetIteratorResult

LocaleInfo

MediaKeySystemConfiguration

MediaStreamConstraints

MediaTrackCapabilities

MediaTrackConstraints

MediaTrackSettings

MediaTrackSupportedConstraints

PushSubscriptionJSON

RTCConfiguration

RTCRtpParameters

SerialPortInfo

TreeCellInfo

VideoColorSpaceInit

I generated this by just adding some logging to `wasm-bindgen-webidl`:

diff --git a/crates/webidl/src/util.rs b/crates/webidl/src/util.rs index 3e8cce3e4..94dddd51c 100644 --- a/crates/webidl/src/util.rs +++ b/crates/webidl/src/util.rs @@ -330,6 +330,9 @@ impl<'src> FirstPassRecord<'src> { // do this much earlier to avoid all the above work if // possible. let ret_ty = signature.orig.ret.to_idl_type(self); + if let IdlType::Dictionary(name) = ret_ty { + println!("{name} used in return position") + } let mut rust_name = snake_case_ident(name); let mut first = true;

Done!

Also deprecated/removed their constructors, e.g. otherwise you can just write EffectTiming::new().get_fill() and it would fail.

Actually, I just realized, that there should be a ton of dictionaries returned from readonly attributes that would fall into this category as well. Additionally, dictionaries can be wrapped in Promise or sequence that we didn't consider here as well.

Ideally we should programmatically detect dictionaries that are never used as a parameter?

In any case, I'm going to revisit this again tomorrow.

Just saw in CI that we can't deprecate EffectTiming::default(), so that's also less then ideal.

EDIT: Just realized this was very unclear. I wanted to say that we can't deprecated the Default implementation because deprecating trait implementations is not supported by Rust. See rust-lang/rust#39935.

Also deprecated/removed their constructors, e.g. otherwise you can just write EffectTiming::new().get_fill() and it would fail.

In the long-term, it might make sense to take all the fields as arguments to new the same as regular required fields. For the moment though, I'm not sure if there's enough need to construct return-only dictionaries to warrant adding a second constructor (which would have to be named something awkward like new2), and we could always add it later.

Actually, I just realized, that there should be a ton of dictionaries returned from readonly attributes that would fall into this category as well. Additionally, dictionaries can be wrapped in Promise or sequence that we didn't consider here as well.

Good point, I didn't think of that.

Ideally we should programmatically detect dictionaries that are never used as a parameter?

Yeah, that would be better.

See daxpedda@0d4bd7e.
I believe that should be pretty accurate from what we can extract from the WebIDL we have.

List of Dictionaries

DOMQuadJSON EcdsaParams ClientRectsAndTexts WebTransportDatagramStats DecoderDoctorNotification IntersectionObserverEntryInit DhImportKeyParams RTCMediaStreamStats PermissionDescriptor Transformer RTCIdentityAssertion LockManagerSnapshot ConsoleCounterError BasePropertyIndexedKeyframe FontFaceSetIteratorResult CollectedClientData NetworkResultOptions FakePluginTagInit RTCIceComponentStats ReadableStreamReadResult Pbkdf2Params MediaTrackSettings ConsoleTimerStart RsaOtherPrimesInfo HIDCollectionInfo DOMWindowResizeEventDetail ProfileTimelineMarker DhKeyGenParams ImageDecodeResult RsaHashedKeyGenParams SerialPortInfo NavigatorUABrandVersion MemoryMeasurement RequestMediaKeySystemAccessNotification CheckerboardReport LocaleInfo RsaPssParams VideoDecoderSupport TreeCellInfo AesCbcParams RsaHashedImportParams MemoryAttributionContainer IterableKeyOrValueResult ConsoleProfileEvent RTCIceCandidateStats MediaTrackSupportedConstraints DNSCacheDict FetchReadableStreamReadDataArray RTCRTPStreamStats BasicCardRequest MemoryBreakdownEntry WidevineCDMManifest RTCRTPContributingSourceStats XRPermissionDescriptor DoubleRange KeyIdsInitData ConnStatusDict VideoEncoderSupport DisplayNameResult ProfileTimelineLayerRect RsaHashedKeyAlgorithm RTCRtpCapabilities ConsoleCounter SocketElement WebTransportStats NativeOSFileWriteAtomicOptions AesCtrParams BasicCardResponse HttpConnDict DNSLookupDict SocketsDict BaseKeyframe LockInfo EncodedVideoChunkMetadata ProfileTimelineStackFrame SignResponse RTCCodecStats PushSubscriptionJSON BaseComputedKeyframe HIDReportInfo AesGcmParams EcKeyImportParams AudioEncoderSupport WebTransportReceiveStreamStats U2FClientData FetchReadableStreamReadDataDone ULongRange DnsCacheEntry ClipboardPermissionDescriptor UALowEntropyJSON FakePluginMimeEntry WebGLContextAttributes XRSessionSupportedPermissionDescriptor MutationObservingInfo RTCCertificateExpiration MediaTrackCapabilities RTCRtpSynchronizationSource SerialInputSignals IterableKeyAndValueResult AnimationPropertyDetails HkdfParams RTCRtpHeaderExtensionCapability JsonWebKey EncodedAudioChunkMetadata BluetoothDataFilterInit ConsoleTimerError RTCStatsReportInternal CryptoKeyPair SvcOutputMetadata AesDerivedKeyParams NetworkCommandOptions HttpConnectionElement EcKeyGenParams WebTransportSendStreamStats OpenWindowEventDetail KeyAlgorithm HttpConnInfo HmacImportParams BluetoothPermissionStorage DhKeyAlgorithm HmacKeyAlgorithm HIDReportItem LifecycleCallbacks MediaSessionActionDetails DOMQuadInit DOMMatrixInit AuthenticationExtensionsClientOutputs RsaOaepParams MemoryAttribution HalfOpenInfoDict CacheBatchOperation AesKeyAlgorithm RTCTransportStats EcKeyAlgorithm RcwnStatus RTCRtpContributingSource ConsoleTimerLogOrEnd DhKeyDeriveParams USBPermissionDescriptor ChromeFilePropertyBag EcdhKeyDeriveParams AllowedBluetoothDevice UADataValues RcwnPerfStats RTCOutboundRTPStreamStats HmacKeyGenParams RTCIceCandidatePairStats L10nElement WebSocketElement ContextAttributes2D AudioDecoderSupport BluetoothPermissionDescriptor AutocompleteInfo RegisterResponse RTCRtpSourceEntry RTCInboundRTPStreamStats ConsoleEvent ConsoleStackEntry WebSocketDict ComputedEffectTiming PushSubscriptionKeys RTCMediaStreamTrackStats USBPermissionStorage HmacDerivedKeyParams RTCStats UnderlyingSource UnderlyingSink NativeOSFileReadOptions Algorithm BlockParsingOptions FileSystemPermissionDescriptor AesKeyGenParams AnimationPropertyValueDetails StorageEstimate AllowedUSBDevice

175 entries, I found a lot of APIs there that were just copied from Firefox but aren't probably real accessible Web APIs, its still a lot to go through.
As I mentioned in #3993 (comment), I believe we should drop this improvement entirely, unfortunately.

I guess if somebody wants to do the legwork here, what needs to be done is to go through each one of those, go to the spec and find out if really all members are always present or which ones aren't so we can mark them, e.g. like in MediaTrackSettings.

crates/webidl/src/util.rs

crates/webidl/src/generator.rs

crates/web-sys/src/features/gen_BlobPropertyBag.rs

Liamolucko · 2024-06-25T23:10:54Z

I forgot to do this earlier, but cc @pablosichert @MOZGIII since you seem to be invested in this.

crates/webidl/src/generator.rs

crates/webidl/src/lib.rs

Liamolucko · 2024-07-03T05:02:16Z

crates/webidl/src/lib.rs

+        let return_ty =
+            optional_return_ty(idl_type.to_syn_type(TypePosition::Return).unwrap().unwrap());


Also deprecated/removed their constructors, e.g. otherwise you can just write EffectTiming::new().get_fill() and it would fail.

In the long-term, it might make sense to take all the fields as arguments to new the same as regular required fields. For the moment though, I'm not sure if there's enough need to construct return-only dictionaries to warrant adding a second constructor (which would have to be named something awkward like new2), and we could always add it later.

Actually, I just realized, that there should be a ton of dictionaries returned from readonly attributes that would fall into this category as well. Additionally, dictionaries can be wrapped in Promise or sequence that we didn't consider here as well.

Good point, I didn't think of that.

Ideally we should programmatically detect dictionaries that are never used as a parameter?

Yeah, that would be better.

crates/webidl/src/lib.rs

MOZGIII · 2024-07-28T21:18:28Z

Sorry for being late, but this looks great!

MOZGIII · 2024-07-28T21:19:34Z

When will this be released?

pablosichert · 2024-07-28T21:24:22Z

Agree, thanks a lot @daxpedda for getting this over the finish line.

I think you struck a good balance of at least deprecating the non-set_* methods so that they can eventually be phased out and make space for the getters.

daxpedda · 2024-07-28T21:26:53Z

When will this be released?

I'm going through my wasm-bindgen backlog right now, with luck today, otherwise tomorrow (TM).

daxpedda requested a review from Liamolucko June 19, 2024 16:23

daxpedda force-pushed the webidl-getters branch from c633cbf to 3a2de00 Compare June 19, 2024 16:24

daxpedda changed the title ~~Generate getters for all WebIDL dictionary types~~ Generate getters for all WebIDL dictionary types and deprecate builder-pattern style setters Jun 19, 2024

daxpedda commented Jun 19, 2024

View reviewed changes

crates/webidl/src/util.rs Show resolved Hide resolved

daxpedda force-pushed the webidl-getters branch from 3a2de00 to 7208f7b Compare June 19, 2024 16:37

daxpedda pushed a commit to daxpedda/wasm-bindgen that referenced this pull request Jun 19, 2024

Generate getters for all WebIDL dictionary types (rustwasm#3993)

3f6d885

Liamolucko reviewed Jun 23, 2024

View reviewed changes

daxpedda force-pushed the webidl-getters branch 2 times, most recently from a657d4b to 27d3daf Compare June 23, 2024 08:23

daxpedda requested a review from Liamolucko June 27, 2024 08:18

daxpedda force-pushed the webidl-getters branch 2 times, most recently from 51d56ef to 0c4e8dc Compare June 27, 2024 08:37

Liamolucko reviewed Jul 3, 2024

View reviewed changes

daxpedda force-pushed the webidl-getters branch from 0c4e8dc to 586982f Compare July 7, 2024 23:44

daxpedda requested a review from Liamolucko July 7, 2024 23:44

daxpedda force-pushed the webidl-getters branch from 586982f to 270e29e Compare July 8, 2024 00:22

daxpedda force-pushed the webidl-getters branch from 270e29e to b5e9e08 Compare July 28, 2024 17:39

daxpedda added 2 commits July 28, 2024 20:20

Generate getters for all WebIDL dictionary types

028dbc9

Deprecate builder-pattern setters for WebIDL dictionaries

13ca1bf

daxpedda force-pushed the webidl-getters branch from b5e9e08 to 13ca1bf Compare July 28, 2024 18:20

daxpedda merged commit 7bdec18 into rustwasm:main Jul 28, 2024
25 checks passed

This was referenced Jul 28, 2024

Implement getters for WebIDL dictionaries #3933

Closed

Update Web Authentication API to level 3 #4000

Merged

MOZGIII mentioned this pull request Aug 6, 2024

Hand-crafted WebTransport Web API bindings MOZGIII/xwt#129

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate getters for all WebIDL dictionary types and deprecate builder-pattern style setters #3993

Generate getters for all WebIDL dictionary types and deprecate builder-pattern style setters #3993

daxpedda commented Jun 19, 2024 •

edited

Loading

Liamolucko Jun 23, 2024

daxpedda Jun 23, 2024

daxpedda Jun 23, 2024

Liamolucko Jun 25, 2024

daxpedda Jun 27, 2024

daxpedda Jun 27, 2024

daxpedda Jun 27, 2024 •

edited

Loading

Liamolucko Jul 3, 2024

daxpedda Jul 7, 2024 •

edited

Loading

Liamolucko commented Jun 25, 2024

Liamolucko Jul 3, 2024

MOZGIII commented Jul 28, 2024

MOZGIII commented Jul 28, 2024

pablosichert commented Jul 28, 2024

daxpedda commented Jul 28, 2024

		let return_ty =
		optional_return_ty(idl_type.to_syn_type(TypePosition::Return).unwrap().unwrap());

Generate getters for all WebIDL dictionary types and deprecate builder-pattern style setters #3993

Generate getters for all WebIDL dictionary types and deprecate builder-pattern style setters #3993

Conversation

daxpedda commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

Liamolucko commented Jun 25, 2024

Choose a reason for hiding this comment

MOZGIII commented Jul 28, 2024

MOZGIII commented Jul 28, 2024

pablosichert commented Jul 28, 2024

daxpedda commented Jul 28, 2024

daxpedda commented Jun 19, 2024 •

edited

Loading

daxpedda Jun 27, 2024 •

edited

Loading

daxpedda Jul 7, 2024 •

edited

Loading