Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: add proc-macro2, quote and syn support #910

Open
wants to merge 9 commits into
base: rust
Choose a base branch
from

Conversation

ojeda
Copy link
Member

@ojeda ojeda commented Oct 10, 2022

With this, it is now possible to do use quote, use syn etc. in the macros crate.

Important note: it is not yet decided whether this will be used/allowed, but it is useful for others to prototype solutions (e.g. pinned initialization, versioning, etc.) assuming these crates are available.

The versions picked are the latest from their respective repositories. I removed the unicode-ident dependency by only allowing ASCII identifiers (i.e. the intersection of what UAX 31 allows and ASCII). This simplifies things, and in particular dissipates licensing headaches.

The syn crate is currently fully enabled (i.e. including all the optional features), which takes a fair amount of time to compile, but I wanted to check that there were no problems having it fully enabled if we ever needed it. With only the default features, it takes about a third of the time to compile. Thus, if possible, we should decide which bits we need and trim down the rest. Meanwhile, everything is there to play with it.

For proc-macro2, I used the common cfgs. We could enable unstable bits if really needed and reasonable, as usual.

I tested that the lazy_static! example works, including from out-of-tree.

ojeda added 9 commits October 10, 2022 00:34
This is a subset of the Rust `proc-macro2` crate,
version 1.0.46, licensed under "Apache-2.0 OR MIT", from:

    https://github.com/dtolnay/proc-macro2/raw/1.0.46/src

The files are copied as-is, with no modifications whatsoever
(not even adding the SPDX identifiers).

For copyright details, please see:

    https://github.com/dtolnay/proc-macro2/blob/1.0.46/README.md#license
    https://github.com/dtolnay/proc-macro2/blob/1.0.46/LICENSE-APACHE
    https://github.com/dtolnay/proc-macro2/blob/1.0.46/LICENSE-MIT

The next two patches modify these files as needed for use within
the kernel. This patch split allows reviewers to double-check
the import and to clearly see the differences introduced.

The following script may be used to verify the contents:

    for path in $(cd rust/proc-macro2/ && find . -type f -name '*.rs'); do
        curl --silent --show-error --location \
            https://github.com/dtolnay/proc-macro2/raw/1.0.46/src/$path \
            | diff --unified rust/proc-macro2/$path - && echo $path: OK
    done

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
The `proc-macro2` crate depends on the `unicode-ident` crate
to determine whether characters have the XID_Start or XID_Continue
properties according to Unicode Standard Annex torvalds#31.

However, we only need ASCII identifiers in the kernel, thus we can
simplify the check and remove completely that dependency.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
This is a subset of the Rust `quote` crate,
version 1.0.21, licensed under "Apache-2.0 OR MIT", from:

    https://github.com/dtolnay/quote/raw/1.0.21/src

The files are copied as-is, with no modifications whatsoever
(not even adding the SPDX identifiers).

For copyright details, please see:

    https://github.com/dtolnay/quote/blob/1.0.21/README.md#license
    https://github.com/dtolnay/quote/blob/1.0.21/LICENSE-APACHE
    https://github.com/dtolnay/quote/blob/1.0.21/LICENSE-MIT

The next patch modifies these files as needed for use within
the kernel. This patch split allows reviewers to double-check
the import and to clearly see the differences introduced.

The following script may be used to verify the contents:

    for path in $(cd rust/quote/ && find . -type f -name '*.rs'); do
        curl --silent --show-error --location \
            https://github.com/dtolnay/quote/raw/1.0.21/src/$path \
            | diff --unified rust/quote/$path - && echo $path: OK
    done

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
This is a subset of the Rust `syn` crate,
version 1.0.102, licensed under "Apache-2.0 OR MIT", from:

    https://github.com/dtolnay/syn/raw/1.0.102/src

The files are copied as-is, with no modifications whatsoever
(not even adding the SPDX identifiers).

For copyright details, please see:

    https://github.com/dtolnay/syn/blob/1.0.102/README.md#license
    https://github.com/dtolnay/syn/blob/1.0.102/LICENSE-APACHE
    https://github.com/dtolnay/syn/blob/1.0.102/LICENSE-MIT

The next two patches modify these files as needed for use within
the kernel. This patch split allows reviewers to double-check
the import and to clearly see the differences introduced.

The following script may be used to verify the contents:

    for path in $(cd rust/syn/ && find . -type f -name '*.rs'); do
        curl --silent --show-error --location \
            https://github.com/dtolnay/syn/raw/1.0.102/src/$path \
            | diff --unified rust/syn/$path - && echo $path: OK
    done

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
The `syn` crate depends on the `unicode-ident` crate
to determine whether characters have the XID_Start or XID_Continue
properties according to Unicode Standard Annex torvalds#31.

However, we only need ASCII identifiers in the kernel, thus we can
simplify the check and remove completely that dependency.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
With all the new files in place from the new crates, this patch
adds support for them in the build system so that the kernel's
`macros` crate can use all their facilities.

In particular, the `syn` crate is fully enabled, i.e. all its
optional dependencies are enabled.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@alex
Copy link
Member

alex commented Oct 10, 2022

A request from me: Can we please script importing/updating versions of these crates, and have the versions we imported recorded in a file that can be read programmatically?

Copy link
Member

@y86-dev y86-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we have been waiting for!

Small problem: I cannot use proc_macro2::* in macros, I think we can just add them to the extern crates list.

quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@
cmd_rustc_procmacro = \
$(RUSTC_OR_CLIPPY) $(rust_common_flags) \
--emit=dep-info,link --extern proc_macro \
--crate-type proc-macro --out-dir $(objtree)/$(obj) \
--extern quote --extern syn --crate-type proc-macro \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need --extern proc_macro2 here, otherwise we cannot use it in macros.

quiet_cmd_rustdoc = RUSTDOC $(if $(rustdoc_host),H, ) $<
cmd_rustdoc = \
OBJTREE=$(abspath $(objtree)) \
$(RUSTDOC) $(if $(rustdoc_host),$(rust_common_flags),$(rust_flags)) \
$(rustc_target_flags) -L$(objtree)/$(obj) \
--extern quote --extern syn \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same situation here, need --extern proc_macro2 (otherwise building docs will fail).

@@ -236,9 +299,10 @@ quiet_cmd_rustsysroot = RUSTSYSROOT
rusttest-prepare: FORCE
$(call if_changed,rustsysroot)

rusttest-macros: private rustc_target_flags = --extern proc_macro
rusttest-macros: private rustc_target_flags = --extern proc_macro \
--extern quote --extern syn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same situation here, need --extern proc_macro2 (otherwise buliding rusttest will fail).

$(call if_changed,rustc_test_library)

rusttestlib-macros: private rustc_target_flags = --extern proc_macro \
--extern quote --extern syn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same situation here, need --extern proc_macro2 (otherwise buliding rusttest will fail).

--edition=2021 \
-Drust_2018_idioms \
-Dunreachable_pub \
-Dunsafe_op_in_unsafe_fn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use --cap-lints allow instead like cargo does for non-local dependencies?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want to guarantee some level of code quality, right? Not sure what benefits this would give us.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't fix the lints anyway without diverging from upstream, which may end up hurting more than improving. And what about if rustc gets a new lint that fires on one of these crates? If lints are denied that would end up breaking the build and the only way for us to unbreak the build without diverging from upstream would be to allow the lint.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustc is very loathe to add warnings, but one of the larger categories we do add to often enough is future incompat warnings. Those eventually upgrade to errors, given enough time, if they are that bad. So the "code quality" may be just fine today but have some subtle detail that recommends a different pattern in the future, and in that case, you will eventually find the problem when it actually becomes upgraded.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In actual practice, I expect the kernel will use only well-tested, fully-developed but slightly-active projects that have their CI run about once every kernel release, or at worst once every few. So the most stringent I would recommend is running a cron job at either rustc's 6 week or the kernel's ~10 week cadence to check for new developments in dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use --cap-lints allow instead like cargo does for non-local dependencies?

Yeah, good idea, we should do this for dependencies that we have not modified (or at least very minimally for build purposes), i.e. like these, but unlike alloc for the moment.

@nbdd0121 nbdd0121 mentioned this pull request Oct 11, 2022
@@ -59,11 +60,56 @@ alloc-cfgs = \
--cfg no_rc \
--cfg no_sync

proc_macro2-skip_flags := \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While working on kproc-macros https://github.com/vincenzopalazzo/kproc-macros and see that prob_macros2 can be optional in projects like Linux Kernel (see https://github.com/dtolnay/syn/blob/master/Cargo.toml#L39)

I think we do not need to include proc_macro2 because it is used just for projects that import syn and quote with cargo.

P.: Well I should not say that if I want kproc-macros in right? 😸

Copy link
Member

@bjorn3 bjorn3 Nov 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dtolnay/syn/blob/master/Cargo.toml#L39 doesn't say that proc-macro2 is optional. It says that the default features are disabled for it. Or did I misunderstand what you meant to say?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh! you are right! damnt! I think I take a look at the wrong Cargo, maybe it was the https://github.com/vincenzopalazzo/kproc-macros/blob/main/kproc-parser/Cargo.toml#L13

Apologies!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants