Parse webIDL from compiler plugin to verify inheritance of DOM objects matches IDL

[jdm]

Given this crate, we could theoretically add a compiler plugin that iterates over the types in script::dom::types, parses the WebIDL file that matches that type name, and verifies that the first field of the DOM type matches the name of the parent type of the interface, per the documentation.

[Succo]

Hi, would that be a good first task? If so I would be interested.

[jdm]

I do not believe this would be a good first task.

[KiChjang]

@jdm How much additional time would be spent on compilation if we perform this check?

[jdm]

No idea!

[KiChjang]

Hmm, so my concern is that we already have CodegenRust.py that runs a python lexer and a parser in order to generate WebIDL bindings in our source code. Running another Rust WebIDL lexer and parser seems wasteful to me... unless the idea is to eventually replace the python one?

[aeweston98]

@jdm I would like to work on this issue. Please let me know if you think I can do it. Thanks!

[aeweston98]

@jdm I am still interested to work on this issue, if it is available.

[jdm]

Sorry, I was on vacation for all of April. There are four main parts to this project:

• extend the dom_struct macro to add a new #[webidl] attribute to structures marked #[dom_struct] (components/dom_struct/lib.rs)
• create a new compiler plugin (modeled after components/script_plugins/unrooted_must_root.rs) which checks for the presence of the #[webidl] attribute, then uses the name of the struct to open the equivalent webidl file found in components/script/dom/webidls
• parse the webidl file using the webidl crate when the file has been opened successfully
• determine the parent interface of the matching interface that has been parsed, and verify that the first field of the rust structure matches that interface name

[jdm]

As to @KiChjang's concerns about compilation time, if we can get this working I think we should figure out how long this check takes in isolation, and whether it affects overall build time significantly.

[aeweston98]

Ok, great I will start working on that. Can you assign the issue to me? Thanks!

[aeweston98]

@jdm I have read the rust docs for Attributes and Procedural Macros and I just want to clarify a few things to make sure I understand correctly. I have taken bullet 1 to mean add a #[webidl] Token to the attributes set in the existing dom_struct macro, like so. Based on the code in unrooted_must_root.rs, the new plugin will then check for the new webidl attribute using the LateContext.tcx.has_attr function. Please let me know if I have misinterpreted anything, otherwise I will start on the compiler plug-in, thanks!

[jdm]

That is correct.

[aeweston98]

@jdm I have made the skeleton for the compiler plugin (based on unrooted_must_root.rs). I added a new file verify_webidl.rs and modified lib.rs.

First, based on the must_root plugin, I chose the LateLintPass trait for WebIdlPass, however I wanted to check if that is correct since there are other LintPass variants that could be used. Additionally, I thought that I should define the function check_struct_def, since this should give access to both the attributes of the struct (to check for webidl attribute) and the string field which is the parent interface name as a TyStr. That should be the only function to implement for WebIdlPass as far as I can tell.

If the above is correct, I will start to work on the actual opening, parsing and checking of the webidl file. Thanks!

[KiChjang]

@aeweston98 The logic in your verify_webidl.rs makes sense, though I think you can get the DefId of the struct itself by calling cx.tcx.hir.local_def_id. Have you made any further progress on this?

[aeweston98]

@KiChjang I am in school right now, so I have been busy for a little while and haven't had a chance to progress further. I would like to continue working on it now, if that is ok.

[jdm]

Ack, I'm sorry for missing the previous comment. You are welcome to continue working on it :)

[aeweston98]

@jdm Sorry for the slow progress. The past few weeks have been very busy with midterms and co-op interviews. I have been making some progress on using the webidl crate to parse the files. I will try to have some results and/or questions by the end of the weekend. Thanks!

[jdm]

@Manishearth Is this redundant with the PR you merged recently?

[Manishearth]

Yes, however mine is a somewhat hacky bridge between the Rust codegen and Python codegen, so improving it would be nice. But not much of a priority.

[KiChjang]

@aeweston98 Are you still working on this?

[aeweston98]

@KiChjang No I am not actively working on it

[CYBAI]

Yes, however mine is a somewhat hacky bridge between the Rust codegen and Python codegen, so improving it would be nice. But not much of a priority.

This is #21105

[krk]

@highfive assign me

[highfive]

Hey @krk! Thanks for your interest in working on this issue. It's now assigned to you!

[krk]

I have a draft PR checking the first field of #[webidl] structs. Currently, it prints errors for the structs that have Reflector as the first field.

Example error:

error: webidl-rust inheritance mismatch, rust: "XRViewport", rust parent: "Reflector", webidl parent: ""
  --> components/script/dom/xrviewport.rs:13:12
   |
13 | pub struct XRViewport {
   |            ^^^^^^^^^^

Definition of XRViewport:

servo/components/script/dom/xrviewport.rs

Lines 12 to 14 in 1720454

	#[dom_struct]
	pub struct XRViewport {
	reflector_: Reflector,

How should I handle the Reflector?

[KiChjang]

@krk If the WebIDL interface does not have a parent, then yes, we should check whether the first field is a Reflector, since that's ultimately the JavaScript object that represents the Rust implementation of the WebIDL interface.

[krk]

@KiChjang After requiring Reflector for interfaces that do not extend other interfaces and handling PaintRenderingContext2D as per

servo/components/script/dom/bindings/codegen/CodegenRust.py

Lines 2205 to 2208 in a14b952

	# PaintRenderingContext2D embeds a CanvasRenderingContext2D
	# instead of a Reflector as an optimization,
	# but this is fine since CanvasRenderingContext2D
	# also has a reflector

there are two errors left in the build:

1. WindowProxy definition is not in WindowProxy.webidl but in Window.webidl. Is this a bug?

error: No such file or directory (os error 2)
  --> components/script/dom/windowproxy.rs:59:12
   |
59 | pub struct WindowProxy {
   |            ^^^^^^^^^^^

error: webidl-rust inheritance mismatch, rust: "TestBindingProxy", rust parent: "Reflector", webidl parent: "TestBinding"
  --> components/script/dom/testbindingproxy.rs:13:12
   |
13 | pub struct TestBindingProxy {
   |            ^^^^^^^^^^^^^^^^
   |
   = note: #[deny(webidl_inherit_correct)] on by default

[jdm]

Interesting! The WindowProxy declaration can probably move to WindowProxy.webidl; I'm not really sure why it was put there originally. And the TestBindingProxy issue looks like a legitimate bug.

[krk]

I have moved WindowProxy declaration to its own file.

[krk]

Search for TestBindingProxy in the repo does not reveal much, it is only declared and has an impl in

servo/components/script/dom/testbindingproxy.rs

Line 13 in eac2607

pub struct TestBindingProxy {

I declared it as

#[dom_struct]
pub struct TestBindingProxy {
    testbinding_: TestBinding,
}

Finally, ./mach build -d can finish successfully 😄