Add a Rust implementation of the Perl preprocessing logic. #279

jeremyroman · 2023-08-01T21:35:24Z

This implementation uses a full HTML parser (Servo's, html5ever) and operates on a DOM. It ends up being somewhat more verbose than Perl, but hopefully also more extensible/maintainable.

jeremyroman · 2023-08-01T21:37:21Z

Currently, this only builds with Rust if PROCESS_WITH_RUST=1 exists in the environment. It also uses cargo run to invoke the binary, rather than assuming it always exists and is up to date (as we could perhaps do on the server).

Still, it works. It's verbose because it's hard to beat PCRE operating on text for conciseness when operating on an actual DOM tree.

@domenic @domfarolino

domenic · 2023-08-02T04:37:54Z

This is great!! I didn't realize there was so much preprocessing in those scripts...

I'm honestly not going to be able to provide great Rust review. Maye @domfarolino can help with that. But really, even not-well-reviewed Rust with unit tests is better than untested Perl (or Pascal). So we should bias toward merging this.

It seems like, given how you've set this up, there are no blockers to merging this. (Although maybe we should update the README to document things a bit?) Things to do before we ship this:

Update the build.whatwg.org server so that people who don't have Rust installed, have an alternate path. Given how the script you've created here takes as input the whole source dir (because it needs the demos and other "boilerplate"), probably the right path is to just allow the entire html-build script to be run server-side, given a compressed zip of the source dir.
Optional: stop having Wattsi use nonstandard void <ref> tags, so we can rip out some of the extra code you had to add here.

I hope to tackle these things myself.

Instead of a nonstandard void element <ref spec=FOO>, use the text content: <ref>FOO</ref>. This allows us to use standard HTML parser/serializer tooling with the HTML source, e.g. for whatwg/html-build#279.

domfarolino

I've only made it through a little of this PR so far but it's looking really great, especially for a Rust noob like me :)

domfarolino · 2023-08-02T19:16:10Z

src/represents.rs

+            }
+            NodeData::Comment { ref contents } if contents.starts_with("REPRESENTS ") => {
+                self.placeholders
+                    .push((node.clone(), contents.subtendril(11, contents.len32() - 11)));


Btw this line means that  would not work, right? (Because of the extra space before "REPRESENTS"). Is this how the current Perl behaves too? I suppose it doesn't matter because all usages of this in the spec are uniform, but I'm just double checking.

The current regex is , which also does not accept a space there. In theory we could try to define a more consistent syntax if we wanted to.

build.sh

Instead of a nonstandard void element <ref spec=FOO>, use the text content: <ref>FOO</ref>. This allows us to use standard HTML parser/serializer tooling with the HTML source, e.g. for whatwg/html-build#279.

This implementation uses a full HTML parser (Servo's, html5ever) and operates on a DOM. It ends up being somewhat more verbose than Perl, but hopefully also more extensible/maintainable.

domenic · 2023-08-06T11:21:24Z

I pushed a commit that removed the the custom <ref> stuff. Please take a look, and especially if there's more we could remove?

Also, I'm wondering how necessary parser.rs is. Could we just use read from stdio using non-async IO? Or could we use an existing package like https://crates.io/crates/html5ever-stream/0.1.0 ?

It was only different from html5ever::driver::Parser in that it used the filtered tokenizer. With that gone, the ordinary Parser struct works.

jeremyroman · 2023-08-08T20:31:20Z

There was indeed more that can be removed; I've done so. All that's left that could be replaced by such a crate is basically the 20-line read loop (which is basically the contents of Parser::read_from).

It could also be replaced by just doing this synchronously, which would be fine in practice. I generally like the idea of being async because it's a little more annoying to retrofit async than to do it up-front, and it's not a lot of code to do here. In principle I like the idea of the build server being able to one day do this processing in a single processing without needing to spawn a separate thread per request, too, even though in practice I realize the build server is not that busy.

domfarolino

Made it through a bit more of this PR, I'll try to review the rest of it over the next couple of days.

domfarolino · 2023-08-22T19:35:08Z

src/main.rs

+
+    // Because parsing can jump around the tree a little, it's most reasonable
+    // to just parse the whole document before doing any processing. Even for
+    // the HTML5 specification, this doesn't take too long.


Hmm, maybe just "HTML Standard" instead of "HTML5 specification"

src/parser.rs

domfarolino · 2023-08-22T20:20:24Z

src/parser.rs

+                    tendril_sink.process(tendril);
+                    break;
+                }
+                Err(ref e) if e.kind() == io::ErrorKind::Interrupted => {}


Am I correct in thinking that this loop may just repeat N times hitting this "interrupted" condition, and we just keep it going until it falls into either the Ok(n) or Ok(0)? Do you know why this is the case? It's not really obvious to me, but I might just be dumb.

This is a common pattern with I/O APIs on POSIX platforms. When a sync syscall is interrupted by a signal handler before any data is read from certain file descriptors, it may return EINTR, which signifies that the operation was interrupted and it's left to userspace to reattempt. See, e.g., https://man7.org/linux/man-pages/man2/read.2.html#:~:text=EINTR%20%20The%20call%20was%20interrupted%20by%20a%20signal%20before%20any%20data%20was%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20read%3B%20see%20signal(7).

This is presumably just a reflection of that API design choice into the Rust std::io module.

Ohh yeah, no I've definitely seen this before in Chromium as well: https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=e4622aaeccea84652488d1822c28c78b7115684f. I should've caught that :)

domfarolino · 2023-08-22T21:05:56Z

src/dom_utils.rs

+}
+
+impl NodeHandleExt for Handle {
+    fn parent_node(&self) -> Option<Handle> {


Can you help me understand the need for this helper? On the surface it seems to just be a more complicated version of the .parent getter, but I'm sure I must be missing something? Is there a benefit to setting the parent to a weak version of itself, and returning the strong ref here?

self.parent is a Cell<Option<Weak<Node>>>: https://docs.rs/markup5ever_rcdom/latest/markup5ever_rcdom/struct.Node.html#structfield.parent

See this for a more detailed discussion of interior mutability with cells: https://doc.rust-lang.org/std/cell/index.html#cellt

Since Weak<> isn't Copy, neither is Option<Weak<>>. Thus the only way to get the value of the cell is to move it out of the cell (here, done with take).

Once we have that weak handle, we can obtain the strong handle that callers generally want to use (because callers here are assuming that the parent does, in fact, exist). Then, since we took the weak handle out, we need to put it back to avoid changing the state of the node.

So, uh, yes, it's a more complicated version of a getter to satisfy the static and dynamic safety checks that apply to interior mutability cells.

This implementation uses a full HTML parser (Servo's, html5ever) and operates on a DOM. It ends up being somewhat more verbose than Perl, but hopefully also more extensible/maintainable.

It was only different from html5ever::driver::Parser in that it used the filtered tokenizer. With that gone, the ordinary Parser struct works.

domenic · 2023-09-13T06:10:45Z

Cargo.toml

+tokio = { version = "1", features = ["full"] }
+html5ever = "*"
+markup5ever_rcdom = "*"
+tempfile = "3"


Seems like tempfile should be a dev-dependency, technically. (Although it doesn't really matter since this isn't a library.)

domenic · 2023-09-13T06:11:36Z

Cargo.toml

+[dependencies]
+tokio = { version = "1", features = ["full"] }
+html5ever = "*"
+markup5ever_rcdom = "*"


We should probably pin these to their current versions? We can hopefully rely on dependabot to update them over time.

domenic · 2023-09-13T06:13:34Z

src/boilerplate.rs

+        let string = tokio::fs::read_to_string(path).await?;
+        Ok(StrTendril::from(string).into_send())
+    })
+}


Should these two functions move to a utils.rs, maybe?

Here and elsewhere, I'm not really sure; I can understand keeping things more encapsulated/local, but it would also be a shame if someone in the future realized they needed something like this for another processor and didn't know about the helpers.

Moved to an io_utils.rs.

domenic · 2023-09-13T06:15:25Z

src/main.rs

+
+    // Find the paths we need.
+    let cache_dir = path_from_env("HTML_CACHE", ".cache");
+    let source_dir = path_from_env("HTML_SOURCE", "../html");


Maybe we should not have defaults here, and only specify the defaults in build.sh. WDYT?

Selfishly, this makes quick uses of cargo run a little easier.

domenic · 2023-09-13T06:16:26Z

src/main.rs

+    // conflicts between them.
+    boilerplate.apply().await?;
+    represents.apply()?;
+    annotate_attributes.apply().await?;


Is it worth trying to make concurrent the two async ones?

Realistically they already do a fair amount of spawned work concurrently (for example, opening files in boilerplate.visit()), which we need to wait for to finish. I don't want the order of these changes to be nondeterministic, though.

src/represents.rs

src/tag_omission.rs

domenic · 2023-09-13T06:20:53Z

src/boilerplate.rs

+            // demand.
+            NodeData::Comment { ref contents } if contents.starts_with("BOILERPLATE ") => {
+                let path = Path::new(contents[12..].trim());
+                if is_safe_path(path) {


Why do we have this is_safe_path check? If we omitted it, what bad thing would happen? Could we assert it, instead of silently skipping the comment if it fails? (Here and below.)

It makes it harder for someone to submit an HTML source file and use it to read back the contents of a file on the build server, which might contain some data we'd rather not have exfiltrated.

Made it an error.

domfarolino

Apologies for the big delay here! Basically looks fine to my somewhat-untrained eyes, but have a few questions here.

domfarolino · 2023-10-11T13:40:28Z

src/annotate_attributes.rs

+        //   <dd><span data-x="attr-a-href">href</span></dd>
+        //   <dd><span data-x="attr-a-someattribute">someattribute</span></dd>
+        //   ...
+        fn is_content_attribute_dt(dt: &Handle) -> bool {


Why is this a separate function btw? It never seems to be called elsewhere, except right below this.

Basically because it looked more awkward (to me) to write as a boolean expression, since early-returning would do something a little confusing, and writing it without early returns would result in some strange nesting.

domfarolino · 2023-10-11T13:43:11Z

src/annotate_attributes.rs

+            };
+
+            // These will be strings like "attr-input-maxlength", which identify particular element-attribute pairs.
+            let data_x = QualName::new(None, ns!(), LocalName::from("data-x"));


super nit: might be good to document /*prefix=*/None here.

I thought about it, though I haven't seen that as a strong convention elsewhere and the parameter genuinely isn't super interesting here.

domfarolino · 2023-10-11T13:59:21Z

src/annotate_attributes.rs

+                .iter()
+                .filter_map(|c| c.get_attribute(&data_x).filter(|v| !v.is_empty()))
+            {
+                // Find the <!-- or: --> comment, if one exists, and extract its contents.


I'm not seeing any comments that match <!-- or: in the source file, yet there appear to be tests for this. Am I missing something in the preprocessing flow?

There is exactly one, but it doesn't have a space which is probably why you didn't fine it. It's on the name attribute as applied to various form elements, and reads:

src/interface_index.rs

domenic

Alright, let's do this.

domenic mentioned this pull request Aug 2, 2023

Change <ref> syntax whatwg/wattsi#152

Merged

domfarolino reviewed Aug 2, 2023

View reviewed changes

jeremyroman and others added 3 commits August 6, 2023 19:44

Add a Rust implementation of the Perl preprocessing logic.

b4767de

This implementation uses a full HTML parser (Servo's, html5ever) and operates on a DOM. It ends up being somewhat more verbose than Perl, but hopefully also more extensible/maintainable.

Remove custom <ref> stuff

3db1b5f

Apply cargo fix

8a18ae9

domenic force-pushed the rust branch from fbd6854 to 8a18ae9 Compare August 6, 2023 11:20

domenic and others added 2 commits August 7, 2023 20:42

Change method name to avoid clippy suppression

ac0b2ea

Remove FilteredParser.

41d864f

It was only different from html5ever::driver::Parser in that it used the filtered tokenizer. With that gone, the ordinary Parser struct works.

domfarolino reviewed Aug 22, 2023

View reviewed changes

two comments from @domfarolino

f89ca54

domenic mentioned this pull request Aug 31, 2023

Allow full html-build runs at new endpoint whatwg/build.whatwg.org#84

Merged

jeremyroman and others added 10 commits September 13, 2023 02:41

Add a Rust implementation of the Perl preprocessing logic.

3023f66

This implementation uses a full HTML parser (Servo's, html5ever) and operates on a DOM. It ends up being somewhat more verbose than Perl, but hopefully also more extensible/maintainable.

Remove custom <ref> stuff

8c0a8d4

Apply cargo fix

19b6e8e

Change method name to avoid clippy suppression

cff9c93

Remove FilteredParser.

f4f7959

It was only different from html5ever::driver::Parser in that it used the filtered tokenizer. With that gone, the ordinary Parser struct works.

two comments from @domfarolino

9d3ad67

Update Docker stuff

d45608d

Better Dockerification

e9c5389

Thanks shellcheck

b077027

Build for release

732cdc1

domenic force-pushed the rust branch from c9eaa69 to 732cdc1 Compare September 13, 2023 00:41

domenic reviewed Sep 13, 2023

View reviewed changes

domfarolino reviewed Oct 11, 2023

View reviewed changes

jeremyroman and others added 13 commits October 16, 2023 16:56

warn on more than one copy of MARKER

91fd569

only accept one marker for interface index

6da2b82

Merge branch 'rust' of ssh://github.com/jeremyroman/html-build into rust

e2db792

add a comment requested by domfarolino

3694299

error rather than ignore unsafe paths

3c5b2eb

make absolute and .. paths an error rather than ignored

cf304a0

io utils

401b997

move heading_level into dom_utils

e9f39c3

format

7b438db

Cargo.toml fixups

4398c55

Invocation changes

438f9ce

Relesae is cargo install default apparently

831cb06

More Dockerfile tweaks

bd57b95

domenic approved these changes Oct 23, 2023

View reviewed changes

Merge branch 'main' into rust

9a81fd4

domenic merged commit b5866ef into whatwg:main Oct 23, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Rust implementation of the Perl preprocessing logic. #279

Add a Rust implementation of the Perl preprocessing logic. #279

jeremyroman commented Aug 1, 2023

jeremyroman commented Aug 1, 2023

domenic commented Aug 2, 2023 •

edited

Loading

domfarolino left a comment

domfarolino Aug 2, 2023

jeremyroman Aug 23, 2023

domenic commented Aug 6, 2023

jeremyroman commented Aug 8, 2023

domfarolino left a comment

domfarolino Aug 22, 2023

jeremyroman Aug 23, 2023

domfarolino Aug 22, 2023

jeremyroman Aug 23, 2023

domfarolino Aug 23, 2023

domfarolino Aug 22, 2023

jeremyroman Aug 23, 2023

domenic Sep 13, 2023

domenic Sep 13, 2023

domenic Sep 13, 2023

jeremyroman Oct 17, 2023

domenic Sep 13, 2023

jeremyroman Oct 17, 2023

domenic Sep 13, 2023

jeremyroman Oct 17, 2023

domenic Sep 13, 2023

jeremyroman Oct 16, 2023

domfarolino left a comment

domfarolino Oct 11, 2023

jeremyroman Oct 16, 2023

domfarolino Oct 11, 2023

jeremyroman Oct 16, 2023

domfarolino Oct 11, 2023

jeremyroman Oct 16, 2023

domenic left a comment

Add a Rust implementation of the Perl preprocessing logic. #279

Add a Rust implementation of the Perl preprocessing logic. #279

Conversation

jeremyroman commented Aug 1, 2023

jeremyroman commented Aug 1, 2023

domenic commented Aug 2, 2023 • edited Loading

domfarolino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Aug 6, 2023

jeremyroman commented Aug 8, 2023

domfarolino left a 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

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

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

Choose a reason for hiding this comment

domfarolino left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

domenic commented Aug 2, 2023 •

edited

Loading