1501116 - Map cbindgen-generated types to their original rust types in rust-bindgen

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Boris Chiou [:boris]]

There are more and more style types generated by cbindgen, and these types should be mapped to the original rust types [1] during rust-bindgen, so we could avoid calling "transmute()" every time.

[1] https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/layout/style/ServoBindings.toml#386-396

Comment 1

[Boris Chiou [:boris]]

There are two error I got while working on this:

1. StyleUnicodeRange:
Add the mapping: { generic = false, gecko = "mozilla::StyleUnicodeRange", servo = "cssparser::UnicodeRange" },

Errors:
 1:20.13 error[E0658]: access to extern crates through prelude is experimental (see issue #44660)
 1:20.13   --> objdirs/obj-browser-debug-no_opt/toolkit/library/x86_64-apple-darwin/debug/build/style-ec8cbb7321328f75/out/gecko/structs.rs:29:30
 1:20.13    |
 1:20.14 29 | pub type StyleUnicodeRange = cssparser::UnicodeRange;
 1:20.14    |                              ^^^^^^^^^
 1:20.14    |
 1:20.14    = help: add #![feature(extern_prelude)] to the crate attributes to enable

It seems we cannot "pub use" the external crate.


2. StyleAppearance:
Add the mapping: { generic = false, gecko = "mozilla::StyleAppearance", servo = "::value::specified::Appearance" },

Errors:
The rust-bindgen will generate an incorrect type in structs.rs:

  pub use self::super::::gecko_bindings::structs::StyleAppearanceas nsITheme_WidgetType;
                       ^^ expected identifier                    ^^ typos

This is generated from an alias type def in nsITheme.h [1]:

  using WidgetType = mozilla::StyleAppearance;

[1] https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/gfx/src/nsITheme.h#58

Looks like if we link `using` to the cbindgen-generated type, the rust-bindgen may generate an incorrect result? Maybe it's a rust-bindgen issue.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

(In reply to Boris Chiou [:boris]  (UTC-7) from comment #1)
> There are two error I got while working on this:
> 
> 1. StyleUnicodeRange:
> Add the mapping: { generic = false, gecko = "mozilla::StyleUnicodeRange",
> servo = "cssparser::UnicodeRange" },

You should be able to use ::cssparser::UnicodeRange, right?

> 2. StyleAppearance:
> Add the mapping: { generic = false, gecko = "mozilla::StyleAppearance",
> servo = "::value::specified::Appearance" },
> 
> Errors:
> The rust-bindgen will generate an incorrect type in structs.rs:
> 
>   pub use self::super::::gecko_bindings::structs::StyleAppearanceas
> nsITheme_WidgetType;
>                        ^^ expected identifier                    ^^ typos
> This is generated from an alias type def in nsITheme.h [1]:
> 
>   using WidgetType = mozilla::StyleAppearance;
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/gfx/src/nsITheme.h#58
> 
> Looks like if we link `using` to the cbindgen-generated type, the
> rust-bindgen may generate an incorrect result? Maybe it's a rust-bindgen
> issue.

This is totally not a rust-bindgen issue, if only it's a bug in the replacement code that we have, which may not account for these kinds of patterns. What's generated before the replacement probably looks like:

pub use self::super::mozilla::StyleAppearance as nsITheme_WidgetType;

(Or something like that)

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 6: Use alias for StylePathCommand.

So we could drop the transmute function while handling the svg path.

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 3: Use alias for StyleDisplay and StyleDisplayMode.

StyleDisplay is mapped to ::values::specified::Display and
StyleDisplayMode is mapped to ::gecko::media_features::DisplayMode.

Depends on D10140

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 5: Use alias for StyleFillRule.

We map StyleFillRule to ::values::generics::basic_shape::FillRule.
However, the current version of rust doesn't support "Type alias enum variants"
(https://github.com/rust-lang/rust/issues/26264):
e.g.
```
pub type StyleFillRule = ::values::generics::basic_shape::FillRule;

let v = StyleFillRule::Nonzero; // errors, we cannot access enum values
                                // via the alias.
```

Therefore, we have to work-around this by explicitly address the
original enum type. e.g. `::values::generics::basic_shape::FillRule::Nonzero`.

Depends on D10141

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 4: Use alias for font related types.

Map the following types:
1. StyleComputedFontWeightRange.
2. StyleComputedFontStretchRange.
3. StyleComputedFontStyleDescriptor.
4. StyleFontDisplay.
6. StyleFontFaceSourceListComponent
5. StyleFontLanguageOverride.

Depends on D10142

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Though I wonder, instead of doing the replacement like we're doing it (by search and replace), we should probably be using blacklisting + the option added here:

  https://github.com/rust-lang-nursery/rust-bindgen/pull/1308

So instead of blacklisting + search-and-replace, we should probably do something like:

  .module_raw_line("root::mozilla", "pub use ::values::specified::Appearance as StyleAppearance;");

That should be much cleaner IMO. Do you want to try that? Otherwise I could give that a shot if you want.

Comment 8

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
> Though I wonder, instead of doing the replacement like we're doing it (by
> search and replace), we should probably be using blacklisting + the option
> added here:
> 
>   https://github.com/rust-lang-nursery/rust-bindgen/pull/1308
> 
> So instead of blacklisting + search-and-replace, we should probably do
> something like:
> 
>   .module_raw_line("root::mozilla", "pub use ::values::specified::Appearance
> as StyleAppearance;");
> 
> That should be much cleaner IMO. Do you want to try that? Otherwise I could
> give that a shot if you want.

Sounds good. Let me try to do this. Thanks. :)

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 1: Add a special list for cbindgen types to avoid generating redundant rust types.

We will blacklist this type and add a module raw line to map the gecko
type to its rust type (as an alias).

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 2: Use alias for StyleAppearance.

So we could avoid generating it in rust-bindgen and drop transmute.

Depends on D10303

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 7: Use alias for StyleUnicodeRange.

Depends on D10140

Comment 12

[Boris Chiou [:boris]]

Attachment: Bug 1501116 - Part 8: Drop "mozilla" prefix in cbindgen_types in ServoBindings.toml.

A minor update to drop the redundant "mozilla" namespace prefix in
`cbindgen_types` array.

Depends on D10305

Comment 13

[Boris Chiou [:boris]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c5783c0586693deb15d2391a407e9e17b84dc4b

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd02ae8fbc9b
Part 1: Add a special list for cbindgen types to avoid generating redundant rust types. r=emilio
https://hg.mozilla.org/integration/autoland/rev/288229844b58
Part 2: Use alias for StyleAppearance. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a01b320f6684
Part 3: Use alias for StyleDisplay and StyleDisplayMode. r=emilio
https://hg.mozilla.org/integration/autoland/rev/64e506c8b6a7
Part 4: Use alias for font related types. r=emilio
https://hg.mozilla.org/integration/autoland/rev/46a60644a820
Part 5: Use alias for StyleFillRule. r=emilio
https://hg.mozilla.org/integration/autoland/rev/16dc8e99aab0
Part 6: Use alias for StylePathCommand. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cf443da6bdeb
Part 7: Use alias for StyleUnicodeRange. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7fac83679690
Part 8: Drop "mozilla" prefix in cbindgen_types in ServoBindings.toml. r=emilio

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/fd02ae8fbc9b
https://hg.mozilla.org/mozilla-central/rev/288229844b58
https://hg.mozilla.org/mozilla-central/rev/a01b320f6684
https://hg.mozilla.org/mozilla-central/rev/64e506c8b6a7
https://hg.mozilla.org/mozilla-central/rev/46a60644a820
https://hg.mozilla.org/mozilla-central/rev/16dc8e99aab0
https://hg.mozilla.org/mozilla-central/rev/cf443da6bdeb
https://hg.mozilla.org/mozilla-central/rev/7fac83679690